Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a crash/issue-report hook API so SDK consumers can inspect crash report details before upload and optionally attach additional log fields, plus adds a shared helper for identifying SDK-reserved (“core”) field names.
Changes:
- Added
CrashReportHook/CrashReportInfoinbd-crash-handlerand plumbed an optional hook throughMonitorto merge returned fields into the emitted crash log. - Added
with_crash_report_hooktobd-logger::LoggerBuilderto wire the hook into crash monitoring. - Added
bd-metadata::is_core_fieldand aCORE_FIELDSlist for reserved field name detection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| bd-metadata/src/lib.rs | Adds CORE_FIELDS + is_core_field helper for reserved/core field detection. |
| bd-logger/src/builder.rs | Adds builder plumbing to accept an optional crash report hook and pass it into Monitor. |
| bd-crash-handler/src/lib.rs | Defines the hook API, stores the hook in Monitor, invokes it during report processing, and merges returned fields. |
| bd-crash-handler/src/monitor_test.rs | Updates test setup to pass the newly-added hook parameter. |
| bd-crash-handler/src/file_watcher.rs | Removes an extraneous blank line (no functional change). |
💡 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 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
0fc003a to
73f8f5d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
4ef38f4 to
b52780d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
2b6e080 to
6343c88
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let info = CrashReportInfo { | ||
| report_type: Self::report_type_to_reason(bin_report.type_()).to_string(), | ||
| crash_reason: crash_reason.cloned(), | ||
| crash_details: crash_details.cloned(), | ||
| session_id: session_id.to_string(), | ||
| fields: state_fields.clone(), |
There was a problem hiding this comment.
CrashReportInfo.fields is populated with state_fields.clone() which includes SDK-generated OOTB fields (e.g. app_id, os_version) and internal fields prefixed with _ (e.g. _app_version_code). This contradicts the intention documented in bd-metadata::is_core_field that core/OOTB fields are filtered out for the crash report hook, and it may expose SDK-internal fields to hook implementers. Consider filtering state_fields before putting it into CrashReportInfo (e.g., drop keys starting with _ and keys in the OOTB/core list) and add/adjust tests accordingly.
| let info = CrashReportInfo { | |
| report_type: Self::report_type_to_reason(bin_report.type_()).to_string(), | |
| crash_reason: crash_reason.cloned(), | |
| crash_details: crash_details.cloned(), | |
| session_id: session_id.to_string(), | |
| fields: state_fields.clone(), | |
| // Filter out core/OOTB fields and internal fields (prefixed with '_') | |
| let filtered_fields: LogFields = state_fields | |
| .iter() | |
| .filter_map(|(key, value)| { | |
| if key.starts_with('_') || ::bd_metadata::is_core_field(key) { | |
| None | |
| } else { | |
| Some((key.clone(), value.clone())) | |
| } | |
| }) | |
| .collect(); | |
| let info = CrashReportInfo { | |
| report_type: Self::report_type_to_reason(bin_report.type_()).to_string(), | |
| crash_reason: crash_reason.cloned(), | |
| crash_details: crash_details.cloned(), | |
| session_id: session_id.to_string(), | |
| fields: filtered_fields, |
| #[derive(Debug, Clone)] | ||
| pub struct CrashReportInfo { | ||
| pub report_type: String, | ||
| pub crash_reason: Option<String>, | ||
| pub crash_details: Option<String>, | ||
| pub session_id: String, | ||
| pub fields: LogFields, |
There was a problem hiding this comment.
CrashReportInfo.report_type currently contains the human-readable string returned by report_type_to_reason (e.g. "Crash" for ReportType::JVMCrash). The field name report_type reads like it would carry the actual report type identifier, so this is easy for API consumers to misinterpret. Consider either renaming the field to something like an "exit reason"/"display name", or changing the type to a structured enum that distinguishes the underlying report type from a presentation string.
What
TBF
Tech spec doc
Verification
Wired in this PR bitdriftlabs/capture-sdk#904