feat(stats)!: add endpoint gating to client-side stats [APMSP-3361]#2040
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 7aa8cd9 | Docs | Datadog PR Page | Give us feedback! |
22670c0 to
b607a25
Compare
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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2040 +/- ##
=======================================
Coverage 73.44% 73.45%
=======================================
Files 465 465
Lines 77949 77982 +33
=======================================
+ Hits 57248 57278 +30
- Misses 20701 20704 +3
🚀 New features to boost your workflow:
|
VianneyRuhlmann
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b607a2507d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .info | ||
| .version | ||
| .as_ref() | ||
| .is_some_and(|v| (v.major, v.minor, v.patch) >= MIN_STATS_AGENT_VERSION) |
There was a problem hiding this comment.
When the agent reports a prerelease such as 7.65.0-rc.1, the parser preserves that suffix in metadata, but this comparison only checks (major, minor, patch), so the prerelease is treated as satisfying the >= 7.65.0 gate. Under semver precedence 7.65.0-rc.1 is lower than 7.65.0, so in environments running RC/dev agents that also advertise /v0.6/stats, client-side stats can be enabled before the minimum supported stable agent version.
Useful? React with 👍 / 👎.
|
/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.
[email protected] unqueued this merge request |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: waiting
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let Some(raw) = Option::<String>::deserialize(deserializer)? else { |
There was a problem hiding this comment.
Small detail but this implies agent version is always a string, because otherwise the ? here is the only place we will return an Err and thus make the whole schema un-parsable.
In a more general note, I don't like how any parse failure in a field on the schema fails the deserialization of the whole struct because it makes for some really unclear errors. I don't have a solution tho :/
There was a problem hiding this comment.
The version is always a string https://github.com/DataDog/datadog-agent/blob/8f7d7790726510ee1ecd8e6b7d57cc9c406691bf/pkg/trace/api/info.go#L25 . I don't think we want to partially parse the version here, either it's a valid semver version and we can't rely on it to check for feature support or it's not and we shouldn't try to rely on it
There was a problem hiding this comment.
One important note here is that this version can also come from the serverless rust agent, which today is also a string, but greatly complicates the usage of version checking :(
There was a problem hiding this comment.
e.g. today the rust agent doesn't return a version at all
There was a problem hiding this comment.
I guess it's ok for now to only expect agent version then. They will need a way to let the tracers know which "flavor" of agent they're talking to anyway.
There was a problem hiding this comment.
The DD_TRACE_STATS_COMPUTATION_ENABLED config was always dependent on the agent advertising support for CSS. Previously support was only checked by the presence of drop_p0s=true in /info however according to the spec we also need the a supported agent version >=7.65 and the /v0.6/stats endpoint to be advertised. I don't think it's a good idea to allow unconditional CSS to be configured
There was a problem hiding this comment.
(edit: I posted this comment before refreshing, didn't see the comment above)
I suppose the rust agents can fake it and return version 7.65, but that's even hackier than gating features by using version checks instead of feature detection. 😅
There was a problem hiding this comment.
The
DD_TRACE_STATS_COMPUTATION_ENABLEDconfig was always dependent on the agent advertising support for CSS.
Exactly. Feature detection vs version checks. The whole point of adding features to /info was to move away from version checks. Why are we going back? 😞
I don't think it's a good idea to allow unconditional CSS to be configured
I agree. I didn't mean unconditional. I meant skip only the new version check (which is arguably a breaking change for users who already had CSS working with DD_TRACE_STATS_COMPUTATION_ENABLED=true), not the check for drop_p0s=true.
There was a problem hiding this comment.
I'm fine with removing the agent version and relying only on client_drop_p0s and the endpoint being available. Also I think the rust agent currently hardcodes client_drop_p0s to be disabled which means CSS won't be enabled regardless of DD_TRACE_STATS_COMPUTATION_ENABLED
There was a problem hiding this comment.
I think the rust agent currently hardcodes
client_drop_p0sto be disabled
Yes! We had to fix that bug recently because that rust agent (there are two rust agents!) was lying to the tracers, announcing CSS support. For now, we can't enable CSS in AWS Lambda at all, but some tracers started enabling CSS by default.
The workloads that enable CSS use the other rust agent (https://github.com/DataDog/serverless-components/tree/main/crates/datadog-serverless-compat), which reports support for client_drop_p0s correctly.
Happy to take this thread to Slack if this is getting too long 😅
VianneyRuhlmann
changed the title
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
|
| major: 7, | ||
| minor: 65, | ||
| patch: 0, | ||
| metadata: None, |
There was a problem hiding this comment.
I don't think you are testing metadata anywhere?
c3f4fda to
0e8a981
Compare
0e8a981 to
7aa8cd9
Compare
VianneyRuhlmann
changed the title
cde8f3a
into
main
…ibdd-data-pipeline, libdd-li... (#2201) # Release proposal for libdd-capabilities-impl, libdd-common, libdd-data-pipeline, libdd-library-config, libdd-remote-config, libdd-sampling, libdd-telemetry, libdd-tinybytes, libdd-trace-utils and their dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-capabilities **Next version:** `2.1.0` **Semver bump:** `minor` **Tag:** `libdd-capabilities-v2.1.0` ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-common **Next version:** `5.1.0` **Semver bump:** `minor` **Tag:** `libdd-common-v5.1.0` ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - fix: update rustls-webpki to 0.103.13 (#2187) - fix: update anyhow for unsoundness (#2186) - feat(machine id): Add helpers in ddcommon to fetch the machine UUID l… (#2163) ## libdd-ddsketch **Next version:** `1.1.0` **Semver bump:** `minor` **Tag:** `libdd-ddsketch-v1.1.0` ### Commits - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(ddsketch): add microbenchmarks for add/encode/collapse (#2125) ## libdd-trace-protobuf **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-protobuf-v4.0.0` ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: add whole key cardinality limit (#2158) - feat(remote-config)!: use the proto file from the agent (#2165) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) ## libdd-capabilities-impl **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-capabilities-impl-v3.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-library-config **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-library-config-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - feat(library-config)!: caller-supplied threadlocal schema and extra process-context attributes (#2162) - fix(otel-thread-ctx): put the threadlocal attributes at the right place in the context (#2167) ## libdd-remote-config **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v2.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(libdd-remote-config)!: hide Target inner properties so they are not leaked (#2182) - feat(remote-config)!: use the proto file from the agent (#2165) - refactor(rc): reexport Endpoint and Tag common types (#2147) ## libdd-trace-normalization **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-normalization-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.1 → ^4.0.0 ### Commits - feat(data-pipeline)!: CSS Trace Filters (#1985) ## libdd-shared-runtime **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-shared-runtime-v2.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) ## libdd-trace-utils **Next version:** `9.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-utils-v9.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(data-pipeline): add agentless export (#2081) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - test(trace-utils): add V05 msgpack decode microbenchmark (#2127) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(trace-utils): add VecMap microbenchmarks (#2126) - chore(stats)!: submit p0 telemetry in stats (#2130) - refactor(change-buffer)!: replace slot index with span_id, fix segment isolation (#2105) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(trace-exporter): add v1 span and its encoder (#2039) - fix(trace-utils): mark decoded span maps as deduped (#2110) - feat(trace-utils)!: change buffer implementation (#2055) - feat(native-spans)!: change buffer foundation (#2046) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - test: fix timeouts on heavily contended scenarios (#2093) ## libdd-telemetry **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-telemetry-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) ## libdd-trace-obfuscation **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-obfuscation-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - ci(miri): skip slow miri tests (#2188) - fix: update anyhow for unsoundness (#2186) ## libdd-trace-stats **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-stats-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) ## libdd-data-pipeline **Next version:** `7.0.0` **Semver bump:** `major` **Tag:** `libdd-data-pipeline-v7.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-telemetry`: ^5.0.1 → ^6.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-stats`: ^5.0.0 → ^6.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - feat(trace_exporter): enable telemetry in stats exporter (#2160) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(stats): emit canonical gRPC status name for OTLP rpc.response.status_code (#2183) - feat(data-pipeline): add agentless export (#2081) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - chore(stats)!: submit p0 telemetry in stats (#2130) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - feat(stats)!: add endpoint gating to client-side stats [APMSP-3361] (#2040) ## libdd-dogstatsd-client **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-dogstatsd-client-v4.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ## libdd-sampling **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-sampling-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 [APMSP-3371]: https://datadoghq.atlassian.net/browse/APMSP-3371?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <[email protected]>
7 participants
What does this PR do?
Check for the stats endpoint to enable CSS.
Edit: The agent version check was removed due to compatibility with serverless agent.
Motivation
According to spec client-side stats should only be enabled when agent version is higher than. 7.65 and the stats endpoint is available.
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.