fix: collect and propagate per-provider errors in multi-provider strategies#1901
fix: collect and propagate per-provider errors in multi-provider strategies#1901suthar26 wants to merge 3 commits intoopen-feature:mainfrom
Conversation
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 improves the error handling capabilities of multi-provider evaluation strategies. By introducing a structured way to collect and expose individual provider errors, it ensures that applications can gain deeper insights into why a flag evaluation failed across multiple providers. This change brings the Java SDK's error reporting in line with the JavaScript SDK's Highlights
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. 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 effectively enhances error reporting for multi-provider strategies by collecting and propagating per-provider error details. The introduction of the ProviderError class and its integration into FirstSuccessfulStrategy and FirstMatchStrategy is well-implemented and aligns the Java SDK with the JavaScript SDK's behavior, which is a great step for consistency across the ecosystem. The changes are accompanied by thorough and well-written tests that cover various failure scenarios.
I have one suggestion regarding code duplication in the new buildAggregateMessage methods, which could be refactored for better maintainability. Overall, this is a solid improvement to the SDK's diagnostics capabilities.
| private static String buildAggregateMessage(List<ProviderError> errors) { | ||
| if (errors.isEmpty()) { | ||
| return "Flag not found in any provider"; | ||
| } | ||
| String details = errors.stream() | ||
| .map(ProviderError::toString) | ||
| .collect(Collectors.joining(", ")); | ||
| return "Flag not found in any provider. Provider errors: [" + details + "]"; | ||
| } |
There was a problem hiding this comment.
This buildAggregateMessage method is nearly identical to the one in FirstSuccessfulStrategy. To reduce code duplication, consider extracting this logic into a shared utility method. A single method that accepts the base error message as a parameter could serve both strategies and improve maintainability.
| private static String buildAggregateMessage(List<ProviderError> errors) { | ||
| if (errors.isEmpty()) { | ||
| return "No provider successfully responded"; | ||
| } | ||
| String details = errors.stream() | ||
| .map(ProviderError::toString) | ||
| .collect(Collectors.joining(", ")); | ||
| return "No provider successfully responded. Provider errors: [" + details + "]"; | ||
| } |
There was a problem hiding this comment.
This buildAggregateMessage method is nearly identical to the one in FirstMatchStrategy. To reduce code duplication, consider extracting this logic into a shared utility method. A single method that accepts the base error message as a parameter could serve both strategies and improve maintainability.
suthar26
force-pushed
the
fix/multiprovider-error-propagation
branch
6 times, most recently
from
3d818cd to
57b84a0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1901 +/- ##
============================================
+ Coverage 92.15% 93.26% +1.11%
- Complexity 647 656 +9
============================================
Files 58 59 +1
Lines 1568 1589 +21
Branches 176 179 +3
============================================
+ Hits 1445 1482 +37
+ Misses 76 62 -14
+ Partials 47 45 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
suthar26
force-pushed
the
fix/multiprovider-error-propagation
branch
2 times, most recently
from
94c667d to
db69386
Compare
chrfwow
left a comment
chrfwow
left a comment
There was a problem hiding this comment.
Is the plan to eventually also track different error codes than Flag not found? Because otherwise we could simplify this a bit
suthar26
force-pushed
the
fix/multiprovider-error-propagation
branch
from
db69386 to
59b34a2
Compare
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
Nit: the hand-rolled builder here re-declares all of ProviderEvaluation's fields, so if the parent ever gains a new field this will silently fall out of sync. Lombok's @SuperBuilder handles inheritance nicely; you'd just need to add @SuperBuilder to both ProviderEvaluation and MultiProviderEvaluation and it takes care of the rest. Alternatively, if changing ProviderEvaluation is off the table, a comment flagging the coupling would be nice.
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
Small thing: ProviderEvaluation's Lombok builder defaults flagMetadata to a non-null ImmutableMetadata via @Builder.Default, but the MultiProviderEvaluation builder doesn't set that default. Neither strategy sets it, so getFlagMetadata() will return null on these results; could cause NPEs downstream. Initializing it in the builder field (e.g. private ImmutableMetadata flagMetadata = ImmutableMetadata.builder().build()) should do the trick. This also goes away if you switch to @SuperBuilder since it'd inherit the default.
| public Builder<T> value(T value) { | ||
| this.value = value; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Small thing: ProviderEvaluation's Lombok builder defaults flagMetadata to a non-null ImmutableMetadata via @Builder.Default, but this builder doesn't set that default. Neither strategy sets it, so getFlagMetadata() will return null on these results; could cause NPEs downstream. Initializing it here (e.g. private ImmutableMetadata flagMetadata = ImmutableMetadata.builder().build()) should do the trick. This also goes away if you switch to @SuperBuilder since it'd inherit the default.
| private String variant; | ||
| private String reason; | ||
| private ErrorCode errorCode; | ||
| private String errorMessage; |
There was a problem hiding this comment.
Nit: this builder re-declares all of ProviderEvaluation's fields, so if the parent ever gains a new field this will silently fall out of sync. Lombok's @SuperBuilder handles inheritance nicely; you'd just need to add @SuperBuilder to both ProviderEvaluation and MultiProviderEvaluation and it takes care of the rest. Alternatively, if changing ProviderEvaluation is off the table, a comment flagging the coupling would be nice.
There was a problem hiding this comment.
addressed this using @SuperBuilder
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
I agree with the spirit of this PR, but I think that there's a few places the use of lombok can prevent some bugs and inconsistencies. I'm not a huge fan of lombok TBH, but we are already "in on it" so I think we should stay consistent.
…tegies Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com>
suthar26
force-pushed
the
fix/multiprovider-error-propagation
branch
from
8d6eec4 to
c06c40c
Compare
|
3 participants
This PR
FirstSuccessfulStrategy and FirstMatchStrategy now collect per-provider error details (provider name, error code, message, exception) as they iterate through providers. When all providers fail, the returned ProviderEvaluation includes:
This aligns the Java SDK with the js-sdk reference implementation, which uses AggregateError with originalErrors[] to expose per-provider failure information to callers.
Changes:
Related Issues
Closes #1900