Skip to content

docs: amend ADR 0009 to tie cache key to OFREP resource (domain, url, auth)#80

Open
jonathannorris wants to merge 8 commits into
mainfrom
worktree-adr9-domain-cache-key
Open

docs: amend ADR 0009 to tie cache key to OFREP resource (domain, url, auth)#80
jonathannorris wants to merge 8 commits into
mainfrom
worktree-adr9-domain-cache-key

Conversation

@jonathannorris

@jonathannorris jonathannorris commented Jun 19, 2026

Copy link
Copy Markdown
Member

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 the targetingKey.

Motivation

The accepted ADR keys the persisted cache on targetingKey (optionally prefixed with an application-supplied cacheKeyPrefix), 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 domain it's bound to.

Changes

  • Cache key becomes hash(url + ":" + auth + ":" + domain + ":" + targetingKey) (with optional cacheKeyPrefix still prepended). cacheKeyPrefix is demoted to a supplement for app-controlled namespacing.
  • Updated the Decision, cache-matching, example payload, and Open Questions feat: OpenAPI proposal for V1 OFREP #2/chore: add pr lint workflow #3/chore: Configure Renovate #4 to match.
  • The bound domain relies on open-feature/spec#393, which supplies the domain to initialize and adds an opt-in domain-scoped declaration the API enforces, so a persisting OFREP provider is bound to a single domain and the domain component of the key is unambiguous.

Open for discussion

Two points flagged as open questions in the ADR:

  • Including the auth credential (Open Question chore: add pr lint workflow #3) is the piece most open to discussion. OFREP supports rotating/short-lived tokens via 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 dropped authToken for the rotation reason (see protocol#64); reviewers may prefer a stable credential identifier or making the auth component opt-in.
  • Removing cacheKeyPrefix (Open Question chore: Configure Renovate #4): now that the URL, auth, and domain are in the key by default, the collisions cacheKeyPrefix was 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.

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>
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ADR-0009 is amended to change how cacheKeyHash is computed for OFREP static-context provider local storage. The key formula changes from hash(targetingKey) (optionally prefixed) to hash(url + ":" + auth + ":" + domain + ":" + targetingKey). Cache matching, implementation notes, and open questions are updated accordingly.

Changes

ADR-0009 Cache Key Strategy Amendment

Layer / File(s) Summary
Amendment header and Decision text: new cache key formula
service/adrs/0009-local-storage-for-static-context-providers.md
Adds the 2026-06-19 amendment notice and rewrites the Decision section so that cacheKeyHash is computed from OFREP base URL, auth credential, bound domain, and targetingKey (with optional cacheKeyPrefix). Updates rationale bullets and the example persisted JSON to show the new hash inputs.
Cache matching, implementation notes, and open questions
service/adrs/0009-local-storage-for-static-context-providers.md
Updates cache-matching rules to use the new cacheKeyHash definition, extends implementation guidance to explicitly cover cache clearing on provider rebind to a different domain, and revises open questions to reflect that collision avoidance is now built into the default key and that cacheKeyPrefix leans toward removal.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: amending ADR 0009 to tie the cache key to OFREP resource components (domain, URL, auth), which is the core objective of this pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the proposed changes to ADR 0009, detailing the new cache key structure and motivation behind the amendment.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands and usage tips.

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 jonathannorris marked this pull request as ready for review June 19, 2026 15:06
@jonathannorris jonathannorris requested a review from a team as a code owner June 19, 2026 15:06

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 win

Align the cacheKeyPrefix wording everywhere.

This amendment says the prefix is only a supplemental namespace, but the earlier answer still describes it as wrapping targetingKey alone. 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

📥 Commits

Reviewing files that changed from the base of the PR and between d1a63a4 and ab411b2.

📒 Files selected for processing (1)
  • service/adrs/0009-local-storage-for-static-context-providers.md

Comment thread service/adrs/0009-local-storage-for-static-context-providers.md
…che key

Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 win

Clarify the rebinding contract.

domain-scoped says 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

📥 Commits

Reviewing files that changed from the base of the PR and between ab411b2 and 46615d6.

📒 Files selected for processing (1)
  • service/adrs/0009-local-storage-for-static-context-providers.md

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.

3 participants