open-feature / dotnet-sdk Goto Github PK

Use https://benchmarkdotnet.org/ to make a suite of performance benchmarks to ensure OpenFeature.SDK has little overhead as possible.

See: #126

#129

Requirements

We already have error codes defined, but it would be nice to define error classes to make things a bit more ergonomic for contributors. Basically something like is already implemented in this contrib ; simple classes wrapping these error types that can easily be thrown.

Requirements

Based on the defined schemas in schemas repository, FeatureProvider should implement ResolveAll method to retrieve all feature flags.

This method would be extremely useful to us to be able to migrate over to OpenFeature.

Implement the most basic version of the specification to allow for retrieving flags in the defined forms (string, number, boolean, and structure)

Looking at the EvaluationContext I see that you can use any object as a value in the context. I am not clear how a provider could make use of non-trivial types, or be able to easily serialize the overall context without some level of interface requirement. (Unless they provided provider specific types, or specified specific types, but that doesn't seem very interoperable.)

In the basic use case, where there are top level basic types (string, number, boolean), I can see how a provider could request something as a type based on the meta-data that they likely have.

In the case of a structure use case I am not sure how that could be accomplished. I do see how it could be accomplished if the types were effectively restricted to each of the base types, as well as ImmutableList and ImmutableDictionary of those those types and more ImmutableLists and ImmutableDictionary.

Or is there going to be a builder which can mange those constraints?

Imagining a context of this general shape:

{
  "name": "example",
  "complex": {
    "field1": "value"
  }
}

How could a provider access the value of field1? Aside from using reflection, or using some pre-specified type?

Edit:
The node implementation contains this type, which I think expresses how this data can be made navigable and understood.

  export type JSONValue = null | string | number | boolean | Date | { [x: string]: JSONValue } | Array<JSONValue>;

Thank you,
Ryan

Make sure we use the latest xUnit version since we should not have any limitations concerning versions during the unit tests.

Originally posted by @austindrenski in #178 (comment)

Add support for evaluation context described in the specification https://github.com/open-feature/spec/blob/main/specification/evaluation-context.md

This has been discussed under open-feature/ofep#29

Allow the users to define the default logger on the client

Implement specification v0.7.0 changes

See changes since last release.

Please note there's no need to complete all these tasks in a single PR (in fact, that's not recommended).

Functional requirements:

• implement flag metadata: open-feature/spec@74c373e
• #131
  • open-feature/spec#192
• implement init/shutdown (done with #158)
• implement events: https://openfeature.dev/specification/sections/events
• #165

Non-functional requirements:

• add relevant tests
• add examples and documentation, see template README
• update specification badge to v0.7.0:

Keep in mind:

• backwards compatibility is highly encouraged
  • take every precaution to ensure existing code written by application authors and existing providers implementations, do not break
• it's recommended these features are released together

Change in the documentation

The link here: https://github.com/open-feature/openfeature.dev/blob/078f35c2b9e76437a3889dbfaf142fd70063f642/docs/reference/technologies/server/dotnet.mdx#L134

I think it is probably intended to link to a github search of some kind.

Investigate applying for myget opensource account this will be used to publish development builds

https://www.myget.org/opensource

I think it's possible to define virtual methods on interfaces. Since the Hook abstract class has no state, I think there's no reason not to simply use IHook everywhere for more flexibility (multiple inheritance, etc)

Is there a reason why the PackageReference to System.Collection.Immutable doesn't allow version 7. Version 7 has been out for 8 months now and this restriction is preventing us from being able to use this library.

Change in the documentation

We currently don't have a readme being published.

We can do follow these steps to do that: https://devblogs.microsoft.com/nuget/add-a-readme-to-your-nuget-package/#add-a-readme-to-your-package

Update the SDK to be compliant with the v0.5.1 spec.

What's Changed

• chore: add additional document statuses by @toddbaert in open-feature/spec#139
• chore: add status badges by @toddbaert in open-feature/spec#148
• chore: section clarity, harden API, providers by @toddbaert in open-feature/spec#149

Update the badge on the readme to

Full Changelog: open-feature/spec@v0.5.0...v0.5.1

Add InMemoryProvider to SDK. See spec.

Tasks:

• add provider
• add tests
• replace e2e tests here with InMemoryProvider.

see Java equivalent.

Parent issue: #126

Requirements

Check open-feature/spec@74c373e

Create a cert, store it in secure place that the OpenFeature team has access to.

https://docs.microsoft.com/en-us/nuget/create-packages/sign-a-package

I'm wondering if we could get a consensus on using release please.

Basically, it's an github action that runs after commits/merges to main. It checks for semantic commits, and if it detects a feat/fix/breaking change, it creates a running PR (see example in Java SDK here). The action automatically and intelligently increments the versions numbers in this PR with new commits to main by searching for annotations in XML/JSON/etc.

When these PRs are merged, the existing release process runs. Currently, we use this in the Java SDK, Go SDK, Node SDK, and all the contrib repos. To ease things for deveopers, we use github's "sqush and merge" setting to use the PR title as the commit message. We also have a github action that lints the commit message. This means that the only thing that needs to follow semantic commits is the PR title.

Benefits:

• breaking changes are identified, audited and recorded at PR time, not at release time, reducing burden on release manager.
• releases are democratic (they are PRs opened by a bot that must be merged)
• semantic versioning is done automatically
• CHANGELOG is built automatically

cc @benjiro

Requirements

The requirement IDs used in the specification changed when we added support for client-side feature flags. The .NET SDK uses those IDs in some tests. These should be reviewed to make sure they match the spec.

Requirements that are out of date:

• dotnet-sdk/test/OpenFeature.Tests/OpenFeatureClientTests.cs
  • Spec IDs: 1.4.5-1.4.14
• dotnet-sdk/test/OpenFeature.Tests/OpenFeatureHookTests.cs
  • Spec IDs: 4.3.3-4.3.8

Here's the diff between spec v0.5.2 and v0.8.0.

Observed behavior

Stumbled onto this while investigating failing unit tests add as part of #181 that fail when run in concert, but pass when run independently.

Working theory is that we're not restarting the consumer thread. Patch forthcoming.

See: #181

Expected Behavior

No response

Steps to reproduce

No response

I'm eventually planning to offer this upstream as part of a new OpenFeature.Extensions.Hosting package à la OpenTelemetry.Extensions.Hosting, but still working through the realistic use cases and general ergonomics, but just mentioning that so this doesn't seem entirely irrelevant to the project at hand.

This has been suggested before, and I think it's a good idea. I would welcome such a PR, or at least an issue proposing it.

I'll open a new issue to track this, and will see about getting a draft PR put together over the next couple of weeks.

Originally posted by @austindrenski in open-feature/dotnet-sdk-contrib#125 (comment)

Change in the documentation

Update specification badge to v0.7.0:

Add the hook behaviour described in the specification https://github.com/open-feature/spec/blob/main/specification/hooks.md

Requirements

As part of C# 8.0, we have access to a Nullable Reference Types (NRT) feature. This allows the have a more explicit flow of nulls in the codebase.

The NRT feature holds you to a higher standard on how you deal with nulls, and as such it issues new warnings on existing code. So that those warnings (however useful) don’t break you, the feature must be explicitly enabled in your code before it starts complaining. Once you do that on existing code, you have work to do to make that code null-safe and satisfy the compiler that you did.

Check: https://devblogs.microsoft.com/dotnet/embracing-nullable-reference-types/

Possible breaking changes

This might bring some potential breaking changes for the providers and customers that consume our library. I think this should be released with a breaking change alert.

Notes

To ease the migration, the new code created can use a preprocessor directive. See https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#nullable-context

Potential tasks

We need a targeting key in the evaluation context. It should be added to the builder, and should be immutable (like all the props in the current implementation).

The evaluation context also lacks the required 'targeting key` field

dotnet-sdk/test/OpenFeature.Tests/OpenFeatureEvaluationContextTests.cs

Line 46 in 2c237df

[Specification("3.1.1", "The `evaluation context` structure MUST define an optional `targeting key` field of type string, identifying the subject of the flag evaluation.")]

Originally posted by @roelofb in #204 (comment)

Due to the controversies around the Moq library (see https://github.com/moq/moq/issues/1372), we may want to consider switching to another mocking library, like e.g. NSubstitute to avoid any privacy issues.
[1] Contains an example of how this was done by another previous Moq user.

Note: Currently we are using v4.18 of Moq, and the controversial change has been introduced with v4.20 and above, so currently this repo is not directly affected by the change. However, we should not merge any renovate PRs bumping the Moq dependency

References:
[1] Afterlife-Guide/AppSettings.Merge#6

Requirements

Use https://github.com/xoofx/dotnet-releaser to manage to release dotnet-sdk

This tool will do the following

• Generate pretty changelogs
• Create github releases with the generated changelogs
• Publish to nuget

Things that will need to be considered

• Standardising tags on PRs (feature, enchantment, bug, etc)
• Update the documentation to reflect this
• Define a PR template
• Define rules around good commit messages, and try to perform some validation/linting around it

Change in the documentation

Update the readme to follow the new template. Once this is ready, the documentation can be configured to pull from the repo automatically.

Examples from other repos:

• https://github.com/open-feature/java-sdk
• https://github.com/open-feature/js-sdk/tree/main/packages/client

Update the SDK to be compliant with the v0.4.0 spec.

Providers

⚠ BREAKING CHANGES

• #129 - Remove EvaluationOptions from FeatureProvider method signatures - #52

General

• Add/update a spec version badge on the readme.

Full Changelog: open-feature/spec@v0.3.1...v0.4.0

Update readme to be consistent with the template here: open-feature/.github#36

DoD:

• README structure matches template
• Examples are up-to-date and added for all TODOs in template
• details only pertaining to developers of the SDK are moved to CONTRIBUTING.md

See similar Java PR: open-feature/java-sdk#382

Update the SDK to be compliant with the v0.2.0 spec.

Flag Evaluation API

• #116 - Have different methods for float and int - #31

Providers

• #119 - Remove context transformers. Add provider hooks - #38

Evaluation Context

• #117 - Ensure context merging behavior is correct - #32
• #120 - Add the ability to fetch all custom fields - #37
• #121 - Ensure evaluation context keys are unique - #32

Hooks

• #119 - Ensure provider hook are correctly ordered - #38

Full Changelog: open-feature/spec@v0.1.0...v0.2.0

We have SBOMs currently for Java and Go. We could use them here as well. I recommend this utility: https://github.com/marketplace/actions/cyclonedx-net-generate-sbom (we're using the clyclonedx format elsewhere and it's popular).

Definition of done:

• SBOMs generated and attached to release artifact in GH, or otherwise made publicly available (for every release)
• only includes dependencies of module in question (not of repo)

Observed behavior

Still trying to figure out all the triggering bits, but tldr; in a mixed-target solution (e.g. net48, net8.0, and netstandard2.0), OpenFeature is breaking source generators when loading the solution in IDEs (e.g. both Rider and Visual Studio), as well as when building directly from MSBuild.exe when certain MSBuild targets are used (seems to relate to the VS install and which task DLLs are loaded).

CSC : error CS8784:
  Generator 'SomeGenerator' failed to initialize. It will not contribute to the output and compilation errors may occur
  as a result. Exception was of type 'FileNotFoundException' with message 'Could not load file or assembly 'System.
  Collections.Immutable, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies.
  The system cannot find the file specified.'.

[C:\a\dotnet\SomeSolution\src\SomeProject\SomeProject.csproj::TargetFramework=netstandard2.0]

where SomeProject.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>
  </PropertyGroup>

  <ItemGroup>
    <ProjectReference Include="../SomeProject.Generators/SomeProject.Generators.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />
  </ItemGroup>

</Project>

and SomeProject.Generators.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.7.0" PrivateAssets="all" />
  </ItemGroup>

</Project>

and then SomeProject.csproj is eventually being used by SomeLegacyWebProject.csproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>library</OutputType>
    <TargetFramework>net48</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <!-- N.B. in reality this is a transitive reference coming from deep within the project graph -->
    <PackageReference Include="System.Collections.Immutable" Version="8.0.0" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="../SomeProject/SomeProject.csproj" />
  </ItemGroup>

  <Import Condition="Exists('$(VSToolsPath)/WebApplications/Microsoft.WebApplication.targets')" Project="$(VSToolsPath)/WebApplications/Microsoft.WebApplication.targets" />

</Project>

Now one look at SomeLegacyWebProject.csproj and the immediate reaction of any reasonable engineer is to blame the legacy of it all, but this project plays nice with all of the other modern components that we continue to shoehorn into it, including a whole slew of OpenTelemetry and OpenTelemetry.* packages, but just not with OpenFeature.

I hate opening issues without a minimal repro, so will keep trying to put one together and ping back here if/when I can, but hoping the nuances of reproducing my actual use case won't stymie making progress on resolving this, since I think it's fairly clear cut.

The primary issue is that OpenFeature flows this constraint onto consumers:

I would like to see the upper-bound of this constraint removed as was originally proposed in #136 since we're back here 6 months later as predicted by @toddbaert in #136 (comment).

See: #136, #137

Expected Behavior

I can reference OpenFeature from projects consuming (transitively or otherwise) version 8.0.0 or later of System.Collections.Immutable without breaking source generators.

Steps to reproduce

No response

• Code needs to be documented - #21
• Readme needs to be updated with more examples - #41
• Add readme to nuget package
• Add examples/samples

This issue lists Renovate updates and detected dependencies. Read the Dependency Dashboard docs to learn more.

Open

These updates have all been created already. Click a checkbox below to force a retry/rebase of any.

• chore(deps): update xunit-dotnet monorepo to v2.8.0 (xunit, xunit.runner.visualstudio)
• chore(deps): update google-github-actions/release-please-action action to v4

Ignored or Blocked

These are blocked by an existing closed PR and will not be recreated unless you click a checkbox below.

• chore(deps): update dependency benchmarkdotnet to v0.13.12
• chore(deps): update dotnet monorepo to v8 (major) (System.Collections.Immutable, System.Threading.Channels)

Detected dependencies

• Check this box to trigger a request for Renovate to run again on this repository

Update the SDK to be compliant with the v0.3.0 spec.

Providers

• #129 - Remove context transformation

Hooks

• #125 - Verify context merging order for before hooks

Full Changelog: open-feature/spec@v0.2.0...v0.3.0

I think we can default this to empty in the FlagEvaluationOptions constructor.

https://github.com/open-feature/spec/blob/1bbbb361952adfa4a94618ffe66e943719be9e50/specification/hooks.md#requirement-451

Requirements

See convo here: #38 (comment)

Requirements

Following open-feature/dotnet-sdk-contrib#132, the OpenFeature.Contrib.Hooks.Otel package now only depends on OpenFeature and OpenTelemetry.Api, the latter of which in turn has just a single dependency on System.Diagnostics.DiagnosticSource.

I'm proposing that we obsolete the OpenFeature.Contrib.Hooks.Otel package and incorporate its contained code into the main OpenFeature project to provide first-class telemetry support in line with other popular projects in the .NET ecosystem.

Is this a good idea?

From open-telemetry/opentelemetry-dotnet (emphasis added):

The inspiration of the OpenTelemetry project is to make every library observable out of the box by having them call OpenTelemetry API directly. However, many libraries will not have such integration, and as such there is a need for a separate library which would inject such calls, using mechanisms such as wrapping interfaces, subscribing to library-specific callbacks, or translating existing telemetry into OpenTelemetry model.

From open-telemetry/opentelemetry-dotnet (emphasis added):

Application developers and library authors use OpenTelemetry API to instrument their application/library. The API only surfaces necessary abstractions to instrument an application/library. It does not address concerns like how telemetry is exported to a specific telemetry backend, how to sample the telemetry, etc. The API consists of Tracing API, Logging API, Metrics API, Context and Propagation API, and a set of semantic conventions.

What if we think OpenTelemetry.Api is just too scary?

I personally have a lot of faith in the open-telemetry/opentelemetry-dotnet team, and thus place a lot of trust in their promise to maintain OpenTelemetry.Api as a lightweight abstractions package, but for good measure, I do want to call out that we could accomplish this proposed in-the-box'ing without taking a dependency on OpenTelemetry.Api.

To do this we would instead take a direct dependency on System.Diagnostics.DiagnosticSource, and then reimplement-the-wheel for ActivityExtensions.RecordException(...), ActivityExtensions.SetStatus(...), etc.

Again, I'm not endorsing this, but it's an option (e.g. among many other reasons, it would be exceptionally peevish if our impl drifts from the upstream impl and exceptions recorded by another library have different formats in Datadog/Dynatrace/etc).

See: open-feature/dotnet-sdk-contrib#132

Implement a comprehensive unit test suite that covers all of the specifications

https://github.com/open-feature/spec/blob/main/specification.json

Requirements

We want to be able to publish development builds to nuget so they are able to be distributed and tested by provider implementers before the major release.

Create a CI/CD pipeline for the repo

As brought up here, there's some potential issues with thread safety in the SDK, particularly around state maintained in the API singleton and clients. Let's use this issue to investigate and discuss some of these.

cc @kinyoklion

Leverage the test-harness and evaluation.feature gherkin suite to create a set of integration tests for .NET. See equivalent examples in Java and JS. Though we should call these "end-to-end" not integration tests, since that's what they are.

The suite should start the test harness container and run it in CI (again, see references in Java and JS).

Alpha Checklist:

• spec compliant
• contains test suite which verifies behavior consistent with spec
• contains test suite with reasonable coverage
• automated publishing
• comprehensive readme

Update the SDK to be compliant with the v0.5.0 spec.

What's Changed

⚠ BREAKING CHANGES

• open-feature/spec#142 Error codes must be a constrained set of values (e.g. enum). #72

Additional changes

• open-feature/spec#140 Clarify that reason must be a string in the flag resolution details and evaluation details.
• open-feature/spec#142 Add optional error message to flag resolution details. #72

General

• Add/update a spec version badge on the readme. #74

Full Changelog: open-feature/spec@v0.4.0...v0.5.0

Observed behavior

Hello. I been working on a custom feature flag solution for our company. It is custom tailed to fit our use cases and needs. I came across OpenFeature a few months back and found it well thought-out, so we adopted it as a standard. Kudos on doing this work.

We have implemented a custom provider that uses our internal feature flag solution. We have many software projects that need to evaluate feature flags and some of them do not support async/await. When I try and call GetBooleanDetails on IFeatureClient I call GetAwaiter().GetResult() at the end to force the code to support synchronous evaluation. This causes a deadlock. I have ensured that our custom provide calls ConfigureAwait(false) on all async calls. Looking at the source code, it appears FeatureClient.GetBooleanDetails awaits Tasks from inner methods, but ConfigureAwait(false) is not called.

My assumption is that because none of the awaited calls with FeatureClient have ConfigureAwait(false), it's causing the deadlock because the calling code tries to resume execution on the same thread (which fails in an MVC application).

Expected Behavior

Calling .GetAwaiter().GetResult() or .Result should not cause a deadlock, but force the code to execute synchronously.

Steps to reproduce

• Configure OpenFeature API with a provider that has async calls.
• Execute: client.GetBooleanDetails(flagKey, false, customContext).Result; within an ASP.Net MVC (4.8 framework) action that does not support async/await or return a Task<>
• Observe that the request never completes and a deadlock is created.