feat: Add context thread, enqueue all RUM and Logging operations for async processing#169
Conversation
394d726 to
8d8cf2c
Compare
e992061 to
16d264d
Compare
16d264d to
d5ae4ae
Compare
It's only used on the context thread, which processes functions serially, so it does not require synchronization.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5ae4aeb35
ℹ️ 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".
| _context_queue->Push([this, func]() { | ||
| DATADOG_ASSERT(_context_provider, "FeatureScope has no _context_provider"); | ||
| const CoreContext context = _context_provider->Get(); | ||
| func(context, _event_generated_func); |
There was a problem hiding this comment.
Avoid dangling
this captures in context queue thunks
ExecuteOnContextThread enqueues a lambda that captures this, then dereferences _context_provider and _event_generated_func later on the context thread. During shutdown, Core::Stop() calls each feature's OnCoreStopping() (which resets/destroys FeatureScope) before it drains the context queue, so any queued thunk that has not run yet can execute with a dangling FeatureScope pointer and trigger use-after-free/crashes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a fair point - fixed in 1dc05e9 by modifying the shutdown order such that the context queue is drained, then all features are stopped, then the storage and upload threads are stopped.
As a consequence, it's no longer possible for a feature implementation to do last-minute work (or produce last-minute events) in response to Feature::Stop() - see 77dfaa9. This is appropriate, as SDK stop is designed to be an immediate, non-blocking operation - in existing RUM SDKs, features are not traditionally given the opportunity to produce events in response to core.stop().
Note that in its current form, the C++ SDK still drains both the context queue and storage queue on shutdown, making Core::Stop() a blocking operation whose overhead scales with the amount of pending work enqueued by the application's API calls. This is nifty in that we can guarantee that if an API call produces an event, that event will be flushed to disk reliably, but it's inconsistent with the behavior of the iOS SDK, which treats shutdown as immediate and non-blocking, abandoning all pending work unless using a special integration-test-only shutdown method. I've created RUM-15042 to track this design discrepancy.
| _context_queue->Push([this, callback]() { | ||
| DATADOG_ASSERT(_context_provider, "FeatureScope has no _context_provider"); | ||
| _context_provider->Update(callback); | ||
| }); |
There was a problem hiding this comment.
Update CoreContext immediately when already on context thread
UpdateContext now always re-enqueues the mutation, even when the caller is already running on the context thread. RUM command processing calls UpdateFeatureContext() from inside ExecuteOnContextThread, so the actual context write is deferred behind already-queued work; a subsequent log callback queued right after a RUM call can therefore run before the RUM context update and be enriched with stale/missing RUM IDs, regressing correlation accuracy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a legitimate error in the implementation. As originally written, calling a RUM API function would enqueue a single function to run on the context thread. That function would perform its command-processing work (updating internal state and producing RUM events), then enqueue another function that would mutate the CoreContext to add the latest RUM UUIDs.
That second function would be enqueued at the end of the queue, causing a legitimate bug. e.g. if you made these three API calls back-to-back:
logger.Info("log1");rum.StartView("my-view");logger.Info("log2");
...then the second log event would be missing the RUM View ID set for my-view.
This is fixed in dac8a27: we now enqueue our two functions back-to-back, serially, in response to a RUM API call, such that in the example above, log2 will consistently see the updated View ID set by StartView.
|
✅ Code Quality ✅ Code Vulnerabilities ✅ Library Vulnerabilities ✅ Secrets 🎉 All green!🛠️ No new code quality issues 🔗 Commit SHA: fcbdf93 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
This function is used to tear down feature state; it may not be used to generate events or perform any work on the context thread (which is now stopped before the call to Feature::Stop()). This is consistent with the behavior of the iOS SDK, which treats Stop() as an immediate, non-blocking shutdown, abandoning all work on the context thread rather than draining the queue.
aa691a9 to
77dfaa9
Compare
|
@codex review Two P1 issues were flagged in a prior review - please verify that both are fixed:
Please confirm these are correctly addressed. |
We don't need two separate definitions of the same function type; it's clearer to just use `EventWriter` everywhere.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77dfaa9f1a
ℹ️ 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".
| // thread or tear down feature state. | ||
| // TODO(RUM-15042): Other SDKs abandon the context thread and shut down in a | ||
| // non-blocking fashion, without draining the context queue | ||
| _context_queue->Stop(); |
There was a problem hiding this comment.
Keep stop hooks able to mutate context before queue shutdown
Stopping the context queue before invoking OnCoreStopping() causes stop-time context mutations to be silently dropped, because FeatureScope::UpdateContext() enqueues via Push() and does not handle a false return once the queue is stopped. In this commit, Rum::Stop() still depends on UpdateContext() to clear ctx.rum, so after a stop/start cycle stale RUM IDs can remain in CoreContext until a later RUM command runs, and early log events may be enriched with incorrect session/view/action IDs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fcbdf93 - rather than requiring each feature to reset its own subset of CoreContext state on Stop() (which can no longer be done because Feature-induced CoreContext mutations are now asynchronous, and no further async work can be done on Stop()), we now just have Core::Start() preemptively reset CoreContext to a clean starting value.
Previously, each feature that stored custom state in `CoreContext` (in practice, just `Rum`) was responsible for clearing its own `CoreContext` state in `Feature::Stop()` so that, for example, if you stop and SDK instance and then restart it, you won't accidentally use stale RUM IDs from its past life. That was a simple enough approach when `CoreContext` mutations happened synchronously, but now that we're using a context thread (which has to be shut down before `Feature::Stop()`), this approach no longer makes sense. It's much simpler to just have the Core itself reset the `CoreContext` value each time it starts. This guarantees that the SDK will always use a clean `CoreContext` on `Start()`, regardless of whether it's stopped and restarted, and without requiring any feature-specific cleanup logic or tricky synchronization on shutdown.
|
@codex review One P1 issue was flagged in a prior review:
Please verify that the issue is fixed. |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
fuzzybinary
left a comment
There was a problem hiding this comment.
Codex caught way more than I would have on this one.
Changes LGTM.
| const EventWriter& event_writer, | ||
| const DiagnosticLogger& in_diagnostic_logger, | ||
| FeatureScope::ExecutionMode mode, | ||
| Queue<std::function<void()>>* context_queue |
There was a problem hiding this comment.
I really wish C++ had a nice way to enforce single ownership with non-owning observers that didn't require shared_ptr.
| ) const; | ||
|
|
||
| void Dispatch(const RumCommand& command); | ||
| void DispatchAsync(const RumCommand& command); |
There was a problem hiding this comment.
I appreciate being more explicit here but we probably could have left this named Dispatch and it would have been fine.
There was a problem hiding this comment.
Yeah, I agree - one of those cases of "the LLM made a silly style choice and I didn't get around to correcting it". I may fix it later 😅
This PR establishes a third background thread (in addition to the storage and upload threads) that's owned by the
Core- the context thread - and it updates theFeatureScopeinterface such that a feature must offload work to this thread in order to 1.) read values fromCoreContext, 2.) mutateCoreContext, or 3.) produce events.This change brings the implementation of the C++ SDK more in line with the design of the iOS and Android SDKs. By design, any significant work done in response to an API call now happens on background threads. A few things have to be done synchronously on the calling thread, but thereafter, the feature enqueues a function on the context queue, and that function will be invoked on the context thread once it's consumed from the queue.
This approach has a couple of key benefits:
The two feature implementations affected by this change are
LoggingandRum. Both sets of API functions now perform the majority of their work (including event construction and JSON serialization) on the context thread.Example per-thread activity on StartView
For example, in the previous implementation, here's what typically happened when the application called
Rum::StartViewfrom its main thread:Rum::StartView:impl::Rumconstructs aStartViewcommandimpl::Rumdispatches that command to the rootRumApplicationScopeRumApplicationScopepropagates command toRumSessionScopeRumSessionScopehandles view lifecycle logic; createsRumViewScope; propagates commandRumViewScopeupdates internal state in response to commandRumViewScopeexamines relatives in scope tree to build a snapshot current RUM stateRumViewScopeassembles a merged set of user attributesRumViewScopeacquires a shared mutex for read and makes an immutable snapshot ofCoreContextand retrieves os info, device info, etc.RumViewScopecombines all of the data it's gathered into aRumViewEventRumViewScopeacquires a mutex in order to access a shared JSON buffer, then serializes the event to JSON, then copies the encoded payload onto the storage queueimpl::Rumexamines the updated scope tree to obtain a new snapshot of Rum application stateimpl::Rumacquires a shared mutex for write and mutatesCoreContextwith up-to-date Rum contextRum::StartViewreturns control to the application.By contrast, here's what happens as of this PR:
Rum::StartView:impl::Rumconstructs aStartViewcommandimpl::Rumconstructs astd::weak_ptrto itselfimpl::Rumconstructs astd::function, capturing both theweak_ptrand the command by value, and copies it onto the context queueRum::StartViewreturns control to the application.CoreContext, then calls the callbackstd::weak_ptr, obtaining a stablestd::shared_ptr<impl::Rum>RumApplicationScopeRumApplicationScopepropagates command toRumSessionScopeRumSessionScopehandles view lifecycle logic; createsRumViewScope; propagates commandRumViewScopeupdates internal state in response to commandRumViewScopeexamines relatives in scope tree to build a snapshot current RUM stateRumViewScopeassembles a merged set of user attributesRumViewScopecombines all of the data it's gathered, along with data passed in via theCoreContextarg, into aRumViewEventRumViewScopeserializes the event to JSON in a persistent buffer (only used by the context thread, so no lock needed), then copies the encoded payload onto the storage queueimpl::Rumexamines the updated scope tree to obtain a new snapshot of Rum application stateimpl::Rumenqueues a function to the context queue that will mutateCoreContextwith up-to-date Rum context once it's processed by the context threadTakeaways from this quick comparison:
CoreContextchanges is predictable and ordered: e.g. a log message will be properly enriched with the RUM Context that was current at the time the log call was madeProfiling results
I made a quick ad hoc repl benchmark that just creates 10 views, adds 5 custom actions per view, and makes 3 log calls for each action, for a total of 60 RUM API calls and 150 log calls, in as short a span of time as possible.
These graphs show the state of
mainbefore these changes (and before the accompanyingstd::string_view->std::stringchange in RUM command types) in purple, and the latest changes in blue.As you'd expect, peak memory usage is a bit higher, because we have an extra queue now, and we have to copy more values in order to shuttle data around between threads:
Likewise, we're performing more allocations per API call:
(This is something I'd like to profile more exhaustively and ultimately optimize, via approaches like CoW, string interning, inline buffers, pool allocators, etc.; but the focus for now remains on building critical SDK functionality.)
However, we can see a clear improvement in main-thread CPU time per API call:
Previously, a RUM API call would take somewhere around 3 microseconds, give or take 1μs. As of this PR, RUM API calls are consistently in the sub-microsecond range.
Additionally, looking at the first few API calls (which historically block for much longer than is typical, because they're performing initial up-front allocations etc.) clearly shows the benefits of offloading this work to the context thread: we no longer block the application on a cold start of the SDK.