This PR:
* stabilizes evaluation API
* stabilizes providers
* hardens hooks, events and context
* hardens all "static context" (client-side) points
This is a pretty substantial step.
`stable` means that we have no more leeway at all in SDKs - **any
breaking changes to the parts of the SDK implementing stable APIs means
a new major version of the SDK is required, without exception**.
I've also opted to "harden" the `static context paradigm` since we have
numerous client implementations in the wild, in particular React and
Angular, but also Android. There could still be breaking changes here
but the bar is higher. I think this is how we're acting already with
regards to these APIs, in all honesty.
I did not promote the [context
propagation](https://openfeature.dev/specification/sections/evaluation-context#33-context-propagation)
sub-section, or the new tracking section - these are _relatively_ new,
though I'm open to opinions on hardening these further.
Potential blockers:
- https://github.com/open-feature/go-sdk/issues/389
- https://github.com/open-feature/spec/issues/270
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
## This PR
- clarify evaluation context in hook requirements
### Related Issues
Fixes#328
### Notes
I added the clarification in the non-normative section. Hopefully, this
addresses the ambiguity around what we mean by `evaluation context`.
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
This is a change that I suspect will have little practical impact for
most users, but helps disambiguate some behavior that recently caused
[issues](https://github.com/open-feature/go-sdk/issues/397) and
[confusion](https://github.com/open-feature/go-sdk/pull/400/files#r2191214490).
This is only one means of addressing this concern, so please don't
hesitate to put forward alternative proposals.
"Idempotent" may not be exactly the right word here, since it's only
idempotent within the scope of one execution of the life-cycle, so I'm
open to copy changes here as well.
# EDIT
⚠️ I've substantially changed this PR based on feedback, and
dismissed all approvals:
As @erka and others have pointed out, practically, `shutdown` has been
used for more than just cleaning up providers, it's also used frequently
to "reset" the API for testing purposes. I think this is a valid
use-case and I don't see any reason why it can't be added to what (at
least my understanding of) the original intent of the function was... so
I've changed 1.6.2 to say that the `shutdown` now also resets the state
of the API fully (removes hooks, providers, event handlers, etc) from
the API.
As @chrfwow noted (and I was concerned about as well) guaranteeing that
`shutdown` is not called while a provider is still starting up will be
tricky to implement. Instead I've changed 1.6.1 to say we will
unconditionally run shutdown on all providers, and also added a
recommendation that providers handle this gracefully in the provider
spec.
@sahidvelji I've also added a pre-amble about shutdown in general as you
requested.
@lukas-reining @erka @beeme1mr @sahidvelji @chrfwow please re-review.
---------
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com>
This clarifies some ambiguity in hook execution order, mentioned in
https://github.com/open-feature/spec/issues/164:
> The issue here is that the overall hook execution order would vary if
any of these hooks have more than one entry. For example:
>
> ```
> API = [A, B]
> Client = [C, D]
> Invocation = [E, F]
> Provider = [G, H]
>
> # `before` list
> before_hooks = API + Client + Invocation + Provider
>
> # spreading directly (like Python)
> list1 = Provider + Invocation + Client + API
> # results in [G, H, E, F, C, D, A, B]
>
> # reversing the before list (like JavaScript)
> list2 = before_hooks[:]
> list2.reverse()
> # results in [H, G, F, E, D, C, B, A]
> ```
This change makes clear that `[H, G, F, E, D, C, B, A]` is the correct
answer here, which is the more intuitive choice, in my opinion.
This is also one of the few (only?) normative spec points that isn't a
single sentence, but instead has point-form clauses, so I've also
attempted to fix that.
I've work-shopped this with a few people and they interpreted it
correctly, though admittedly, the term "stack-wise" is doing a lot of
work here. I'm hopeful that our audience is familiar enough with that
sort of term that the intent is clear.
I believe there's a few SDKs where this is incorrectly implemented, I'll
audit those and create issues if this is merged. I don't expect that
such an implementation change will significantly impact anyone.
---------
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->
## This PR
<!-- add the description of the PR here -->
Even though the README says that no explicit status implies experimental
status, I think we should explicitly mark the Hooks section experimental
to align with the other sections.
### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->
### Notes
<!-- any additional notes for this PR -->
### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->
### How to test
<!-- if applicable, add testing instructions under this section -->
Signed-off-by: Sahid Velji <sahidvelji@gmail.com>
## This PR
Loosens the value definition for hook data.
In the example within the spec, and within the implementation in the
dotnet-sdk, the intent was to allow any type of data. For example
creating an open telemetry span in before, storing it hook data, and
ending it in after.
### Notes
Related to: https://github.com/open-feature/dotnet-sdk/pull/387
---------
Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->
## This PR
<!-- add the description of the PR here -->
Add support for the hook-data concept for hooks. Hook-data allows for
per-evaluation data to be propagated between hooks.
This is especially useful for analytics purposes where you may want to
measure things that happen between stages, or you want to do something
like create a span in one stage and close it in another.
This concept is similar to the `series data` concept for LaunchDarkly
hooks.
https://github.com/launchdarkly/open-sdk-specs/tree/main/specs/HOOK-hooks#evaluationseriesdata
Unlike `series data` the data in this approach is mutable. This is
because the `before` stage already has a return value. We could
workaround this by specifying a return structure, but it maybe seems
more complex. The data is only passed to a specific hook instance, so
mutability is not of great concern.
Some functional languages may still need to use an immutable with return
values approach.
I can create an OFEP if we think this merits discussion prior to
proposal.
### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->
Related discussion in a PR comment.
https://github.com/open-feature/java-sdk/pull/1049#discussion_r1718895742
---------
Signed-off-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Just replaces some PNGs with mermaid.
This is nice because they can change color for the website, and they are
more rigorous and maintainable (we already use mermaid in the spec).
old:
new:
```mermaid
flowchart LR
A[Application] --> API(Evaluation API)
API --> P[Provider]
P --> FMS[(Flag Management System)]
```
```mermaid
flowchart LR
B(('Before' stage)) ---> FE[Flag Evaluation]
B -..->|Error| E
FE ---> A(('After' stage))
FE -..->|Error| E(('Error' stage))
A ---> F(('Finally' stage))
E -..-> F
```
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
As discussed in recent meetings, we've had complaints about this from
multiple sources. This PR adds a stipulation that no logging is done by
the SDK during flag evaluation. It also defines a very simple `logging
hook` concept as an included utility. These should be very simple to
write and will provide all the needed logging but give authors more
control than built in log statements. It will also be a very nice intro
to the hooks concept for users.
Here's the Java implementation:
https://github.com/open-feature/java-sdk/pull/1084
---------
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
## This PR
- adds hard new lines to GitHub notes
### Notes
GitHub does some magic behind the scenes that isn't part of the markdown
standard. This causes some issues on our doc site. Explicitly adding a
hard line break using double space helps us avoid this issue.
### Follow-up Tasks
Update the docs to include this change.
---------
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Adding some additional non-normative text to clarify a few things at the
prompt of @federicobond .
The meaning and justification should be self-evident, and if it's not I
did a bad job. 😅
---------
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
No substantial changes here, just editorial improvements and
corrections.
When implementing https://github.com/open-feature/spec/pull/241, I
noticed a few oversights (see comments).
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
* moves provider state into SDK (SDK now maintains provider
state/lifecycle; provider interface is now "stateless")
* refine semantics around client state when context is
pending/reconciled by using new events/states, instead of STALE
* add PROVIDER_FATAL error code
* add PROVIDER_RECONCILING event (client only)
* add RECONCILING provider status status (client only)
Resolves: https://github.com/open-feature/spec/issues/238
---------
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de>
Co-authored-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Fabrizio Demaria <fabrizio.f.demaria@gmail.com>
* loosen shutdown requirements to support things like go ctx
* clarify how to handle init/shutdown of providers bound to multiple names
* clarify that authors can await the READY event during init
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
**Marking Evaluation API and Providers `hardening`** and a few
non-functional changes here that improve structure.
specifically:
- explicitly mark and number all sections
- section headings have consistent numbering (provider section numbering
was different than the other docs)
- removed "draft" language
An alternative to marking all of `provider` and `evaluation API` as
hardening would be just marking all existing sections therein as
hardening, which might be better since we'd have to do that anyway if we
added a new experimental section.
see: https://github.com/open-feature/spec/issues/146 for more on release
goals.
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Todd Baert