docs: amend ADR 0009 to tie cache key to OFREP resource (domain, url, auth)#80
docs: amend ADR 0009 to tie cache key to OFREP resource (domain, url, auth)#80jonathannorris wants to merge 12 commits into
Conversation
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>
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughADR-0009 updates the persisted cache key for OFREP static-context providers to include OFREP base URL, auth credential, bound ChangesADR-0009 Cache Key Strategy Amendment
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
jonathannorris
requested review from
JamieSinn,
MattIPv4,
lukas-reining,
thomaspoignant and
toddbaert
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/adrs/0009-local-storage-for-static-context-providers.md (1)
309-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the
cacheKeyPrefixwording everywhere.This amendment says the prefix is only a supplemental namespace, but the earlier answer still describes it as wrapping
targetingKeyalone. Please update that section too, or the ADR will present two incompatible formulas.Proposed fix
- When provided, the cache key becomes `hash(cacheKeyPrefix + ":" + targetingKey)` instead of `hash(targetingKey)`. + When provided, the cache key becomes `hash(cacheKeyPrefix + ":" + url + ":" + auth + ":" + domain + ":" + targetingKey)` instead of `hash(url + ":" + auth + ":" + domain + ":" + targetingKey)`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/adrs/0009-local-storage-for-static-context-providers.md` around lines 309 - 316, The ADR contains inconsistent descriptions of cacheKeyPrefix between sections. The new amendment (2026-06-19) describes cacheKeyPrefix as a supplemental namespace for storage partition separation, but an earlier section still presents it as wrapping targetingKey alone. Locate the earlier section that describes the cacheKeyPrefix configuration and update its wording to clarify that cacheKeyPrefix is now only a supplement for namespacing across storage partitions the application controls directly, not the primary mechanism for preventing collisions. The cache key collision avoidance now relies on hashing the OFREP URL, auth credential, domain, and targetingKey together by default, with cacheKeyPrefix serving as an optional escape hatch only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/adrs/0009-local-storage-for-static-context-providers.md`:
- Around line 39-49: The cache key hash construction using simple
colon-separated concatenation of raw fields is ambiguous and collision-prone
because URLs, credentials, domains, and other fields can contain colon
characters, causing different field combinations to potentially hash to the same
value. Update the hash input construction to use an unambiguous serialization
approach such as length-prefixed encoding for each field, a serialization format
like JSON, or delimiters that encode field boundaries explicitly to ensure that
the hash inputs for the OFREP base URL, auth credential, domain, targetingKey,
and optional cacheKeyPrefix remain distinguishable regardless of their content.
---
Outside diff comments:
In `@service/adrs/0009-local-storage-for-static-context-providers.md`:
- Around line 309-316: The ADR contains inconsistent descriptions of
cacheKeyPrefix between sections. The new amendment (2026-06-19) describes
cacheKeyPrefix as a supplemental namespace for storage partition separation, but
an earlier section still presents it as wrapping targetingKey alone. Locate the
earlier section that describes the cacheKeyPrefix configuration and update its
wording to clarify that cacheKeyPrefix is now only a supplement for namespacing
across storage partitions the application controls directly, not the primary
mechanism for preventing collisions. The cache key collision avoidance now
relies on hashing the OFREP URL, auth credential, domain, and targetingKey
together by default, with cacheKeyPrefix serving as an optional escape hatch
only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fca43c2e-4ef6-4694-a3e4-947f09fb839e
📒 Files selected for processing (1)
service/adrs/0009-local-storage-for-static-context-providers.md
…che key Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/adrs/0009-local-storage-for-static-context-providers.md (1)
291-295:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the rebinding contract.
domain-scopedsays one instance can only bind to one domain, but the next bullet still describes what to do when the provider is rebound to a different domain. Please make one of those behaviors explicit so implementers don't end up with conflicting cache-invalidation rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/adrs/0009-local-storage-for-static-context-providers.md` around lines 291 - 295, There is a contradiction in the provider requirements: the first bullet states that persisting providers should be domain-scoped so each instance binds to at most one domain, but the last bullet describes behavior for when the provider is rebound to a different domain. Clarify the rebinding contract by either removing the domain-rebinding requirement from the last bullet if domain-scoped truly means no rebinding is possible, or revise the domain-scoped explanation to explicitly allow rebinding with appropriate cache invalidation. Choose one clear behavior pattern and ensure all bullets align with it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@service/adrs/0009-local-storage-for-static-context-providers.md`:
- Around line 291-295: There is a contradiction in the provider requirements:
the first bullet states that persisting providers should be domain-scoped so
each instance binds to at most one domain, but the last bullet describes
behavior for when the provider is rebound to a different domain. Clarify the
rebinding contract by either removing the domain-rebinding requirement from the
last bullet if domain-scoped truly means no rebinding is possible, or revise the
domain-scoped explanation to explicitly allow rebinding with appropriate cache
invalidation. Choose one clear behavior pattern and ensure all bullets align
with it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e976145c-67ce-4a5d-8b34-25aa8d523cc8
📒 Files selected for processing (1)
service/adrs/0009-local-storage-for-static-context-providers.md
MattIPv4
left a comment
MattIPv4
left a comment
There was a problem hiding this comment.
LGTM, but is there interest in discussing the transformer approach I proposed in open-feature/js-sdk-contrib#1566 -- that seems even more versatile than this proposed update, while still solving for the concerns that originally landed on just having targeting key to avoid volatility?
|
@MattIPv4 we can look at that separately to this, would likely need an ADR change as well. I think this change to include the |
|
👍 That's fair, can be looked into after. I can't think of an immediate use case (domain solves our issue), but it seems like a nice bit of flexibility to ensure folks have full control of the caching rather than the caching making assumptions (which is what caused this change to be needed). |
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
askpt
left a comment
askpt
left a comment
There was a problem hiding this comment.
Minor comment. The rest looks good.
| - **Amendment:** Collision avoidance no longer depends on an application-supplied prefix. The cache key now ties to the OFREP resource and identity by default (`hash(url + ":" + auth + ":" + domain + ":" + targetingKey)`), so providers pointing at different servers, using different credentials, or bound to different domains on the same storage partition do not collide without any application configuration. The bound `domain` relies on [open-feature/spec#393](https://github.com/open-feature/spec/pull/393) supplying it to the provider's `initialize` function. The optional `cacheKeyPrefix` above remains as a supplement for namespacing across storage partitions an application controls directly. See Open Question #3 for the resource-binding rationale. | ||
| 3. Should the cache key also be tied to the OFREP resource the evaluation was fetched from, rather than relying on the application to supply a distinguishing `cacheKeyPrefix`? | ||
| - **OFREP URL** Folding the OFREP base URL into the cache key (e.g. `hash(url + ":" + domain + ":" + targetingKey)`) ties cached results directly to the resource that produced them, so a provider reconfigured to point at a different server does not serve another server's cached evaluations, and same-origin instances pointing at different servers separate automatically without an explicitly configured prefix. The base URL is stable across restarts, so it does not reintroduce the volatile-input problem. This mirrors vendor SDKs that key their persisted cache by SDK key or environment (Statsig, Eppo, ConfigCat). The cost is that changing the configured URL silently invalidates the cache, which is usually the desired behavior. | ||
| - **Auth header:** Including the auth credential would tie the cache even more tightly to the protected resource, but credentials are not always stable. OFREP supports rotating/short-lived tokens via `headersFactory`, and a rotating bearer token would change the hash on every rotation. This is the same reason the original ADR dropped `authToken` from the cache key (see the [protocol#64](https://github.com/open-feature/protocol/pull/64) discussion). |
There was a problem hiding this comment.
This open question is very provider-dependent. I would prefer to get it removed from the key. If a provider has a refresh of 10 minutes, that would mean the cache is invalidated quite often.
Wouldn't make more sense to tie to the ETag instead of auth header?
There was a problem hiding this comment.
agreed the auth credential isn't always stable, I called that out in the open question. on ETag: it can be used differently depending on the provider, but generally it's a caching/version identifier rather than a resource identifier, so I'm not sure it fits as a key component. you'd also want the local cache to survive across ETag versions, keying on it would mean every config change misses the existing entry and defeats the persistence. it also can't be computed offline on a cold start to find the right entry, and we already persist it alongside the entry as a staleness signal.
the real question is what makes the better default. including auth almost guarantees isolation: two different credentials (projects, environments, keys) against the same url + domain can't read each other's cache without the app configuring anything. dropping it makes the default looser, that isolation is no longer guaranteed unless the app sets a cacheKeyPrefix. the cost of including it is that a short-lived credential (a JWT rotating every few minutes) fragments and effectively invalidates the cache on every rotation.
so it comes down to how common very low refresh times are for OFREP static-context providers in practice. if rotating-per-request tokens are the exception, the safer default seems worth it. a middle ground is keying on a stable credential identifier rather than the raw token, or making the auth component opt-in. curious where you both land.
There was a problem hiding this comment.
I think perhaps a lambda for a key generation might still be the best method, but by default I would do "maximum scoping" and allow people to narrow it from there. The worst thing that can happen is caches trampling each-other as we saw happen with @MattIPv4 ... between that and performance loss, I would default to maximum isolation and allow users (and wrapping providers) to relax the caching to their needs.
WRT Etag: I think it's a poor cache key ingredient. It's a cache validator, not a key. A cache key should be "assemblable" before any response is received - to put it another way, it should be based on PARAMs to a request, not RESULTS of a request, or key caching use-cases become impossible.
There was a problem hiding this comment.
I think perhaps a lambda for a key generation might still be the best method, but by default I would do "maximum scoping" and allow people to narrow it from there
What do you mean by "maximum scoping"?
I had a lambda approach in open-feature/js-sdk-contrib#1566, where by default the entire context is used. This guarantees maximum safety out of the box, so no trampling by default, and folks have to opt in to having a less specific key that introduces risk.
There was a problem hiding this comment.
What do you mean by "maximum scoping"?
I mean that the out-of-the-box defaults should add as many factors the to cache key as possible, and if they are un-needed or un-wanted in some cases, people can relax them; that's "fail safe but slow" in worse case scenarios.
There was a problem hiding this comment.
Ah, perfect -- that's the same way I've been thinking about this.
There was a problem hiding this comment.
sounds good, will update this PR to reflect that.
lukas-reining
left a comment
lukas-reining
left a comment
There was a problem hiding this comment.
Looks good to me. Only the auth token is still open to me.
| Static-context evaluations on the server can depend on context properties beyond `targetingKey`, so cached values may not reflect the current full context. | ||
| However, hashing the full context is impractical for local-cache-first startup because many implementations set volatile context properties on initialization (e.g. `lastSessionTime`, `lastSeen`, `sessionId`) that would change the hash on every app restart, defeating the purpose of persistence. | ||
| The accepted tradeoff is that the cache is keyed by stable user identity: a change in `targetingKey` (user switch, logout) invalidates the cache, but changes to other context properties do not. | ||
| The accepted tradeoff is that the cache is keyed by stable inputs (the OFREP base URL, the auth credential, the bound `domain`, and the `targetingKey`): a change in `targetingKey` (user switch, logout), in the bound `domain`, or in the resource the provider points at invalidates the cache, but changes to other context properties do not. |
There was a problem hiding this comment.
My largest concern is that the auth credential might not be "stable" in that sense e.g. with a JWT of 5 minutes expiry.
This might be okay though.
What I think could become complicated is when an app starts it might try to fetch a new token first, then fail do the evaluation and be stuck with a new credential but wanting to use the cache.
There was a problem hiding this comment.
replied on André's thread with the same theme: the tradeoff is a safer default (auth in the key guarantees cache isolation between credentials) vs the fragmentation risk you're describing with short-lived tokens. your app-startup scenario (fetch new token, evaluation fails, stuck wanting the old cache) is the sharp edge of that. let's converge there.
|
|
||
| - "Local storage" means a local persistent key-value store appropriate for the runtime, such as browser `localStorage` on the web or an equivalent mobile storage mechanism | ||
| - Providers should version their persisted format so future schema changes can be handled safely | ||
| - Persisting providers should declare themselves `domain-scoped` (per [open-feature/spec#393](https://github.com/open-feature/spec/pull/393)) so the API binds each instance to at most one `domain`. This keeps the `domain` component of the cache key unambiguous and avoids a single shared instance writing entries for more than one `domain` |
There was a problem hiding this comment.
Does this include the default domain? So it means it should implement the optional parameter?
There was a problem hiding this comment.
the default provider has no bound domain, so initialize is called without one and the domain component of the key is just empty, which is a valid single scope. a domain-scoped provider isn't required to have a domain, but per spec#393 (req 2.4.4) it must accept the domain at init, so yes, to actually scope its cache it needs to implement that optional parameter and use it.
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…into worktree-adr9-domain-cache-key
7 participants
Amends ADR 0009 (static-context local persistence) to tie the persisted cache key to the OFREP resource the evaluation was fetched from, by including the provider's bound
domain, the OFREP base URL, and the auth credential, in addition to thetargetingKey.Motivation
The accepted ADR keys the persisted cache on
targetingKey(optionally prefixed with an application-suppliedcacheKeyPrefix), leaving collision avoidance up to the application. A persisted evaluation is really a function of the resource it came from (server + credential + bound domain) and the identity it was requested for, so the cache key should reflect that automatically.Surfaced in open-feature/js-sdk-contrib#1566, where the OFREP web provider's local persistence has no way to know which
domainit's bound to.Changes
hash(url + ":" + auth + ":" + domain + ":" + targetingKey)(with optionalcacheKeyPrefixstill prepended).cacheKeyPrefixis demoted to a supplement for app-controlled namespacing.domainrelies on open-feature/spec#393, which supplies thedomaintoinitializeand adds an opt-indomain-scopeddeclaration the API enforces, so a persisting OFREP provider is bound to a singledomainand thedomaincomponent of the key is unambiguous.Open for discussion
Two points flagged as open questions in the ADR:
headersFactory, and a token that rotates on every request would defeat persistence. But auth doesn't change on every request, so for the common case of a stable credential it still provides useful separation between projects/environments/keys against the same URL. The original ADR droppedauthTokenfor the rotation reason (see protocol#64); reviewers may prefer a stable credential identifier or making the auth component opt-in.cacheKeyPrefix(Open Question chore: Configure Renovate #4): now that the URL, auth, anddomainare in the key by default, the collisionscacheKeyPrefixwas added to solve are handled automatically. I lean toward removing it to simplify the config surface, but open to keeping it as an explicit escape hatch.Marked as a draft for discussion.