Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking changes] Beta update #4

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

manio143
Copy link
Collaborator

Motivation

After having some thought on the current abstraction model I have decided to make the abstraction much looser to allow for more provider-led customizations. In particular the filters in GrowthBook form general expressions and are not tied to per context property filtering. This should allow more flexibility in driving features within Excos Platform and enable me to incorporate more 3rd party feature providers.

Breaking Changes

  1. Removed the IFeatureVariantOverride in order for the client to not be concerned with feature overrides. Those should be implemented via other means (e.g. a custom ILoadContextualOptions)
  2. Removed Allocation from the data model. It can be implemented by each provider as a special filter and it can be skipped for settings which are not allocated per user/session.
  3. Generalized filters over the whole context instead of being applied per property - this allows more flexible filtering logic and some performance optimizations compared to previous implementation.
  4. Variant priority is now mandatory.
  5. FeatureMetadata type that was used for injecting resolved variants as part of the options has been removed. Instead user should call the IFeatureEvaluation interface to get the list of resolved variants and their Ids.
  6. The builder interface for in code feature specification is changed a little to account for model changes.
  7. ExcosOptions class has been removed. Instead UserId is the default allocation unit for configuration provided features. You can no longer set the default globally.

Testing

I had some unit tests and they gave some confidence, but I feel like I could have missed something, so I may need to add more tests.

  • Add more tests to verify no accidental breaking changes.

Performance

By changing the abstraction model of filters and then pushing in on the IOptionsContext constraint, I was able to redesign how filtering is executed for the configuration based provider. Excos.Options is now in the same ballpark as Microsoft.FeatureManagement.

Object pools have been removed as they don't seem to be adding much positive value (allocations stay in gen 0).

GrowthBook

I was able to properly implement the GB specification (with some caveats) so now most of the unit tests are actually passing. The ones that don't have been listed in the comment as exceptions with explanations why my implementation differs.

Docs

I've updated the GB Guide and Usage docs to reflect the changes.

@manio143 manio143 requested a review from Copilot December 26, 2024 15:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 41 out of 55 changed files in this pull request and generated 3 comments.

Files not reviewed (14)
  • src/Excos.Options.GrowthBook/Excos.Options.GrowthBook.csproj: Language not supported
  • src/Excos.Options.Tests/Excos.Options.Tests.csproj: Language not supported
  • src/Excos.Options/Abstractions/Data/FilterCollection.cs: Evaluated as low risk
  • src/Excos.Options/Abstractions/Data/Filter.cs: Evaluated as low risk
  • src/Excos.Options/Abstractions/Data/FeatureCollection.cs: Evaluated as low risk
  • src/Excos.Options.Tests/Contextual/ContextReceiverTests.cs: Evaluated as low risk
  • src/Excos.Options.Tests/ConfigurationBasedFeaturesTest.cs: Evaluated as low risk
  • src/Excos.Options/Abstractions/Data/Feature.cs: Evaluated as low risk
  • docs/DataModel.md: Evaluated as low risk
  • src/Excos.Options.GrowthBook/GrowthBookFeatureParser.cs: Evaluated as low risk
  • src/Excos.Options.GrowthBook/Readme.md: Evaluated as low risk
  • docs/Extensibility.md: Evaluated as low risk
  • docs/Usage.md: Evaluated as low risk
  • src/Excos.Options.GrowthBook/GrowthBookFeatureCache.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Excos.Options.GrowthBook.Tests/Tests.cs:183

  • The property 'Id' should be accessed through 'Variants[1].Id' instead of 'features[0][1].Id'.
Assert.Equal("label:1", features[0][1].Id);

src/Excos.Options.GrowthBook.Tests/Tests.cs Show resolved Hide resolved
src/Excos.Options.GrowthBook.Tests/Tests.cs Show resolved Hide resolved
src/Excos.Options.GrowthBook.Tests/Tests.cs Show resolved Hide resolved
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.

1 participant