[Issue Reporter Hook] Emit callback with issue report details#414
[Issue Reporter Hook] Emit callback with issue report details#414FranAguilera wants to merge 6 commits intomainfrom
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.
| // SDK-internal ("core") field names that should not be exposed as custom fields. | ||
| // Fields prefixed with "_" are also considered core fields. | ||
| // See https://docs.bitdrift.io/sdk/features-fields.html#custom-fields | ||
| const CORE_FIELDS: &[&str] = &[ | ||
| "app_id", | ||
| "app_version", | ||
| "carrier", | ||
| "config_version", | ||
| "context", | ||
| "detail", | ||
| "device_id", | ||
| "foreground", | ||
| "log_level", | ||
| "model", | ||
| "network_type", | ||
| "os", | ||
| "os_version", | ||
| "platform", | ||
| "radio_type", | ||
| "reason", | ||
| "report_type", | ||
| "sdk_version", | ||
| "session_id", | ||
| ]; | ||
|
|
||
| #[must_use] | ||
| pub fn is_core_field(field: &str) -> bool { | ||
| field.starts_with('_') || CORE_FIELDS.contains(&field) | ||
| } |
There was a problem hiding this comment.
good point bot, should we reuse?
| /// Sets a hook that is invoked before each crash report is sent. The hook can inspect | ||
| /// the report and return additional log fields. A timeout is enforced to prevent blocking. | ||
| #[must_use] |
There was a problem hiding this comment.
The docstring says "A timeout is enforced to prevent blocking", but the hook is passed through to bd_crash_handler::Monitor and invoked synchronously without any timeout protection. Either implement an actual timeout (e.g., call the hook in a dedicated task/thread and bound wait time) or adjust the documentation to match the current behavior.
There was a problem hiding this comment.
Still talks about a timeout here
There was a problem hiding this comment.
good catch, updating in sec
| fields.extend(hook_fields); | ||
|
|
There was a problem hiding this comment.
Because crash log field annotation happens later by marking all fields as LogFieldKind::Ootb, any hook-supplied fields added here will also be treated as OOTB. If the hook is meant for user/custom fields, consider plumbing hook fields through as AnnotatedLogFields (or otherwise tagging them as Custom) so they don't get misclassified.
| /// Sets a hook that is invoked before each crash report is sent. The hook can inspect | ||
| /// the report and return additional log fields. A timeout is enforced to prevent blocking. | ||
| #[must_use] |
There was a problem hiding this comment.
Still talks about a timeout here
| // SDK-internal ("core") field names that should not be exposed as custom fields. | ||
| // Fields prefixed with "_" are also considered core fields. | ||
| // See https://docs.bitdrift.io/sdk/features-fields.html#custom-fields | ||
| const CORE_FIELDS: &[&str] = &[ | ||
| "app_id", | ||
| "app_version", | ||
| "carrier", | ||
| "config_version", | ||
| "context", | ||
| "detail", | ||
| "device_id", | ||
| "foreground", | ||
| "log_level", | ||
| "model", | ||
| "network_type", | ||
| "os", | ||
| "os_version", | ||
| "platform", | ||
| "radio_type", | ||
| "reason", | ||
| "report_type", | ||
| "sdk_version", | ||
| "session_id", | ||
| ]; | ||
|
|
||
| #[must_use] | ||
| pub fn is_core_field(field: &str) -> bool { | ||
| field.starts_with('_') || CORE_FIELDS.contains(&field) | ||
| } |
There was a problem hiding this comment.
good point bot, should we reuse?
| // SDK-internal ("core") field names that should not be exposed as custom fields. | ||
| // Fields prefixed with "_" are also considered core fields. | ||
| // See https://docs.bitdrift.io/sdk/features-fields.html#custom-fields | ||
| const CORE_FIELDS: &[&str] = &[ |
There was a problem hiding this comment.
I don't think all of these are fields like config_version, device_id and session_id so not sure where this list came from? Why would we not expose stuff like network type and foreground via this callback?
There was a problem hiding this comment.
the idea is to expose the custom fields that customer added only
What
TBF
Tech spec doc
Verification
Wired in this PR bitdriftlabs/capture-sdk#904