RUM-17613: timeseries [3/5] Pass the live RUM context to timeseries pipelines - #3717
Conversation
🎉 All green!🧪 All tests passed 🔄 Datadog retried 1 test - 1 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: 341c6b1 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tvaleev/feature/RUM-17613-2-timeseries-schema #3717 +/- ##
=================================================================================
- Coverage 73.41% 73.39% -0.02%
=================================================================================
Files 1012 1012
Lines 37072 37074 +2
Branches 6385 6384 -1
=================================================================================
- Hits 27216 27210 -6
+ Misses 8034 8033 -1
- Partials 1822 1831 +9
🚀 New features to boost your workflow:
|
a2fbff9 to
27abec6
Compare
2c34b26 to
61448ac
Compare
satween
changed the title
satween
changed the title
timeseries [3/4] Flush the batch in background
![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: 61448ace2d
ℹ️ 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".
| val dataPoint = reader.read() | ||
| synchronized(this) { |
There was a problem hiding this comment.
When a session or the RUM feature stops while a scheduled tick is inside reader.read(), onSessionStop() can flush the buffer before this block acquires the lock. The tick then adds its point after the final flush, does not reschedule because its generation is inactive, and the collector is discarded, so that point is permanently lost. The terminal flush needs to wait for an in-flight read/add operation or arrange another flush after the point is added.
Useful? React with 👍 / 👎.
Timeseries batches carried the RumContext captured when RumSessionScope created the collector, so every event was attributed to whatever view was active at session start. The collector now tracks the context and hands it to Pipeline.execute()/flush() per call, and RumSessionScope feeds it the active context on each handled event. Pipeline takes over its own synchronization as part of that: reader.read() stays outside the lock so a concurrent flush() never waits for /proc I/O. Ref: RUM-17613
27abec6 to
40fa7af
Compare
61448ac to
341c6b1
Compare
satween
changed the title
timeseries [3/4] Flush the batch in backgroundtimeseries [3/5] Pass the live RUM context to timeseries pipelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 341c6b1286
ℹ️ 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".
| synchronized(pipeline) { | ||
| if (isActive(generation)) pipeline.execute() | ||
| } | ||
| pipeline.execute(rumContext) |
There was a problem hiding this comment.
When a view transition is handled while reader.read() is in progress, this argument has already captured the previous rumContext; after the read completes, Pipeline.execute() can drain the buffer using that stale view ID even though the new view is active. This contradicts the intended drain-time attribution and can compute session.hasReplay from the wrong view, so the live context should be obtained after sampling, at the point the buffer is drained.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Timeseries batches carried the
RumContextcaptured whenRumSessionScopecreated the collector, so every event was attributed to whatever view happened to be active at session start. The collector now tracks the context and hands it toPipeline.execute()/Pipeline.flush()per call, andRumSessionScopefeeds it the active context on every handled event.Pipelinetakes over its own synchronization as part of that:reader.read()stays outside the lock, so a concurrentflush()waits only for the buffer and never for/procI/O.The sampling lifecycle is unchanged here — background suspension still behaves exactly as before, gated on
TimeseriesConfiguration.collectInBackground. [4/5] reworks it.Review checklist (to be filled by reviewers)
Ref: RUM-17613