feat: supply the bound domain to provider initialization#393
feat: supply the bound domain to provider initialization#393jonathannorris wants to merge 10 commits into
Conversation
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe specification is extended to add domain parameter passing to provider initialization, define domain-scoped provider semantics, and constrain domain-scoped providers to single-domain binding. The provider ChangesDomain scoping for provider initialization and binding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
Thank you :) this looks good to me |
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
|
||
| ```java | ||
| The `domain` the provider is registered under is also supplied, allowing the provider to scope domain-specific behavior, such as partitioning a persistent cache, so that multiple providers sharing the same storage do not collide. | ||
| A `provider` instance is initialized only once, even when bound to multiple `domains`; in that case the `domain` supplied is the one under which it was first registered. |
There was a problem hiding this comment.
I am a bit worried about this being confusing to new provider authors but I think it is worth it.
There was a problem hiding this comment.
We could potentially have an optional domain updated function on the provider if we think its necessary.
There was a problem hiding this comment.
I think this is mostly documentation. Most providers can probably ignore it - but if you need to maintain some cached state, it's important use to use this. I think that's the simple doc line we can add - this is a param for provider-maintained namepacing.
There was a problem hiding this comment.
@toddbaert agreed, I think we keep this as a MAY rather than required; it's a documentation/namespacing param most providers can safely ignore. And the single-domain binding for domain-scoped providers (1.1.8.1) removes the need for a separate domain-setter I floated earlier: the domain is fixed at init for the providers that actually care, so we avoid the races a separate setter would introduce.
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
| #### Requirement 2.4.1 | ||
|
|
||
| > The `provider` **MAY** define an initialization function which accepts the global `evaluation context` as an argument and performs initialization logic relevant to the provider. | ||
| > The `provider` **MAY** define an initialization function which accepts the global `evaluation context` and the bound `domain` (if any) as arguments and performs initialization logic relevant to the provider. |
There was a problem hiding this comment.
is it optional to supply either, or if you supply one you must supply the other? it's not clear from the wording
There was a problem hiding this comment.
@toddbaert @JamieSinn I think we just keep 2.4.1 language-agnostic and word it so the domain is clearly optional/additive, leaving each SDK to introduce it backwards-compatibly in its own idiom (optional arg, overload, separate interface, whatever fits).
The MAY already allows this; I'd just tweak the wording so it's obvious the domain is optional to consume and existing providers aren't forced into a breaking change. Worth noting this will still be a breaking change in Go (its Init(EvaluationContext) error is a required interface method with no overloads) and likely a few other languages, where a separate optional interface with a fallback would be needed instead.
| The initialization function is an ideal place for such logic. | ||
|
|
||
| The `domain` the provider is registered under is also supplied, allowing the provider to scope domain-specific behavior, such as partitioning a persistent cache, so that multiple providers sharing the same storage do not collide. | ||
| A `provider` instance is initialized only once, even when bound to multiple `domains`; in that case the `domain` supplied is the one under which it was first registered. |
There was a problem hiding this comment.
How does this change, if by any chance somebody reassigns the first domain to a different provider? What is the expectation, should this change the behaviour? Theoretically, there could be a new provider with a different backend and still the same cache keys.
There was a problem hiding this comment.
How does this change, if by any chance somebody reassigns the first domain to a different provider? What is the expectation, should this change the behaviour? Theoretically, there could be a new provider with a different backend and still the same cache keys.
Ya, that's a good point.
Similarly, you can, hypothetically, use the same provider instance in 2 domains. That also brings up a similar question (which domain assignment "wins"). I don't have great answers to these at the moment.
That said, I since domains are the fundamental mechanism of isolation, I think we need to supply this information to providers so they can properly scope their caches, etc.
An obvious alternative is to provide a domain setter, but if that's separated from the init, I think it brings up a bunch of possible races we don't want to contend with.
There was a problem hiding this comment.
@aepfli @toddbaert I think both of these are covered now:
- Reassigning a domain to a different provider with a different backend: the OFREP cache key in the follow-up ADR (docs: amend ADR 0009 to tie cache key to OFREP resource (domain, url, auth) protocol#80) includes the OFREP URL and auth, so a different backend produces a different key even on the same
domain. The new provider won't read the old one's cached entries. - Same instance bound to two domains ("which wins"): with the
domain-scopeddeclaration (2.4.3) and 1.1.8.1, a provider that maintains per-domain state can only be bound to a singledomain, and the API rejects a second binding. So there's no ambiguity for the providers that actually care. Stateless providers can still share across domains since they don't key anything on it.
…g enforcement Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
Pushed an update adding a The point that a provider isn't 1:1 with a domain is fair, so rather than relying on providers to use the bound domain correctly, this makes it an enforced contract:
Stateless providers are unaffected and can still back multiple domains; only opt-in providers are constrained. This also resolves the open question about a shared instance only seeing its first domain. OFREP cache key construction lives in the follow-up ADR: open-feature/protocol#80. |
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…ization Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
Added I kept it as a provider-side |
Amends provider initialization so the
domaina provider is bound to is supplied to itsinitializefunction, letting providers scope domain-specific behavior (such as partitioning a persistent cache) to that domain.Motivation
Provider-level persistent caches (e.g. the OFREP web provider's local persistence) currently have no way to know which
domainthey're bound to, so two providers on the same storage partition can collide on cache entries even though OpenFeature's domain model is supposed to keep them isolated. See slack thread, open-feature/js-sdk-contrib#1566, and ADR 0009.A follow-up OFREP ADR will add the domain to the cache key.
Changes
Two parts, both kept language-agnostic so implementations can use overloads, optional arguments, etc.:
Supply the bound domain to init (folded into the existing init requirements, per earlier review feedback):
2.4.1(provider): theinitializefunction accepts the globalevaluation contextand the bounddomain(if any).1.1.2.2(API): theprovider mutatorMUST supply the bounddomain(if any) when invokinginitialize.Let a provider declare it's domain-scoped, enforced by the API (new requirements):
2.4.3(provider): aproviderMAY declare itselfdomain-scopedwhen it holds per-domainstate such as a persistent cache.1.1.8(condition) /1.1.8.1(API): when a provider isdomain-scoped, theprovider mutatorMUST NOT bind it to more than onedomain, and rejects any attempt to do so.Resolved
The earlier open question (a provider bound to multiple
domainsis initialized once and receives only its first binding'sdomain) is now addressed for stateful providers: declaringdomain-scopedrestricts the instance to a singledomain, so thedomainit keys on is unambiguous. Stateless providers are unaffected and can still back multipledomains.