Skip to content

feat(libdd-trace-utils): check for empty value in header datadog-client-computed-stats #1900

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
gh-worker-dd-mergequeue-cf854d merged 4 commits into from
Apr 22, 2026
Merged

feat(libdd-trace-utils): check for empty value in header datadog-client-computed-stats #1900

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17c3f1d
check for empty value in header datadog-client-computed-stats
duncanpharvey Apr 20, 2026
dd9f5ac
remove is_header_empty
duncanpharvey Apr 22, 2026
9da57c8
update struct doc for TracerHeaderTags
duncanpharvey Apr 22, 2026
b2bcec9
Merge branch 'main' into duncan-harvey/datadog-client-computed-stats
duncanpharvey Apr 22, 2026
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
44 changes: 42 additions & 2 deletions libdd-trace-utils/src/tracer_header_tags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,8 @@ impl<'a> From<&'a HeaderMap<HeaderValue>> for TracerHeaderTags<'a> {
if headers.get("datadog-client-computed-top-level").is_some() {
tags.client_computed_top_level = true;
Comment on lines 133 to 134

Copilot AI Apr 21, 2026

Copy link

Choose a reason for hiding this comment

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

datadog-client-computed-top-level is still treated as enabled solely by header presence (is_some()), so an explicitly empty header value (""), if sent, will set client_computed_top_level=true. This conflicts with the struct doc comment (“Any non-empty value will mean 'yes'”) and now differs from the datadog-client-computed-stats handling in the next lines. Consider parsing the value and requiring it to be non-empty (similar to client_computed_stats) to make the semantics consistent.

Suggested change
if headers.get("datadog-client-computed-top-level").is_some() {
tags.client_computed_top_level = true;
if let Some(v) = headers.get("datadog-client-computed-top-level") {
tags.client_computed_top_level = !is_header_empty(v.to_str().unwrap_or_default());

Copilot uses AI. Check for mistakes.

@duncanpharvey duncanpharvey Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The datadog-client-computed-top-level header is outside the scope of this change. Will leave this header handling as is until the need arises to change it.

@yannham yannham Apr 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we still at least fix the struct doc (potentially in a separate PR? If the Codex assessment is true, contradicting doc and implementation can be confusing.

@duncanpharvey duncanpharvey Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the TracerHeaderTags struct doc to accurately reflect the behavior of these fields:

libdatadog/libdd-trace-utils/src/tracer_header_tags.rs

Lines 30 to 35 in 9da57c8

// specifies that the client has marked top-level spans, when set. If the header is present
// this value will resolve to 'true'
pub client_computed_top_level: bool,
// specifies whether the client has computed stats so that the agent doesn't have to. If the
// header is present and is non-empty this value will resolve to 'true'
pub client_computed_stats: bool,

}
if headers.get("datadog-client-computed-stats").is_some() {
tags.client_computed_stats = true;
if let Some(v) = headers.get("datadog-client-computed-stats") {
tags.client_computed_stats = !is_header_empty(v.to_str().unwrap_or_default());
}
if let Some(count) = headers.get("datadog-client-dropped-p0-traces") {
tags.dropped_p0_traces = count
Expand All @@ -150,6 +150,10 @@ impl<'a> From<&'a HeaderMap<HeaderValue>> for TracerHeaderTags<'a> {
}
}

fn is_header_empty(value: &str) -> bool {

@yannham yannham Apr 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: I'm not sure it's worth making a bespoke function to just proxy str::is_empty

@duncanpharvey duncanpharvey Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough! I removed this function and check for the empty string directly:

libdatadog/libdd-trace-utils/src/tracer_header_tags.rs

Lines 136 to 138 in 9da57c8

if let Some(v) = headers.get("datadog-client-computed-stats") {
tags.client_computed_stats = !v.to_str().unwrap_or_default().is_empty();
}

value.is_empty()
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -262,4 +266,40 @@ mod tests {
assert_eq!(tags.dropped_p0_traces, 12);
assert_eq!(tags.dropped_p0_spans, 0);
}

#[test]
fn test_is_header_empty() {
// Empty string is true
assert!(is_header_empty(""));

// Truthy and arbitrary non-empty values are false
for val in &["1", "t", "T", "TRUE", "True", "true"] {
assert!(
!is_header_empty(val),
"expected is_header_empty({val:?}) to be false"
);
}
}

@yannham yannham Apr 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nitpick: we're basically testing the implementation of std::str::is_empty here. I'm a bit skeptical about the value of doing this.

@duncanpharvey duncanpharvey Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed this test, see #1900 (comment)


#[test]
fn test_header_map_to_tags_computed_stats_empty_string() {
let val = "";
let mut header_map = HeaderMap::new();
header_map.insert("datadog-client-computed-stats", val.parse().unwrap());
let tags: TracerHeaderTags = (&header_map).into();
assert!(
!tags.client_computed_stats,
"expected client_computed_stats=false for datadog-client-computed-stats header value {val:?}"
);
}

#[test]
fn test_header_map_to_tags_computed_stats_not_set() {
let header_map = HeaderMap::new();
let tags: TracerHeaderTags = (&header_map).into();
assert!(
!tags.client_computed_stats,
"expected client_computed_stats=false when datadog-client-computed-stats header is not set"
);
}
}
Loading