docs: add ADR for SSE bulk evaluation change notifications#63
docs: add ADR for SSE bulk evaluation change notifications#63jonathannorris wants to merge 11 commits intomainfrom
Conversation
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Summary of ChangesHello @jonathannorris, 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 a new Architectural Decision Record (ADR) to integrate Server-Sent Events (SSE) into the OFREP specification. The primary goal is to enable real-time flag updates and reduce server load by moving away from exclusive polling for change detection. The ADR defines how SSE endpoints will be communicated, the format of notification events, and the expected behavior for providers to manage these connections and re-fetch evaluations efficiently. Highlights
Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces ADR-0008, proposing Server-Sent Events (SSE) for bulk evaluation change notifications in OFREP. The document outlines the motivation, decision, response schema, event format, provider behavior, OpenAPI schema additions, consequences, open questions, and implementation notes. The changes are well-documented and address a significant limitation of the current polling-only approach. The ADR is comprehensive and considers various aspects of SSE integration, including potential complexities and risks. The open questions section is particularly valuable for guiding future discussions and refinements.
There was a problem hiding this comment.
Pull request overview
This PR adds ADR-0008 to propose Server-Sent Events (SSE) as a standardized mechanism for real-time flag change notifications in OFREP, addressing the polling limitations explicitly acknowledged in ADR-0005. The ADR follows the established pattern of building on vendor survey feedback and maintaining backward compatibility.
Changes:
- Introduces optional SSE connection endpoints in bulk evaluation responses for real-time change notifications
- Defines a notification-only pattern where SSE events trigger re-fetches rather than streaming full payloads
- Specifies SSE-specific metadata transport via query parameters (
sseEtag,sseLastModified) for SSE-triggered requests
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
askpt
left a comment
askpt
left a comment
There was a problem hiding this comment.
Added a couple of comments for discussion.
| Server-Sent Events (SSE) is a W3C standard that fits this use case well: | ||
| - Unidirectional (server-to-client), matching the notification pattern | ||
| - Runs over standard HTTP without protocol upgrades | ||
| - Natively supported in browsers via the `EventSource` API |
There was a problem hiding this comment.
I would consider adding that mobile is also supported. After a quick search I confirmed that.
There was a problem hiding this comment.
yea Mobile should be supported with SSE will make that clearer. I recommend that we stick to the LaunchDarkly Event Source libraries where its not built-in:
There was a problem hiding this comment.
@jonathannorris can you please add that there are implementations for all mobile platforms supported by OF as suggested by @askpt?
| ## Open Questions | ||
|
|
||
| 1. **Should `refetchEvaluation` be required, or should providers refetch on any SSE message?** Requiring a specific `type` field enables future event types without triggering unnecessary refetches. Refetching on any message is simpler. This ADR recommends requiring `type=refetchEvaluation` for forward compatibility. | ||
| 2. **Should providers support streaming full evaluation payloads over SSE?** This ADR focuses on the notification pattern. Full payload streaming could be specified as a separate event type in a future revision. |
There was a problem hiding this comment.
I believe as a potential v2 for SSE, I would like to experiment having a json PATCH similar behaviour. What I mean is, instead of forcing a full refresh, the event would send the flags that need to be updated.
For example:
[
{ "op": "add", flagKey: "test", value: { } },
{ "op": "replace", flagKey: "test", "path": "/defaultValue", "value": "false" },
{ "op": "remove", flagKey: "test"}
]
This adds extra complexity but it would be a good improvement.
There was a problem hiding this comment.
yea makes sense, but yea I would leave that for a new ADR.
There was a problem hiding this comment.
Tbh., to me this would be the way out of the box as described above.
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
…iminator Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
jonathannorris
force-pushed
the
feature/adr-0008-sse-bulk-evaluation
branch
from
0f4ec8c to
94b1fc6
Compare
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
…events Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Add an optional `refreshConnections` array to the bulk evaluation response (`POST /ofrep/v1/evaluate/flags`). When present, it provides connection endpoints that the provider connects to for real-time flag change notifications. | ||
| This is primarily intended for static-context providers (for example web/mobile clients) that rely on bulk evaluations. |
There was a problem hiding this comment.
The PR description/linked issue describe adding an optional sse connection array, but this ADR specifies a refreshConnections field. This mismatch will be confusing for implementers; either update the PR description/issue text to match refreshConnections or adjust the ADR terminology to match the intended field name.
| ``` | ||
| id: evt-1234 | ||
| event: message | ||
| data: {"type": "refetchEvaluation", "etag": "\"abc123\"", "lastModified": 1771622898} | ||
| ``` |
There was a problem hiding this comment.
The document uses two different concepts for “event type”: the SSE envelope example shows event: message while later sections talk about receiving a refetchEvaluation event. Please make it explicit whether refetchEvaluation is the SSE event: name or the JSON data.type, and keep the examples/diagram consistent to avoid ambiguous client implementations.
| SSE-->>Client: event: refetchEvaluation (etag, lastModified) | ||
| Client->>Server: POST /ofrep/v1/evaluate/flags?sseEtag=etag&sseLastModified=lastModified | ||
| alt Flags changed | ||
| Server-->>Client: 200 OK (new flags + ETag) | ||
| Client->>Client: Update cache, emit ConfigurationChanged | ||
| else Flags unchanged | ||
| Server-->>Client: 304 Not Modified | ||
| end |
There was a problem hiding this comment.
The sequence diagram shows SSE-triggered re-fetches as POST /ofrep/v1/evaluate/flags?sseEtag=... and then expects a 304 Not Modified response. HTTP 304 semantics require a conditional request (e.g., If-None-Match), and the existing static provider guideline requires sending If-None-Match when an ETag is available. Clarify that SSE-triggered re-fetches should still include the normal conditional request headers (plus optional SSE metadata), otherwise implementers may lose 304-based cache validation.
|
It would be helpful to include an error scenario as well. At the moment, the ADR focuses only on the happy-path flow. For example, consider a data center issue where some pods have already been updated while others have not. The client receives a refresh event over SSE, but the subsequent refetch request is routed to a pod that has not yet been updated. How should this situation be handled? |
beeme1mr
requested review from
erka,
lukas-reining,
thomaspoignant and
toddbaert
|
|
||
| OFREP currently relies exclusively on polling for flag change detection in client-side (static context) providers. As described in [ADR-0005](0005-polling-for-bulk-evaluation-changes.md), polling was chosen initially for simplicity, with the explicit expectation that additional change detection mechanisms would be added later. | ||
|
|
||
| This ADR focuses primarily on static-context providers (for example web and mobile SDK providers) that use bulk evaluation caching patterns. It does not introduce SSE requirements for dynamic-context providers that primarily use single-flag evaluations. |
There was a problem hiding this comment.
Why only for static-context?
While I agree that this is the much more needed use case, the PROVIDER_CONFIGURATION_CHANGED event would still be good to support for OFREP in dynamic context scenarios with this addition.
| Server-Sent Events (SSE) is a W3C standard that fits this use case well: | ||
| - Unidirectional (server-to-client), matching the notification pattern | ||
| - Runs over standard HTTP without protocol upgrades | ||
| - Natively supported in browsers via the `EventSource` API |
There was a problem hiding this comment.
@jonathannorris can you please add that there are implementations for all mobile platforms supported by OF as suggested by @askpt?
|
|
||
| ## Decision | ||
|
|
||
| Add an optional `refreshConnections` array to the bulk evaluation response (`POST /ofrep/v1/evaluate/flags`). When present, it provides connection endpoints that the provider connects to for real-time flag change notifications. |
There was a problem hiding this comment.
I do not find refreshConnections very speaking, but this might just be an opinion of mine.
To me something like providerStaleneChannels or providerConfiguarationChangedChannels, would be more intuitive and reflect the spec. Even though I am not happy with channel but I could not come up with something better yet.
https://openfeature.dev/docs/reference/concepts/events/#provider_stale
| Add an optional `refreshConnections` array to the bulk evaluation response (`POST /ofrep/v1/evaluate/flags`). When present, it provides connection endpoints that the provider connects to for real-time flag change notifications. | ||
| This is primarily intended for static-context providers (for example web/mobile clients) that rely on bulk evaluations. | ||
|
|
||
| SSE is used as a **notification-only** mechanism -- events signal the provider to re-fetch the bulk evaluation via the existing endpoint, rather than streaming full evaluation payloads. This keeps the SSE message format simple, reuses existing infrastructure, and avoids duplicating evaluation logic. |
There was a problem hiding this comment.
I am going back and forth with this. Your arguments for this make sense.
But for many clients this would still mean huge amount of requests coming in parallel if all providers are notified in parallel. You could delay the events but that would contradict our effort to enable real time update. This can also become complex.
Sending the diff (or all flags if wanted) would make this much more predictable in my view. The downside would be so decide which flag to update but maybe simple UPSERT or per flag instructions would make it.
|
|
||
| Each refresh connection object has: | ||
| - `type` (string, required): The connection type. Currently `"sse"` is the only defined value. Providers must ignore entries with unknown types for forward compatibility, allowing new push mechanisms to be added without breaking existing clients. | ||
| - `url` (string, required): The endpoint URL. The URL is opaque to the provider and may include authentication tokens, channel identifiers, or other vendor-specific query parameters. Implementations must treat this URL as sensitive -- it may contain auth tokens or channel credentials -- and must not log or persist the full URL including query string. |
There was a problem hiding this comment.
The opaqueness is a very good idea!
| ## Open Questions | ||
|
|
||
| 1. **Should `refetchEvaluation` be required, or should providers refetch on any SSE message?** Requiring a specific `type` field enables future event types without triggering unnecessary refetches. Refetching on any message is simpler. This ADR recommends requiring `type=refetchEvaluation` for forward compatibility. | ||
| 2. **Should providers support streaming full evaluation payloads over SSE?** This ADR focuses on the notification pattern. Full payload streaming could be specified as a separate event type in a future revision. |
There was a problem hiding this comment.
Tbh., to me this would be the way out of the box as described above.
This PR
This PR adds ADR-0008 (#62) to propose Server-Sent Events (SSE) for bulk evaluation change notifications in OFREP.
sseconnection array to bulk evaluation responses.refetchEvaluation) and provider lifecycle guidance (reconnect, fallback, coalescing).sseEtag,sseLastModified) only for requests triggered directly by SSE messages.Notes
Includes open questions on query params vs custom headers, replay guarantees (
Last-Event-ID), and security expectations for tokenized SSE URLs.