Skip to content

Conversation

jblacker
Copy link
Contributor

@jblacker jblacker commented May 15, 2025

This PR

Implements the Multiprovider specified in "Appendix A: Included Utilities" of the OpenFeature Spec. This implementation is an improvement upon what I initially implemented in the go-sdk-contrib repository here & here.

This includes implementations for all three strategies described in the spec along with some common functions. Unlike my original implementation this includes full support for hooks & event handling from the included handlers.

Related Issues

open-feature/go-sdk-contrib#599

Notes

I reimplemented this here from the go-sdk-contrib repo on suggestion of other members. This includes the refactoring improvements that I was mentioning in discussions on my open PR over there.

This PR is currently in draft because it is built on top of the exported mocks in order to provide a full unit test suite.

Follow-up Tasks

  • e2e / Gherkin tests should be written
  • Minor clean up & improvements

How to test

  • Create an application and pass a multiprovider to the OpenFeature SDK that includes 2 or more providers. Verify that the results are as expected per the selected strategy.

jblacker added 16 commits May 7, 2025 15:55
- Also updated the generated filenames to remove the test prefix allowing them to be exportable
- Added a sed command to prepend the build tag "testtools" to the mock files
- Updated test commands to include the "testtools" build tag
- Added step to delete the backups generated by sed

Signed-off-by: Jordan Blacker <[email protected]>
- Remove legacy mocks
- Update Makefile

Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
@jblacker jblacker changed the title Implement multiprovider feat: Implement multiprovider May 15, 2025
@erka
Copy link
Contributor

erka commented Jun 3, 2025

Hey @jblacker.

Is there any road block to move this forward? Do you need any help?

@jblacker
Copy link
Contributor Author

jblacker commented Jun 3, 2025

I've actually been on vacation. I'm going to pick back up on this tomorrow.

jblacker and others added 7 commits June 4, 2025 17:05
Signed-off-by: Jordan Blacker <[email protected]>
This is due to the fact that slog.DiscardHandler is not introduced until go 1.24

Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
This reverts commit ea29d0f.

Signed-off-by: Jordan Blacker <[email protected]>
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

❌ Patch coverage is 72.35495% with 243 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.79%. Comparing base (53a89f3) to head (f722ff3).

Files with missing lines Patch % Lines
openfeature/multiprovider/multiprovider.go 62.94% 121 Missing and 5 partials ⚠️
openfeature/multiprovider/isolation.go 61.16% 82 Missing and 5 partials ⚠️
openfeature/multiprovider/comparison_strategy.go 89.13% 17 Missing and 3 partials ⚠️
openfeature/multiprovider/strategies.go 88.37% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #354      +/-   ##
==========================================
- Coverage   85.69%   80.79%   -4.90%     
==========================================
  Files          20       27       +7     
  Lines        1433     2312     +879     
==========================================
+ Hits         1228     1868     +640     
- Misses        184      408     +224     
- Partials       21       36      +15     
Flag Coverage Δ
e2e 80.79% <72.35%> (-4.90%) ⬇️
unit 80.79% <72.35%> (-4.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant and well-structured multiprovider feature to the Go SDK. The implementation of different evaluation strategies, along with robust error and event handling, is a valuable addition. The code is clean, well-tested, and demonstrates good concurrency practices. My review includes a few minor suggestions to enhance maintainability and correct a documentation inconsistency.

@sahidvelji
Copy link
Contributor

I'm going to be away until Sept 2nd, so I likely won't have the time to review thoroughly until then.

jblacker and others added 2 commits September 4, 2025 15:06
@jblacker jblacker force-pushed the implement-multiprovider branch from 304831b to 76cf8b7 Compare September 4, 2025 19:09

// NewComparisonStrategy creates a new instance of ComparisonStrategy. The fallback provider specified is called when
// there is a comparison failure -- prior to returning a default result. The Comparator parameter is optional and nil
// can be passed as long as ObjectEvaluation is never called. Unless the `alwaysUseCustom` parameter is true the default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an alwaysUseCustom parameter here. Go doc comments also don't do anything special with backticks, so you should avoid them.

Copy link
Contributor Author

@jblacker jblacker Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. That was removed during some refactoring, but I forgot to update the docs. I'll get this cleaned up. The backticks is just a bad habit from markdown. I'll clean that up too throughout.

// there is a comparison failure -- prior to returning a default result. The Comparator parameter is optional and nil
// can be passed as long as ObjectEvaluation is never called. Unless the `alwaysUseCustom` parameter is true the default
// comparisons built into Go will be used. The custom Comparator will only be used for ObjectEvaluation. However, if the
// parameter is set to true the custom Comparator will always be used regardless of evaluation type. If ObjectEvaluation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use doc links to refer to any exported identifier, like [Comparator] and [of.FeatureProvider.ObjectEvaluation]. See https://tip.golang.org/doc/comment#doclinks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated throughout.

// StrategyFirstMatch First provider whose response that is not FlagNotFound will be returned. This is executed
// sequentially, and not in parallel.
StrategyFirstMatch EvaluationStrategy = "strategy-first-match"
// StrategyFirstSuccess First provider response that is not an error will be returned. This is executed sequentially
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// StrategyFirstSuccess First provider response that is not an error will be returned. This is executed sequentially
// StrategyFirstSuccess returns the first provider response that is not an error. This is executed sequentially.

Write the other doc comments in a similar fashion. They should read like sentences and the first word after the identifier being documented should not be capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I'll update throughout

Comment on lines +28 to +29
ReasonAggregated of.Reason = "AGGREGATED"
ReasonAggregatedFallback of.Reason = "AGGREGATED_FALLBACK"
Copy link
Contributor

@sahidvelji sahidvelji Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in its own const () block since it's not an [EvaluationStrategy].

ReasonAggregated of.Reason = "AGGREGATED"
ReasonAggregatedFallback of.Reason = "AGGREGATED_FALLBACK"

ErrAggregationNotAllowedText = "object evaluation not allowed for non-comparable types without custom comparable func"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in its own const () block since it's not an [EvaluationStrategy].

Comment on lines 169 to 170
// AsNamedProviderSlice Converts the map into a slice of NamedProvider instances
func toNamedProviderSlice(m ProviderMap) []*NamedProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the doc comment to match the function name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@sahidvelji
Copy link
Contributor

Left some more comments. I didn't get time to look at the whole PR, so I may have more comments later.

@jblacker jblacker requested a review from sahidvelji September 8, 2025 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move multi-provider into SDK
4 participants