feat(sidecar): enable telemetry for stats#2175
Open
VianneyRuhlmann wants to merge 1 commit into
Open
Conversation
This was referenced Jun 29, 2026
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
📚 Documentation Check Results📦
|
Contributor
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. |
Contributor
🔒 Cargo Deny Results📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: fd8554e | Docs | Datadog PR Page | Give us feedback! |
Contributor
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
|
VianneyRuhlmann
force-pushed
the
06-24-feat_trace_exporter_enable_telemetry_in_stats_exporter
branch
from
8fcfab9 to
94ad5e8
Compare
VianneyRuhlmann
force-pushed
the
06-29-feat_sidecar_enable_telemetry_for_stats
branch
from
1f87e19 to
4c38bb8
Compare
VianneyRuhlmann
force-pushed
the
06-29-feat_sidecar_enable_telemetry_for_stats
branch
from
4c38bb8 to
6413c5f
Compare
VianneyRuhlmann
force-pushed
the
06-24-feat_trace_exporter_enable_telemetry_in_stats_exporter
branch
from
94ad5e8 to
5706ea9
Compare
![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: 4c38bb8b78
ℹ️ 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".
Comment on lines
+298
to
+300
| let instance_id = InstanceId { | ||
| session_id: runtime_id.to_owned(), | ||
| runtime_id: runtime_id.to_owned(), |
There was a problem hiding this comment.
When the first IPC fallback span for a service/env arrives before any telemetry client exists, add_span_to_concentrator passes the connection session id as this runtime_id, so this block creates a cached telemetry client whose InstanceId.runtime_id is also the session id. TelemetryCachedClient::new copies that field into builder.runtime_id, and the client is then reused by service/env, so subsequent normal telemetry for the app reports the wrong runtime id. Avoid creating a new telemetry client from this fallback without a real InstanceId, or pass the actual runtime id through the IPC path.
Useful? React with 👍 / 👎.
| .map(|s| { | ||
| ( | ||
| make_exporter(s, s.endpoint.clone(), Duration::from_secs(10)), | ||
| make_exporter(s, s.endpoint.clone(), Duration::from_secs(10), None), |
There was a problem hiding this comment.
When ddog_sidecar_flush drains stats via flush(... traces_and_stats: true ...) before the periodic loop runs, this exporter is built with None, so StatsExporter::send cannot emit the datadog.tracer.stats.collapsed_spans telemetry for collapsed spans in that flush. The buckets are drained by this send, so the periodic exporter that does have a worker handle never gets another chance to report them; the flush path should use the concentrator's telemetry worker as well.
Useful? React with 👍 / 👎.
Comment on lines
+154
to
+158
|
|
||
| let exporter = make_exporter( | ||
| &state, | ||
| state.endpoint.clone(), | ||
| flush_interval, |
There was a problem hiding this comment.
This binds the flush loop to the telemetry worker that existed when the concentrator was created, but telemetry clients are removed on LifecycleAction::Stop while SHM concentrators are global and can remain active until an idle flush removes them. If another runtime for the same service/env continues using the existing concentrator after a Stop/start cycle, get_or_create_concentrator returns the old state and collapsed-span points keep going to the stopped worker instead of the newly created telemetry client; look up or refresh the worker when flushing rather than capturing it for the concentrator lifetime.
Useful? React with 👍 / 👎.
Comment on lines
+154
to
+158
|
|
||
| let exporter = make_exporter( | ||
| &state, | ||
| state.endpoint.clone(), | ||
| flush_interval, |
There was a problem hiding this comment.
When one service/env has multiple active stats concentrators, such as different versions because the concentrator key includes version, each per-concentrator exporter registers a fresh datadog.tracer.stats.collapsed_spans context on the same telemetry worker. The telemetry buckets aggregate by ContextKey, not by metric name/tags, so collapsed-span counts from those exporters are flushed as separate identical series instead of a single count; cache the metric context per worker and share it across exporters.
Useful? React with 👍 / 👎.
bwoebi
reviewed
Jun 30, 2026
Comment on lines
+290
to
+296
| let trace_config = session.get_trace_config(); | ||
| let runtime_metadata = RuntimeMetadata::new( | ||
| trace_config.language.clone(), | ||
| trace_config.language_version.clone(), | ||
| trace_config.tracer_version.clone(), | ||
| ); | ||
| drop(trace_config); |
Contributor
There was a problem hiding this comment.
Suggested change
| let trace_config = session.get_trace_config(); | |
| let runtime_metadata = RuntimeMetadata::new( | |
| trace_config.language.clone(), | |
| trace_config.language_version.clone(), | |
| trace_config.tracer_version.clone(), | |
| ); | |
| drop(trace_config); | |
| let runtime_metadata = { | |
| let trace_config = session.get_trace_config(); | |
| RuntimeMetadata::new( | |
| trace_config.language.clone(), | |
| trace_config.language_version.clone(), | |
| trace_config.tracer_version.clone(), | |
| ) | |
| }; |
bwoebi
approved these changes
Jun 30, 2026
VianneyRuhlmann
force-pushed
the
06-24-feat_trace_exporter_enable_telemetry_in_stats_exporter
branch
from
5706ea9 to
dae98bf
Compare
VianneyRuhlmann
force-pushed
the
06-29-feat_sidecar_enable_telemetry_for_stats
branch
from
6413c5f to
9d522ee
Compare
Eldolfin
reviewed
Jun 30, 2026
Comment on lines
+308
to
+317
| || {session | ||
| .session_config | ||
| .lock_or_panic() | ||
| .as_ref() | ||
| .cloned() | ||
| .unwrap_or_else(|| { | ||
| warn!("Session telemetry config unavailable for env={env} version={version} service={service_name}; telemetry disabled in stats"); | ||
| Config::default() | ||
| }) | ||
| }, |
Contributor
There was a problem hiding this comment.
Suggested change
| || {session | |
| .session_config | |
| .lock_or_panic() | |
| .as_ref() | |
| .cloned() | |
| .unwrap_or_else(|| { | |
| warn!("Session telemetry config unavailable for env={env} version={version} service={service_name}; telemetry disabled in stats"); | |
| Config::default() | |
| }) | |
| }, | |
| || { | |
| session | |
| .session_config | |
| .lock_or_panic() | |
| .as_ref() | |
| .cloned() | |
| .unwrap_or_else(|| { | |
| warn!( | |
| "Session telemetry config unavailable for env={env} \ | |
| version={version} service={service_name}; telemetry \ | |
| disabled in stats" | |
| ); | |
| Config::default() | |
| }) | |
| }, |
Contributor
There was a problem hiding this comment.
I was wondering why cargo fmt didn't format this part and according to claude:
when call args contain complex closures and rustfmt can't find valid layout within width, it falls back to leaving the expression untouched rather than producing malformed output.
with that format, cargo fmt works again.
(btw I checked and escaped newlines in strings are also un-indented so the log still looks like before)
VianneyRuhlmann
force-pushed
the
06-29-feat_sidecar_enable_telemetry_for_stats
branch
from
9d522ee to
fd8554e
Compare
VianneyRuhlmann
force-pushed
the
06-24-feat_trace_exporter_enable_telemetry_in_stats_exporter
branch
from
dae98bf to
3a03e6a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Enable span concentrator telemetry in sidecar.