feat: Include RUM context in crash reports#182
Conversation
d2f54f9 to
4a1526c
Compare
|
✅ Code Quality ✅ Code Vulnerabilities ✅ Library Vulnerabilities ✅ Secrets 🎉 All green!🛠️ No new code quality issues 🔗 Commit SHA: 9dd9597 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
ee9599c to
4a1526c
Compare
|
/gitlab resync-job-status |
|
View all feedbacks in Devflow UI.
30 jobs status updated and 12 skipped for a total of 42 jobs |
4a1526c to
473bd16
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 473bd16d1e
ℹ️ 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".
| _rum_application_id.ToBytes(buf, std::size(buf)); | ||
| s_rum_application_id.Set(buf); |
There was a problem hiding this comment.
NUL-terminate UUID buffer before setting Crashpad annotations
UUID::ToBytes serializes exactly 36 characters without a terminator, but this buffer is passed directly to StringAnnotation::Set as a C-string. When buf[36] is non-zero, annotation writes can include trailing stack garbage (and potentially read past intended bounds), so crash uploads may contain corrupted RUM IDs. Ensure the buffer is explicitly terminated (or use a length-aware setter) before calling Set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed with 73a2a76; buffer is now zero-filled to ensure null-termination.
| if (!self || !self->_crash_handler) { | ||
| return; |
There was a problem hiding this comment.
Skip RUM context writes when crash handler init failed
This handler only checks _crash_handler for non-null, but Start() keeps _crash_handler even when Initialize() returns false. In that failure mode, context-change messages still invoke SetRumContext; for the in-process backend this attempts file writes despite failed setup (for example after .crashes creation/open failure), causing repeated failed I/O and possible stray temp-file behavior. Track initialization success (or clear _crash_handler on failure) before forwarding context updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed with 9dd9597; impl::CrashReporting now discards and destroys its _crash_handler reference if ICrashHandler::Initialize() fails. Once the feature is started, it either has a valid, initialized handler or it has no handler; there is no longer an inbetween state.
| static char s_crash_filename[256]; // Path to crash report file | ||
| static char s_crash_context_filename[260]; // Path to companion crash context file |
There was a problem hiding this comment.
I hate to be that guy but.... are we sure 256 / 260 are the right values here? Is there much harm in doubling it?
There was a problem hiding this comment.
This is a good point, especially since we ultimately want crash report files to be stored within the user-configurable storage path. I'll bump these buffer sizes for both POSIX and Win32 implementations.
|
Other than my nit about file path size, this LGTM. |
|
(A forthcoming change which standardizes how we store filesystem paths will address the 256-char limit noted earlier. I'm going to merge this PR as-is, since crash report storage paths will get bumped to a limit of ~512 once they incorporate that change.) |
refs: RUM-14026
This PR builds upon #181 to make the
CrashReportingfeature respond to changes inCoreContext, allowing the crash handler implementation to be notified whenRumFeatureContextchanges so that it can persist RUM UUIDs to be included in crash reports.CrashReportingimplementsMakeMessageHandler, returning a function that handlesContextChangedMessageICrashHandler::SetRumContext, if and only if RUM UUIDs have changedCrashpadCrashHandlerimplementsSetRumContextby formatting UUIDs as strings and storing them incrashpad::StringAnnotationvaluescrashpad_handlerprocess reads these values from the crashing process's memory and includes them in the multipart/form-data uploadInProcessCrashHandlerimplementsSetRumContextby flushing UUIDs to disk in a binary formatcrash_<timestamp>_<pid>: kept open for the lifetime of the SDK; written to from async-signal-safe path in the event of a crashcrash_<timestamp>_<pid>.ctx: repopulated in full and closed/flushed any time crash context changes.ctx.tmpand then rename to.ctxto ensure atomic writesSetRumContextis called, indicating that UUIDs have meaningfully changed, those binary values are reserialized to the context fileNoopCrashHandlerimplementsSetRumContextby doing nothing :)Additionally, the temporary Python tooling in
process-crash-reportis updated to read the context file and parse RUM IDs, allowing us to validate that our crash reports have accurate context.