Skip to content

[crashtracking] Make siginfo an optional field for CrashPing #1326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dd-mergequeue merged 1 commit into from
Nov 10, 2025
Merged

[crashtracking] Make siginfo an optional field for CrashPing #1326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions datadog-crashtracker/src/crash_info/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,19 +395,30 @@ impl CrashInfoBuilder {
Ok(self)
}

/// This method requires that the builder has a UUID, siginfo, and metadata set
/// This method requires that the builder has a UUID and metadata set.
/// Siginfo is optional for platforms that don't support it (like Windows)
pub fn build_crash_ping(&self) -> anyhow::Result<CrashPing> {
let sig_info = self.sig_info.clone().context("sig_info is required")?;
let sig_info = self.sig_info.clone();
let metadata = self.metadata.clone().context("metadata is required")?;

CrashPingBuilder::new(self.uuid)
.with_sig_info(sig_info)
.with_metadata(metadata)
.build()
let mut builder = CrashPingBuilder::new(self.uuid).with_metadata(metadata);
if let Some(sig_info) = sig_info {
builder = builder.with_sig_info(sig_info);
}
builder.build()
}

pub fn is_ping_ready(&self) -> bool {
self.sig_info.is_some() && self.metadata.is_some()
// On Unix platforms, wait for both metadata and siginfo
// On Windows, siginfo is not available, so only wait for metadata
#[cfg(unix)]
{
self.metadata.is_some() && self.sig_info.is_some()
}
#[cfg(windows)]
{
self.metadata.is_some()
}
}
}

Expand All @@ -430,7 +441,7 @@ mod tests {

assert!(!crash_ping.crash_uuid().is_empty());
assert!(Uuid::parse_str(crash_ping.crash_uuid()).is_ok());
assert_eq!(crash_ping.siginfo(), &sig_info);
assert_eq!(crash_ping.siginfo(), Some(&sig_info));
assert_eq!(crash_ping.metadata(), &metadata);
assert!(crash_ping.message().contains("crash processing started"));
}
Expand Down
60 changes: 38 additions & 22 deletions datadog-crashtracker/src/crash_info/telemetry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,18 @@ impl CrashPingBuilder {

pub fn build(self) -> anyhow::Result<CrashPing> {
let crash_uuid = self.crash_uuid;
let sig_info = self.sig_info.context("sig_info is required")?;
let sig_info = self.sig_info;
let metadata = self.metadata.context("metadata is required")?;

let message = self.custom_message.unwrap_or_else(|| {
format!(
"Crashtracker crash ping: crash processing started - Process terminated with {:?} ({:?})",
sig_info.si_code_human_readable, sig_info.si_signo_human_readable
)
if let Some(ref sig_info) = sig_info {
format!(
"Crashtracker crash ping: crash processing started - Process terminated with {:?} ({:?})",
sig_info.si_code_human_readable, sig_info.si_signo_human_readable
)
} else {
"Crashtracker crash ping: crash processing started - Process terminated".to_string()
}
Comment on lines 61 to +74
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Include "unknown signal" in crash ping default message

When sig_info is absent the builder falls back to "Crashtracker crash ping: crash processing started - Process terminated" and never mentions that the signal is unknown. However, test_crash_ping_builder_validation now asserts crash_ping.message().contains("unknown signal"), so the new test will always fail and Windows crash pings will be logged without an explicit indication that no signal data was available. The else branch should produce a message that contains "unknown signal" (or the test expectation should be adjusted).

Useful? React with 👍 / 👎.

Copy link
Contributor Author

@gyuheon0h gyuheon0h Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused because your comment is for how the code was in the first commit, but the code snippet you show is the latest, correct version. Also I only asked you for a review after the latest version was pushed...

Bad AI!

});

Ok(CrashPing {
Expand All @@ -84,7 +88,8 @@ impl CrashPingBuilder {
#[derive(Debug, Serialize)]
pub struct CrashPing {
crash_uuid: String,
siginfo: SigInfo,
#[serde(skip_serializing_if = "Option::is_none")]
siginfo: Option<SigInfo>,
message: String,
version: String,
kind: String,
Expand All @@ -104,8 +109,8 @@ impl CrashPing {
&self.metadata
}

pub fn siginfo(&self) -> &SigInfo {
&self.siginfo
pub fn siginfo(&self) -> Option<&SigInfo> {
self.siginfo.as_ref()
}

fn current_schema_version() -> String {
Expand Down Expand Up @@ -237,20 +242,26 @@ impl TelemetryCrashUploader {
.await
}

fn build_crash_ping_tags(&self, crash_uuid: &str, sig_info: &SigInfo) -> String {
fn build_crash_ping_tags(&self, crash_uuid: &str, sig_info: Option<&SigInfo>) -> String {
let metadata = &self.metadata;
let mut tags = format!(
"uuid:{},is_crash_ping:true,service:{},language_name:{},language_version:{},tracer_version:{},si_code_human_readable:{:?},si_signo:{},si_signo_human_readable:{:?}",
"uuid:{},is_crash_ping:true,service:{},language_name:{},language_version:{},tracer_version:{}",
crash_uuid,
metadata.application.service_name,
metadata.application.language_name,
metadata.application.language_version,
metadata.application.tracer_version,
sig_info.si_code_human_readable,
sig_info.si_signo,
sig_info.si_signo_human_readable
metadata.application.tracer_version
);

if let Some(sig_info) = sig_info {
tags.push_str(&format!(
",si_code_human_readable:{:?},si_signo:{},si_signo_human_readable:{:?}",
sig_info.si_code_human_readable,
sig_info.si_signo,
sig_info.si_signo_human_readable
));
}

self.append_optional_tags(&mut tags);
tags
}
Expand Down Expand Up @@ -533,7 +544,7 @@ mod tests {
// Structured message format
let message_json: serde_json::Value =
serde_json::from_str(log_entry["message"].as_str().unwrap())?;
assert_eq!(message_json["siginfo"], serde_json::to_value(sig_info)?);
assert_eq!(message_json["siginfo"], serde_json::to_value(&sig_info)?);
assert!(message_json["crash_uuid"].is_string());
assert!(Uuid::parse_str(message_json["crash_uuid"].as_str().unwrap()).is_ok());

Expand Down Expand Up @@ -720,17 +731,20 @@ mod tests {
#[test]
#[cfg_attr(miri, ignore)]
fn test_crash_ping_builder_validation() {
// Test that crash ping can be built with only metadata (no sig_info)
let mut crash_info_builder = CrashInfoBuilder::new();
crash_info_builder
.with_metadata(Metadata::test_instance(1))
.unwrap();
let result = crash_info_builder.build_crash_ping();
assert!(result.is_err());
assert!(result
.unwrap_err()
.to_string()
.contains("sig_info is required"));
assert!(result.is_ok());
let crash_ping = result.unwrap();
assert!(crash_ping.siginfo().is_none());
assert!(crash_ping
.message()
.contains("Crashtracker crash ping: crash processing started - Process terminated"));

// Test that crash ping fails without metadata
let mut crash_info_builder = CrashInfoBuilder::new();
crash_info_builder
.with_sig_info(crate::SigInfo::test_instance(1))
Expand All @@ -742,7 +756,7 @@ mod tests {
.to_string()
.contains("metadata is required"));

// Test successful build with both required fields
// Test successful build with both fields
let mut crash_info_builder = CrashInfoBuilder::new();
crash_info_builder
.with_sig_info(crate::SigInfo::test_instance(1))
Expand All @@ -752,6 +766,8 @@ mod tests {
.unwrap();
let result = crash_info_builder.build_crash_ping();
assert!(result.is_ok());
let crash_ping = result.unwrap();
assert!(crash_ping.siginfo().is_some());
}

#[test]
Expand All @@ -770,7 +786,7 @@ mod tests {
assert!(Uuid::parse_str(crash_ping.crash_uuid()).is_ok());
assert!(crash_ping.message().contains("crash processing started"));
assert_eq!(crash_ping.metadata(), &metadata);
assert_eq!(crash_ping.siginfo(), &sig_info);
assert_eq!(crash_ping.siginfo(), Some(&sig_info));
}

#[tokio::test]
Expand Down
4 changes: 2 additions & 2 deletions datadog-crashtracker/src/receiver/receive_report.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,8 @@ pub(crate) async fn receive_report_from_stream(

//TODO: This assumes that the input is valid UTF-8.
loop {
// We need to wait until at least we receive config, metadata, and siginfo before sending
// the crash ping
// We need to wait until at least we receive config, metadata, and siginfo (on non-Windows
// platforms) before sending the crash ping
if !crash_ping_sent && builder.is_ping_ready() {
if let Some(ref config_ref) = config {
let config_clone = config_ref.clone();
Expand Down
12 changes: 9 additions & 3 deletions examples/ffi/crashinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,12 @@ int main(void) {
check_result(ddog_crasht_CrashInfoBuilder_with_os_info_this_machine(builder.get()),
"Failed to set os_info");

// Test uploading a crash ping without siginfo
auto ping_endpoint = ddog_endpoint_from_filename(to_slice_c_char("/tmp/crash_ping_test"));
check_result(ddog_crasht_CrashInfoBuilder_upload_ping_to_endpoint(builder.get(), ping_endpoint),
"Failed to upload crash ping");
ddog_endpoint_drop(ping_endpoint);

auto sigInfo = ddog_crasht_SigInfo {
.addr = "0xBABEF00D",
.code = 16,
Expand All @@ -252,10 +258,10 @@ int main(void) {
check_result(ddog_crasht_CrashInfoBuilder_with_sig_info(builder.get(), sigInfo),
"failed to add signal info");

auto ping_endpoint = ddog_endpoint_from_filename(to_slice_c_char("/tmp/crash_ping_test"));
check_result(ddog_crasht_CrashInfoBuilder_upload_ping_to_endpoint(builder.get(), ping_endpoint),
auto ping_endpoint2 = ddog_endpoint_from_filename(to_slice_c_char("/tmp/crash_ping_test"));
check_result(ddog_crasht_CrashInfoBuilder_upload_ping_to_endpoint(builder.get(), ping_endpoint2),
"Failed to upload crash ping");
ddog_endpoint_drop(ping_endpoint);
ddog_endpoint_drop(ping_endpoint2);

auto crashinfo = extract_result(ddog_crasht_CrashInfoBuilder_build(builder.release()),
"failed to build CrashInfo");
Expand Down
Loading