Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add Dependency Injection and Hosting support for OpenFeature #310
base: main
Are you sure you want to change the base?
feat: Add Dependency Injection and Hosting support for OpenFeature #310
Changes from 52 commits
963b54c
a803429
79ee277
9f3c40e
6786bd8
f373eb0
1f86298
f1039a2
5905cc6
6c3c15e
bd20d40
441aa4f
fa04665
fcea8a6
fc8dcab
0a996aa
76e1816
84dd0e0
572d67b
d7e5fbb
f60b5d8
8b59e53
c644df6
f4f108b
43bcde9
47e87c2
3650de2
0027489
5638163
5de7ceb
2e99210
880df92
9258706
19664b0
9c21943
a23c5ea
7906a3b
1850159
63ead66
70708ec
d615b04
ad95765
ce23cdc
7716f4a
12d48c2
1f0468e
d480da4
cc5f5bc
90d172c
2072e45
c503b30
9a5a343
be43fcc
84f8e18
94f4d66
756a7f7
36b5f8c
c9339ec
980d0be
d4a5e3d
8387def
5077a1c
95e8ddc
f157496
49991ec
5bc2c47
9ea9b01
4066b89
367518b
70c269f
b20bc13
c251f56
8b2ab4b
3800447
e111714
cb63b1c
0e53494
dc765e6
4f80593
0a76193
ac98ddd
4af39ce
0fa6dc3
926c0d4
9b8d9f2
aa488ae
1f8e9e4
3aaa7df
ffef1ac
1acb4fa
96889dd
54c145e
55582d7
8ebdad4
3089129
fe5e072
10ea9f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for not resolving
FeatureProvider
directly in the ctor beside having custom exception for that situation? In effect it not allow call-site validation to detect thatFeatureProvider
is missing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for not resolving FeatureProvider directly in the constructor is that some implementations of FeatureProvider are not fully controlled by us, and they may execute significant logic during construction. For example, certain providers like LaunchDarkly perform extensive initialization within the constructor, which can take time and potentially fail (as seen in LaunchDarkly's Provider Implementation and LdClient Constructor).
By not resolving FeatureProvider in the constructor, we can safely perform this logic asynchronously in a start method, which avoids delays or failures during object instantiation. Additionally, I use GetService to log meaningful messages in case of any issues.
In short, this approach allows us to handle potential failures asynchronously during startup and ensures proper logging in case of errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally providers are doing such initialization in their
initialize()
method, but we can't control them all, and I see your point. Generally I'd prefer constructor resolution as well but I can understand the advantages you describe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want an optional value for domain, or some other method for working with domain-scoped providers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider using PolySharp. It is smart and customizable source generator which will detect what exact feature need to be poly-filled and generate the code for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I’ll definitely take a closer look and see how it can fit into our current approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arttonoyan / @wwalendz-relativity, thanks for bringing this to our attention. I need someone else to confirm if bringing an extra library to use these polyfils is okay.
@toddbaert or @beeme1mr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think it's better just do use the version in the #else here; using both a "nice" API and a "less nice" API is more complex than just using the "less nice" API, IMO.
Unless there some performance implication here I think this just hurts readability a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the pattern/reasoning behind using a
sealed record
and extensions here?I have not too much experience with .NET so I am mostly wondering why this is the chosen/preferred way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukas-reining Thanks for your review.
Reasoning for using a
record
inOpenFeatureBuilder
and extension methods:Immutability:
Using a
record
inOpenFeatureBuilder
ensures immutability, which is crucial in configurations where state changes should not happen after initialization. Records in C# provide an easy way to define immutable types and handle value-based equality. This matches the pattern used in other parts of ASP.NET Core, like the AuthenticationBuilder class, where the object holds configuration data (likeIServiceCollection
) and doesn’t change its internal state directly.Following .NET Patterns (e.g., AuthenticationBuilder):
The
AuthenticationBuilder
is a great example from ASP.NET Core, where the builder encapsulatesIServiceCollection
and allows adding services via extension methods. Our approach toOpenFeatureBuilder
follows the same design, where extension methods add specific configuration options, making the builder pattern more extensible without altering its core structure. Microsoft actively uses this builder pattern across many of their packages, not just for authentication. It's proven to be a robust way to add functionality in a clean and modular manner.Extension Methods Simplify Extensibility:
Like how GoogleExtensions.cs and other authentication schemes are added via extension methods on
AuthenticationBuilder
, the same idea applies toOpenFeatureBuilder
. This allows developers to extend the functionality ofOpenFeatureBuilder
in a modular way, adding new features without needing to modify or subclass the builder itself. This aligns with Microsoft’s general approach to keep core objects clean and extensible via extensions.Many Uses Across Microsoft Packages:
While
AuthenticationBuilder
is a useful example, this builder and extension pattern is widely used throughout Microsoft’s packages, such as in DI (Dependency Injection), logging, and configuration. It allows developers to write clean, easily maintainable code that is open to extensions without modifying the core framework.Reconsidering the
sealed
Keyword:In some cases, the use of
sealed
on a builder might restrict extensibility via inheritance. Although the builder pattern usually focuses on extension methods rather than inheritance, removingsealed
can be beneficial if you envision a need for developers to extendOpenFeatureBuilder
through inheritance. It opens up the possibility for advanced users to create custom builders while still maintaining the base functionality.In summary, the use of a
record
forOpenFeatureBuilder
fits well with established patterns in .NET, likeAuthenticationBuilder
, making it immutable and easily extensible through extension methods. The use of this pattern is not isolated; it’s widespread across Microsoft packages and proves to be a useful approach. The consideration of removingsealed
can further enhance the extensibility of the builder, allowing more advanced use cases where inheritance might be needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the
sealed
modifier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @arttonoyan!
I am still not sure why we use the record.
If I am not looking at the wrong place, we do not have any data.
The
AuthenticationBuilder
is aclass
and not arecord
.The
AuthenticationBuilder
also provides the basic functionality directly so maybe we would only need the extensions from src/OpenFeature.Hosting/OpenFeatureBuilderExtensions.cs but we could inline src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs as they are directly defined in the same package.I have no hard opinion on this, I would just like to understand the reasoning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in our case, there isn't a significant difference between using a class or a record. The main reason I opted for a record is because it allows for a more concise and compact syntax, especially when the class primarily holds immutable data, like the Services property. Functionally, it would be nearly identical to using a class like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our case we have one immutable property
Services
and one mutableIsContextConfigured
.This is not too important I guess but I am just wondering why we should do different then e.g.
ASP.NET Core
in your example.As you said you removed
sealed
for extensibility, but I would argue that classes, as forAuthenticationBuilder
, would be more idiomatic and extensible than extending a record as "a class can not inherit from a record" and the pattern for extending OpenFeature would be different than for theASP.NET Core
built-ins: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record#inheritanceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the use of
record
also caused confusion. I had to read a bit to understand why we'd use it in this case.Personally, since other builders we have are implemented as classes I think I'd prefer that, but it's not a strong preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukas-reining @toddbaert Thank you for your thoughtful feedback! I completely understand your point, especially regarding consistency with other builders being implemented as classes. I can see how using a record in this case might cause some confusion. It’s not a strong preference for me either, I will change with the more familiar pattern of using classes for builders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add, other reason why we should seal classes is in dotnet 6, we got some small performance improvements: https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-6/#“peanut-butter”
As for the record, I would prefer to start using it by the reasons that @arttonoyan mentioned before. But I am happy to keep as a class until we decide to move it to record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure: why is this
internal
?If our package benefits from
IsContextConfigured
could it be beneficial for external extensions too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! I initially made it internal because it was primarily used for internal configuration, and the public setter was intended for use within the internal scope. However, you raise a good point. There's no strong reason not to make it public, as it could provide useful information for consumers. We can certainly change it to:
public bool IsContextConfigured { get; internal set; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having branching in all places where we would like to consume
ExecutionContext
we could always register empty instance ofExecutionContext
. This will end up with more "pure" methods. In that caseIsContextConfigured
is not needed.services.TryAddSingleton(ExecutionContext.Empty);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t register the EvaluationContext as a singleton because the client can operate without any context or with null. Under the hood, it checks and sets the Empty context as needed. The purpose of IsContextConfigured is to avoid redundant service resolution and nullability checks. If no context is registered, we bypass fetching the EvaluationContext entirely, preventing unnecessary GetService or GetRequiredService calls.
Also, when the context is registered, I register it as transient. The reason for this is to control its lifecycle within the scope of the class where it’s used—in this case, IFeatureClient. Will be confusing to register the context as transient in one case and Empty as singleton in another. However, this approach ensures proper lifecycle management based on the context's availability and usage.
Additionally, the IsContextConfigured check is only performed once during the service provider building process. In scenarios where context resolution isn’t required, the IFeatureClient can still function without a context, but engineers can always explicitly use ExecutionContext.Empty if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context can be added at both the global and client level. Global context is most useful for static values (for example, a value representing the timezone of the server or the cloud provider it's running on, or the application version).
We might need multiple contexts setting methods for the different scopes: global, client, and perhaps transaction level once we implement the transaction propagation feature. As it is, I'm not sure it's obvious to a user which one would be used when they do
AddContext(...)