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

feat!: internally maintain provider status #276

Merged
merged 10 commits into from
Jul 3, 2024

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jun 12, 2024

This PR implements a few things from spec 0.8.0:

  • implements internal provider status (already implemented in JS)
    • the provider no longer updates its status to READY/ERROR, etc after init (the SDK does this automatically)
    • the provider's state is updated according to the last event it fired
  • adds PROVIDER_FATAL error and code
  • adds "short circuit" feature when evaluations are skipped if provider is NOT_READY or FATAL
  • removes some deprecations that were making the work harder since we already have pending breaking changes.

Fixes: #250

@kinyoklion
Copy link
Member

Maybe more related to the spec than to this specific implementation. But I did find it somewhat hard to handle the runtime status after initialization status when the SDK automatically implements ready.

I think it makes it easy for simple providers, and somewhat cumbersome for more complex providers. I would actually prefer to be able to opt out.

Basically an underlying SDK may already emit these events, but we have to connect and translate these events conditionally around initialization. So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

Example of attempting to reconcile this in a python provider: https://github.com/launchdarkly/openfeature-python-server/blob/243a8dc515bcd16fe8c7d5a0913fbdfe6e073aa0/ld_openfeature/provider.py#L46

@toddbaert
Copy link
Member Author

toddbaert commented Jun 12, 2024

So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

I can see the issue here. We have some similar logic in flagd since it also supports automatic reconnection. I think for the majority of providers though, such events around reconnection etc, are not supported; it does indeed seem to be something featured in the most rigorous implementations.

I'm open to some kind of opt-out for emitting events on READY, but I'm not sure if the ROI is there. 🤔 I suppose it would be fairly easy to implement. 🤔

@kinyoklion
Copy link
Member

So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

I can see the issue here. We have some similar logic in flagd since it also supports automatic reconnection. I think for the majority of providers though, such events around reconnection etc, are not supported; it does indeed seem to be something featured in the most rigorous implementations.

I'm open to some kind of opt-out for emitting events on READY, but I'm not sure if the ROI is there. 🤔 I suppose it would be fairly easy to implement. 🤔

I think an opt-out would work.

I've not thought about it extensively, but another option may be to have automatic events be part of a base class that more complex providers could choose not to use instead of it being in the API. Though that path isn't the most sustainable unless you have a language that supports more "mixin" type classes. So maybe not.

Maybe also we can leave it as is and think if the situation can be improved. Overall I feel like getting initialization correct has become more complex, so maybe the abstraction needs adjusted some.

@toddbaert
Copy link
Member Author

So after initialization if a provider fails we want to emit that event, but if during initialization it fails we need to not emit that event from the provider because the SDK also will emit it.

I can see the issue here. We have some similar logic in flagd since it also supports automatic reconnection. I think for the majority of providers though, such events around reconnection etc, are not supported; it does indeed seem to be something featured in the most rigorous implementations.
I'm open to some kind of opt-out for emitting events on READY, but I'm not sure if the ROI is there. 🤔 I suppose it would be fairly easy to implement. 🤔

I think an opt-out would work.

I've not thought about it extensively, but another option may be to have automatic events be part of a base class that more complex providers could choose not to use instead of it being in the API. Though that path isn't the most sustainable unless you have a language that supports more "mixin" type classes. So maybe not.

Maybe also we can leave it as is and think if the situation can be improved. Overall I feel like getting initialization correct has become more complex, so maybe the abstraction needs adjusted some.

I thought a bit about an opt-out. I think we could add some fields/metadata to the provider to signal this and possibly other behaviors. Perhaps we could open an new issue to discuss this in the spec.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert
Copy link
Member Author

I had to rebase this and resolve conflicts on the recently merged concellationToken/Async chnage.

@toddbaert toddbaert force-pushed the feat/provider-status-internal branch from 626ee10 to 1ddcf33 Compare June 17, 2024 18:22
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

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

I added a few comments and concerns about the ValueTask.

src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
src/OpenFeature/Model/ProviderEvents.cs Outdated Show resolved Hide resolved
src/OpenFeature/ProviderRepository.cs Outdated Show resolved Hide resolved
toddbaert and others added 2 commits June 28, 2024 09:31
Co-authored-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert and others added 2 commits June 28, 2024 09:31
Co-authored-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
src/OpenFeature/Api.cs Outdated Show resolved Hide resolved
Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

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

LGTM. I don't have any more concerns about this PR. Thanks @toddbaert

toddbaert and others added 2 commits July 2, 2024 08:51
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert merged commit 63faa84 into main Jul 3, 2024
11 checks passed
@toddbaert toddbaert deleted the feat/provider-status-internal branch July 3, 2024 19:47
toddbaert added a commit that referenced this pull request Aug 21, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.0.0](v1.5.0...v2.0.0)
(2024-08-21)

Today we're announcing the release of the OpenFeature SDK for .NET,
v2.0! This release contains several ergonomic improvements to the SDK,
which .NET developers will appreciate. It also includes some performance
optimizations brought to you by the latest .NET primitives.

For details and migration tips, check out:
https://openfeature.dev/blog/dotnet-sdk-v2

### ⚠ BREAKING CHANGES

* domain instead of client name
([#294](#294))
* internally maintain provider status
([#276](#276))
* add CancellationTokens, ValueTasks hooks
([#268](#268))
* Use same type for flag metadata and event metadata
([#241](#241))
* Enable nullable reference types
([#253](#253))

### 🐛 Bug Fixes

* Add missing error message when an error occurred
([#256](#256))
([949d53c](949d53c))
* Should map metadata when converting from ResolutionDetails to
FlagEvaluationDetails
([#282](#282))
([2f8bd21](2f8bd21))


### ✨ New Features

* add CancellationTokens, ValueTasks hooks
([#268](#268))
([33154d2](33154d2))
* back targetingKey with internal map
([#287](#287))
([ccc2f7f](ccc2f7f))
* domain instead of client name
([#294](#294))
([4c0592e](4c0592e))
* Drop net7 TFM
([#284](#284))
([2dbe1f4](2dbe1f4))
* internally maintain provider status
([#276](#276))
([63faa84](63faa84))
* Use same type for flag metadata and event metadata
([#241](#241))
([ac7d7de](ac7d7de))


### 🧹 Chore

* cleanup code
([#277](#277))
([44cf586](44cf586))
* **deps:** Project file cleanup and remove unnecessary dependencies
([#251](#251))
([79def47](79def47))
* **deps:** update actions/upload-artifact action to v4.3.3
([#263](#263))
([7718649](7718649))
* **deps:** update actions/upload-artifact action to v4.3.4
([#278](#278))
([15189f1](15189f1))
* **deps:** update actions/upload-artifact action to v4.3.5
([#291](#291))
([00e99d6](00e99d6))
* **deps:** update codecov/codecov-action action to v4
([#227](#227))
([11a0333](11a0333))
* **deps:** update codecov/codecov-action action to v4.3.1
([#267](#267))
([ff9df59](ff9df59))
* **deps:** update codecov/codecov-action action to v4.5.0
([#272](#272))
([281295d](281295d))
* **deps:** update dependency benchmarkdotnet to v0.14.0
([#293](#293))
([aec222f](aec222f))
* **deps:** update dependency coverlet.collector to v6.0.2
([#247](#247))
([ab34c16](ab34c16))
* **deps:** update dependency coverlet.msbuild to v6.0.2
([#239](#239))
([e654222](e654222))
* **deps:** update dependency dotnet-sdk to v8.0.204
([#261](#261))
([8f82645](8f82645))
* **deps:** update dependency dotnet-sdk to v8.0.301
([#271](#271))
([acd0385](acd0385))
* **deps:** update dependency dotnet-sdk to v8.0.303
([#275](#275))
([871dcac](871dcac))
* **deps:** update dependency dotnet-sdk to v8.0.400
([#295](#295))
([bb4f352](bb4f352))
* **deps:** update dependency githubactionstestlogger to v2.4.1
([#274](#274))
([46c2b15](46c2b15))
* **deps:** update dependency microsoft.net.test.sdk to v17.10.0
([#273](#273))
([581ff81](581ff81))
* **deps:** update dotnet monorepo
([#218](#218))
([bc8301d](bc8301d))
* **deps:** update xunit-dotnet monorepo
([#262](#262))
([43f14cc](43f14cc))
* **deps:** update xunit-dotnet monorepo
([#279](#279))
([fb1cc66](fb1cc66))
* **deps:** update xunit-dotnet monorepo to v2.8.1
([#266](#266))
([a7b6d85](a7b6d85))
* Enable nullable reference types
([#253](#253))
([5a5312c](5a5312c))
* in-memory UpdateFlags to UpdateFlagsAsync
([#298](#298))
([390205a](390205a))
* prompt 2.0
([9b9c3fd](9b9c3fd))
* Support for determining spec support for the repo
([#270](#270))
([67a1a0a](67a1a0a))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
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.

[FEATURE] Make provider interface "stateless", SDK maintains provider state
5 participants