feat(libdd-trace-utils): check for empty value in header datadog-client-computed-stats#1900
Conversation
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
duncanpharvey
force-pushed
the
duncan-harvey/datadog-client-computed-stats
branch
from
0496440 to
17c3f1d
Compare
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: b2bcec9 | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1900 +/- ##
==========================================
- Coverage 71.74% 71.68% -0.07%
==========================================
Files 434 434
Lines 69940 69954 +14
==========================================
- Hits 50181 50147 -34
- Misses 19759 19807 +48
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
duncanpharvey
requested review from
a team,
kathiehuang and
lucaspimentel
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR updates libdd-trace-utils header parsing so that an explicitly empty Datadog-Client-Computed-Stats header value no longer disables agent-computed stats, enabling compatibility with tracers that send the header with an empty string (notably Java).
Changes:
- Treat
datadog-client-computed-stats: ""as falsy when converting fromHeaderMaptoTracerHeaderTags. - Add unit tests covering empty-string and absent-header cases for
datadog-client-computed-stats.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if headers.get("datadog-client-computed-top-level").is_some() { | ||
| tags.client_computed_top_level = true; |
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| fn is_header_empty(value: &str) -> bool { |
There was a problem hiding this comment.
Nitpick: I'm not sure it's worth making a bespoke function to just proxy str::is_empty
There was a problem hiding this comment.
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
| 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" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nitpick: we're basically testing the implementation of std::str::is_empty here. I'm a bit skeptical about the value of doing this.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Tests failed on this commit 5345da5: What to do next?
|
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
duncan-harvey/datadog-client-computed-stats
branch
What does this PR do?
Treat an empty string as a falsy value for the
Datadog-Client-Computed-Statsheader.Motivation
We want to support agent computed stats in the Serverless Compatibility Layer. Currently when the
Datadog-Client-Computed-Statsheader is sent it always disables agent computed stats, even when the value of the header is an empty string.https://datadoghq.atlassian.net/browse/SVLS-8789
Additional Notes
Datadog-Client-Computed-Statsheader where other tracers omit the header altogetherHow to test the change?
Added a debug log in a test build:
Before change with
DD_TRACE_STATS_COMPUTATION_ENABLED=falseAfter change with
DD_TRACE_STATS_COMPUTATION_ENABLED=false