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 14 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>
|
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:
📝 WalkthroughWalkthroughADR-0009 revises 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.
There was a problem hiding this comment.
implemented the change with the full hash as the default + cache key generator function to override.
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.
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
| - **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). | ||
| - **Answer:** Include all three (OFREP base URL, auth credential, and bound `domain`) in the default cache key, following the principle of maximum isolation out of the box: the default scopes as tightly as possible, and applications relax it where they do not need it. The auth credential is included by default; setups with rotating or short-lived credentials can drop it via the cache-key generator (Open Question #4) rather than weakening the default for everyone. | ||
| 4. Should the optional `cacheKeyPrefix` configuration option be removed entirely? | ||
| - **Answer:** Yes, removed. `cacheKeyPrefix` is replaced by a more general cache-key generator function. The generator receives the request inputs (OFREP base URL, auth credential, bound `domain`, `targetingKey`, and the full static context) and returns the key material the provider hashes into `cacheKeyHash`. This subsumes prefixing (prepend a namespace string) and additionally lets applications narrow the key (e.g. drop the auth credential for rotating tokens) or broaden it (e.g. add stable context fields). The provider always controls hashing; the generator only returns the value to hash. |
There was a problem hiding this comment.
This makes this amendment a breaking change for providers who have already implemented this. Are we comfortable with that?
There was a problem hiding this comment.
seeing that OFREP is still pre-1.0 I believe we are generally.
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
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)
327-335: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
cacheKeyPrefixanswer.Open Question
#2still describescacheKeyPrefixas supported, but the amendment and Open Question#4replace that option with the cache-key generator. Keeping both paths in the document makes the ADR self-contradictory.Proposed fix
- - **Answer:** Yes. Providers should support an optional `cacheKeyPrefix` configuration option. When provided, the cache key becomes `hash(cacheKeyPrefix + ":" + targetingKey)` instead of `hash(targetingKey)`. The prefix value is left to the application author (e.g., the OFREP base URL, a project or auth token, or any other distinguishing string). The default (no prefix) keeps the single-provider case simple. See the `cacheKeyPrefix` section in the Decision above. + - **Answer:** No. Collision avoidance is handled by the default key inputs, and custom namespacing now uses the cache-key generator instead of `cacheKeyPrefix`.🤖 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 327 - 335, The Open Question `#2` answer still refers to the removed cacheKeyPrefix option, which conflicts with the later amendment and Open Question `#4`. Update the answer in this ADR section to remove the stale cacheKeyPrefix guidance and align it with the cache-key generator approach, using the surrounding cacheKeyPrefix/cache-key generator discussion to keep the document consistent.
🤖 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 57-63: The CacheKeyGenerator contract currently marks targetingKey
as optional, which weakens the cache-key guarantees and should match the OpenAPI
requirement. Update the type definition in the local storage ADR so targetingKey
is required in the input object for CacheKeyGenerator, and make sure any related
references to the generator contract or EvaluationContext-based cache key logic
align with that required field.
---
Outside diff comments:
In `@service/adrs/0009-local-storage-for-static-context-providers.md`:
- Around line 327-335: The Open Question `#2` answer still refers to the removed
cacheKeyPrefix option, which conflicts with the later amendment and Open
Question `#4`. Update the answer in this ADR section to remove the stale
cacheKeyPrefix guidance and align it with the cache-key generator approach,
using the surrounding cacheKeyPrefix/cache-key generator discussion to keep the
document consistent.
🪄 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: 3b8862d6-1d46-48b9-bffd-e31fad2457a1
📒 Files selected for processing (1)
service/adrs/0009-local-storage-for-static-context-providers.md
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.