feat(openfeature): improve exposure buffering - #9742
Conversation
Overall package sizeSelf size: 7.99 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | https-proxy-agent | 7.0.6 | 27.71 kB | 101.18 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB | | proxy-from-env | 2.1.0 | 15.64 kB | 15.64 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2026-08-09 18:07:46 Comparing candidate commit ec9d0a4 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2322 metrics, 36 unstable metrics.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ec9d0a4 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec9d0a4372
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| eventSize = Buffer.byteLength(JSON.stringify(event)) | ||
| } catch (error) { | ||
| log.warn('%s could not serialize an event, dropping event: %s', this.constructor.name, error.message) |
There was a problem hiding this comment.
When an exposure or its subject.attributes has a toJSON/getter that throws null or undefined, this catch block immediately throws a new TypeError while reading error.message; because flush() also runs from the interval and beforeExit, that exception can escape into and crash the instrumented application instead of dropping the malformed event. Log the caught value without assuming it is an Error, including in the identical catch inside #send.
AGENTS.md reference: AGENTS.md:L222-L225
Useful? React with 👍 / 👎.
| if (this._buffer.length < this._bufferLimit) { | ||
| this._buffer.push(event) |
There was a problem hiding this comment.
Because size validation now happens only during flush(), events over the 1 MB limit still consume slots and replace valid older exposures in this ring. For example, if the queue contains 1,000 valid events and a synchronous burst of oversized evaluation contexts arrives before the timer runs, those invalid events evict the valid batch and are then themselves discarded during flushing, potentially delivering nothing; the count cap also no longer prevents a burst of large unique objects from consuming excessive memory. Oversized events must not be allowed to displace sendable entries in the bounded queue.
Useful? React with 👍 / 👎.
dd-oleksii
left a comment
dd-oleksii
left a comment
There was a problem hiding this comment.
I think the only user-facing change I question is preserving recent events. The rest of comments is just me struggling with the code
| if (!this._dropWarningLogged) { | ||
| this._dropWarningLogged = true | ||
| log.warn( | ||
| '%s dropped exposure event(s) at cap %d. This may invalidate experiment results.', |
There was a problem hiding this comment.
minor: exposure-specific logging in a potentially shared "base" writer. It doesn't look like it's actually shared, so maybe we should merge the two — this would make the interaction between BaseFFEWriter and ExposuresWriter easier to follow
|
|
||
| /** @type {ExposureEvent[]} */ | ||
| #pendingEvents = [] | ||
| #routeResolved = false |
There was a problem hiding this comment.
minor: #routeResolved is a confusing name and actually seems to mean something other than it says:
- route can be set in constructor but
#routeResolvedis not set here setEnabledcan be called with empty/missingroutebut#routeResolvedis set here regardless
The most accurate name for the field is #setEnabledCalled but maybe it should be #isInitializing or similar
| this._buffer[this._bufferStart] = event | ||
| this._bufferStart = (this._bufferStart + 1) % this._bufferLimit |
There was a problem hiding this comment.
minor: is there a significant reason to prefer recent events? This seems to introduce more complexity and room for error.
Dropping newer events is also very slightly faster (because allocators optimize for young objects dying young).
| continue | ||
| } | ||
|
|
||
| if (batch.length > 0 && this._payloadSizeLimit && batchSize + eventSize > this._payloadSizeLimit) { |
There was a problem hiding this comment.
nitpick: the piece here looks like it would allow a single event to exceed the whole payload size limit (when batch.length === 0 && batchSize + eventSize > this._payloadSizeLimit).
batch.length > 0 could be removed because that's an impossible case. batchSize + eventSize > this._payloadSizeLimit implies batch is not empty
| if (batch.length > 0 && this._payloadSizeLimit && batchSize + eventSize > this._payloadSizeLimit) { | |
| if (this._payloadSizeLimit && batchSize + eventSize > this._payloadSizeLimit) { |
| continue | ||
| } | ||
|
|
||
| if (this._payloadSizeLimit && eventSize > this._payloadSizeLimit) { |
There was a problem hiding this comment.
nitpick: I was a bit confused by two almost identical checks. One way to generalize this is to make sure that the event size limit is ≤ payload size limit (which should always be the case), so we can drop the second check.
In constructor:
if (this._payloadSizeLimit) {
this._eventSizeLimit = Math.min(this._eventSizeLimit || Infinity, this._payloadSizeLimit)
}| this.constructor.name, PENDING_MAX_EVENTS) | ||
| } | ||
| } | ||
| super.append(events) |
There was a problem hiding this comment.
major (🐛): the base writer will try to periodically flush these events, ignoring #enabled. If a flush attempt happens before setEnabled is called, this will likely lead to loosing events (or them going through?) — not the result we want in either case
EDIT: no, the flush is overridden to skip flush when #enabled is false. I hate implementation inheritance. This is another place where the flow is convoluted — merging two classes would simplify it
2 participants
Stacked on #9741.
Motivation
Exposure buffering uses separate queues before and after route selection. The active writer also serializes events during
append().These paths have different capacity behavior. Payload overflow can also start a flush from the evaluation path.
Changes and Decisions
Validation
The shared system-test contract ran locally against
ec9d0a437.TEST_LIBRARY=nodejs ./run.sh FEATURE_FLAGGING_AND_EXPERIMENTATION tests/ffe/test_exposure_egress.pypassed one test.TEST_LIBRARY=nodejs ./run.sh FEATURE_FLAGGING_AND_EXPERIMENTATION_AGENTLESS_DIRECT tests/ffe/test_exposure_egress.pypassed one test.TEST_LIBRARY=nodejs ./run.sh FEATURE_FLAGGING_AND_EXPERIMENTATION_AGENTLESS_SERVERLESS tests/ffe/test_exposure_egress.pypassed one test withserverless-init:1.9.13.