docs: add ADR for SSE bulk evaluation change notifications#63
docs: add ADR for SSE bulk evaluation change notifications#63jonathannorris merged 26 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
|
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
There was a problem hiding this comment.
Added a couple of comments for discussion.
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>
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.
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.
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
… query param rationale Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jonathan Norris <jonathan@taplytics.com>
e875fcb to
e191390
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
| "refreshConnections": [ | ||
| { | ||
| "type": "sse", | ||
| "url": "https://sse.example.com/event-stream?channels=env_abc123_v1", |
There was a problem hiding this comment.
The URL should be split between hostname and path properties.
This allows for systems that need to override the endpoint without having to hackily substring and replace. Generally required by larger/proxied users.
There was a problem hiding this comment.
I think keeping url as a single opaque string is the right call here. The server fully controls the URLs it returns in the response, so if a proxied setup needs different endpoints the server can return the correct URLs directly. I don't think we should be modifying these URLs in the provider layer.
There was a problem hiding this comment.
I think keeping url as a single opaque string is the right call here.
I think so for the same reason.
There was a problem hiding this comment.
A proxied setup will need to be able to re-write the URL; while keeping the path and auth/channel specifics it will need to be able to re-write the hostname and protocol.
Using a regex solution to do this in the proxy doesn't exactly seem the correct solution, and quite hacky.
There was a problem hiding this comment.
Made an update here to support proxied/origin-rewrite setups without changing the default model.
url remains the default representation, but the ADR now also allows a structured endpoint form with origin + requestUri for implementations that need to override the origin cleanly while preserving the request target. The intent is that exactly one of url or endpoint is provided for an event stream entry.
There was a problem hiding this comment.
I am not sure about this.
How would that work? How would a provider implementation use the origin + requestUri to support a proxied setup?
@JamieSinn could you elaborate on how you would imagine to use this?
I would like to avoid any extra options that are not necessary or giving a larger benefit.
There was a problem hiding this comment.
This is something we're likely to use internally for server-side SSE connections at Dynatrace.
In our previous proxy setup at DevCycle, we effectively re-broadcast SSE messages from the proxy to internal SDKs/providers. In that model, the provider needs to connect to the local proxy origin, while preserving the path/query/channel-specific parts of the original SSE endpoint. Keeping this as a single opaque url makes that override logic pretty awkward and usually turns into string replacement.
There was a problem hiding this comment.
This is something we're likely to use internally for server-side SSE connections at Dynatrace.
Yesh I guessed so.
In that model, the provider needs to connect to the local proxy origin, while preserving the path/query/channel-specific parts of the original SSE endpoint. Keeping this as a single opaque url makes that override logic pretty awkward and usually turns into string replacement.
I am not convinced of that. In my view adding special parameters for a non-standard case that can trivially be replaced by leaving the splitting of the URL to the implementer of that proxy logic.
JS Example:
const urlString = 'https://example.com:8080/path/to/page?param1=value1¶m2=value2#hash';
const url = new URL(urlString);
const origin = url.origin;
const pathAndParams = url.pathname + url.search + url.hash; There was a problem hiding this comment.
Yep - happy to expand on it.
With DevCycle - we had to split this between hostname and path due to the way that our downstream SDK proxy needed to split the endpoints and rewrite them based on the host request parameters that were incoming.
You can see some of this implementation here:
https://github.com/DevCycleHQ/sdk-proxy/blob/main/http_endpoints.go#L247
https://github.com/DevCycleHQ/sdk-proxy/blob/main/http_endpoints.go#L199
If we were to implement the url concept and the SDK proxy were to operate in cloud mode (which it can, and quite a few customers do use it in) - then there is no way to re-write the url property without putting the breaking nature of the path/upstream origin on the SDK proxy implementation.
This is definitely an ease of use request for the proxy use case - but I don't really see the harm in the additional option to have your hostname/endpoint path if the default is the full URL.
toddbaert
left a comment
There was a problem hiding this comment.
I have mostly nits, small additions, and naming improvement suggestions. Overall I think this is a great addition.
I do agree with @lukas-reining 's point here: #63 (comment)
… server-side providers Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
…y param answer Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
|
@lukas-reining / @thomaspoignant any further questions or approvals before I merge this? |
lukas-reining
left a comment
There was a problem hiding this comment.
Approval from me, I would just like to have sorted out the proxy url idea before merging.
Thanks again @jonathannorris, I know this took a lot of time :)
This PR
This PR adds ADR-0008 (#62) to propose Server-Sent Events (SSE) for bulk evaluation change notifications in OFREP.
refreshConnectionsarray to bulk evaluation responses, with atypediscriminator to support future push mechanisms beyond SSE.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.