feat: improve MultiProvider parity with JS SDK reference#113
feat: improve MultiProvider parity with JS SDK reference#113jonathannorris wants to merge 4 commits into
Conversation
- Replace fail-fast initialization with allSettled pattern: initialize and onContextSet now run all child providers to completion before throwing a MultiProviderAggregateError, matching the JS SDK's Promise.allSettled behavior - Replace DispatchQueue.sync with NSLock to avoid cooperative thread pool starvation when called from async task groups - Extract shared makeUniqueProviderNames utility, removing duplicate in ComparisonStrategy - Add ComparisonStrategy for evaluating all providers and comparing results, with fallback provider and mismatch callback support - Add per-provider hook execution with context isolation via TaskLocal-based ProviderHookExecutionContext, propagating client metadata and hints - Rename types to UpperCamelCase (ChildProviderRecord, ContextMutatingHook, HookContextCapturingHook, NamedProviderMetadata) - Remove fragile type+name fallback in ComparisonStrategy.providersMatch, using only reference identity Closes #109 Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the MultiProvider by aligning it with the JS SDK. The changes include allSettled-style error handling for initialization and context changes, a new ComparisonStrategy, and improved hook execution with context propagation. The implementation is solid and well-tested. I've provided a few suggestions to refine the code for conciseness and clarity.
| var collected: [(String, Error?)] = [] | ||
| for await result in group { | ||
| collected.append(result) | ||
| } | ||
| return collected.compactMap { name, error in | ||
| error.map { (providerName: name, error: $0) } | ||
| } |
There was a problem hiding this comment.
The error collection logic can be made more concise and efficient by using a for await loop with pattern matching, which avoids creating an intermediate array collected that includes non-error results.
| var collected: [(String, Error?)] = [] | |
| for await result in group { | |
| collected.append(result) | |
| } | |
| return collected.compactMap { name, error in | |
| error.map { (providerName: name, error: $0) } | |
| } | |
| var errors: [(providerName: String, error: Error)] = [] | |
| for await (name, error) in group { | |
| if let error { | |
| errors.append((providerName: name, error: error)) | |
| } | |
| } | |
| return errors |
| var collected: [(String, Error?)] = [] | ||
| for await result in group { | ||
| collected.append(result) | ||
| } | ||
| return collected.compactMap { name, error in | ||
| error.map { (providerName: name, error: $0) } | ||
| } |
There was a problem hiding this comment.
Similar to the initialize method, the error collection logic here can be simplified to be more concise and avoid creating an intermediate array.
| var collected: [(String, Error?)] = [] | |
| for await result in group { | |
| collected.append(result) | |
| } | |
| return collected.compactMap { name, error in | |
| error.map { (providerName: name, error: $0) } | |
| } | |
| var errors: [(providerName: String, error: Error)] = [] | |
| for await (name, error) in group { | |
| if let error { | |
| errors.append((providerName: name, error: error)) | |
| } | |
| } | |
| return errors |
| return fallbackResolution | ||
| } | ||
|
|
||
| return firstResolution ?? fallbackResolution |
There was a problem hiding this comment.
The ?? fallbackResolution is redundant. firstResolution is guaranteed to be non-nil at this point because:
- The function returns early if
providersis empty. - If
providersis not empty,firstResolutionis set on the first successful evaluation. - If all evaluations fail with an error,
encounteredErrorwill be true, and the function will throwComparisonStrategyError.providerErrors, so this line is not reached.
Force-unwrapping is safe here and makes the intent clearer that firstResolution is expected to exist.
| return firstResolution ?? fallbackResolution | |
| return firstResolution! |
…ength lint Move event/status tracking, hook execution, and logger-enabled evaluation methods into separate extension files to keep each file under the 350-line type_body_length threshold. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…lint Move event aggregation and status tracking into a standalone MultiProviderStatusTracker class (matching JS SDK's StatusTracker pattern), reducing the MultiProvider type body to ~332 lines (under the 350-line error threshold). Also simplify error collection in initialize/onContextSet using direct for-await pattern, and consolidate evaluation methods with a shared evaluateStrategy helper. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…h lint The swiftlint error was in the test file (MultiProviderTests at 488 lines), not the source. Split into MultiProviderStrategyTests (evaluation/strategy tests) and MultiProviderFeatureTests (events, hooks, tracking, lifecycle). Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
1 participant
Summary
allSettled-style initialization and context change: all child providers run to completion before throwing aMultiProviderAggregateError, matching the JS SDK'sPromise.allSettledpatternComparisonStrategyfor evaluating all providers and comparing results, with fallback provider and mismatch callback@TaskLocal-basedProviderHookExecutionContext, propagating client metadata and invocation hintsDispatchQueue.syncwithNSLockto avoid cooperative thread pool starvation in async task groupsmakeUniqueProviderNamesutility, removing duplication betweenMultiProviderandComparisonStrategyImplementation
MultiProvider.swiftallSettledinit/context pattern,NSLock,MultiProviderAggregateError, shared name dedup, UpperCamelCase typescomparisonStrategy.swiftComparisonStrategy, uses sharedmakeUniqueProviderNames, identity-only provider matchingproviderHookExecutionContext.swift@TaskLocalstorage for client metadata and hints during provider hook executionOpenFeatureClient.swiftProviderHookExecutionContextStorageMultiProviderTests.swiftNotes
ComparisonStrategyevaluates providers sequentially because theStrategyprotocol is synchronous. The JS SDK uses parallel evaluation (runMode = 'parallel'). Parallel evaluation would require making the protocol async.ProviderHookExecutionContextis only populated when evaluations go throughOpenFeatureClient. DirectMultiProviderusage will havenilclient metadata — provider hooks still run but without client context.Related Issues
Closes #109
Supersedes #110