docs: add ADR 0009 for static-context cache-first local persistence#64
docs: add ADR 0009 for static-context cache-first local persistence#64jonathannorris wants to merge 17 commits intomainfrom
Conversation
|
Cursor Agent can help with this pull request. Just |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces an Architectural Decision Record (ADR) proposing a standard approach for web and mobile OFREP providers to enhance offline functionality. It aims to improve user experience by enabling the persistence of feature flag evaluations in local storage, ensuring that applications can operate and maintain consistent feature states even with intermittent network connectivity or after restarts. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces ADR 0009, which proposes persisting static-context evaluations in local storage for web and mobile providers to improve offline capabilities. The ADR is well-structured and covers the main aspects of the proposal. My review includes a few suggestions to enhance the clarity and robustness of the proposed design, specifically regarding the handling of server errors during initialization, clarifying the purpose of the persisted timestamp, and making the initialization flow more explicit and unambiguous.
Note: Security Review has been skipped due to the limited scope of the PR.
e64867d to
9ac24a6
Compare
lukas-reining
left a comment
There was a problem hiding this comment.
Thanks @jonathannorris, left some feedback.
As you, I think this is a very important addition to the OFREP providers.
I have some thoughts regarding the cache key, which could also influence the whole concept due to the static context possibly also not being available as you said in open question no. 2.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
85eddc7 to
13ee5b3
Compare
Co-authored-by: Jonathan Norris <jonathannorris@users.noreply.github.com> Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Co-authored-by: Jonathan Norris <jonathannorris@users.noreply.github.com> Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Clarify ADR 0009 with provider behavior, persistence examples, and implementation guidance for local cached bulk evaluations. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Clarify initialization flow, explain the persisted timestamp, and define temporary server failures as eligible for persisted fallback. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Specify the cacheKeyHash formula and restore explicit open questions for reviewer feedback. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Document an explicit provider option for turning off persisted local storage. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Drop authToken from cache key derivation and replace sha256 with generic hash(), per reviewer feedback. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
…tion Specify CACHED as the evaluation reason when serving from persisted storage. Remove fallback scope open question since the decision section already addresses it. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Use must not for auth/config error fallback to prevent masking real problems. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Local storage availability is a platform constraint, not a consequence of the proposal. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Specify that flag values are stored in plaintext and accessible to same-origin code or compromised devices. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
The specific storage key and record model are implementation details. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Remove redundant implementation notes that overlap with the decision section. Simplify mermaid diagram initialize call to use context. Signed-off-by: Norris <jonathan.norris@dynatrace.com> Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Replace fallback-on-failure with cache-first initialization pattern aligned with vendor SDKs (LaunchDarkly, Statsig, DevCycle, Eppo). Provider loads from persisted cache immediately on startup, refreshes from network in background, and emits PROVIDER_CONFIGURATION_CHANGED when fresh values arrive. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
13ee5b3 to
5dbb057
Compare
Fix PROVIDER_FATAL to PROVIDER_ERROR with fatal error code per spec. Add rationale for READY vs STALE on cache-hit startup. Clarify cache key tradeoff (targetingKey vs full context). Note existing provider implementations will need lifecycle refactors. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces ADR 0009, which proposes a cache-first local persistence strategy for static-context OFREP providers. The ADR is well-structured and detailed, covering the motivation, design, consequences, and implementation considerations. The proposed cache-first initialization aligns with industry best practices and addresses the 'flash-of-defaults' problem effectively. I have a couple of suggestions to improve the clarity and security aspects of the proposal.
On the cache-hit path, if the background refresh fails with 401/403/400, the provider continues serving cached values for the current session but clears the persisted entry. This ensures the next cold start uses the cache-miss path, making auth errors immediately visible. Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Summary
PROVIDER_READY,PROVIDER_CONFIGURATION_CHANGED)hash(targetingKey), no auth token dependencyMotivation
Every major vendor SDK (LaunchDarkly, Statsig, DevCycle, Eppo) uses cache-first initialization by default. Current OFREP static-context providers keep their cache in memory only, losing all state on restart. See vendor mobile SDK caching research for a detailed comparison.
Related
Test plan