Optimise OTEL metrics collection to reduce allocations#8834
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6127818c1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (spanFormattable.TryFormat(buffer, out var written, format: default, provider: CultureInfo.InvariantCulture)) | ||
| { | ||
| return HashChars(buffer.Slice(0, written), hash); |
There was a problem hiding this comment.
Keep tag hashing consistent with exported values
When the process runs under a culture whose numeric formatting differs from invariant culture, this makes semantically different exported tag values share one MetricPoint. For example, with CurrentCulture = de-DE, a tag value 3.14d hashes as "3.14" here, so it collides with the string tag "3.14"; however OtlpMetricsSerializer still exports metric tags with tag.Value?.ToString(), so the double would be emitted as "3,14" while the string is emitted as "3.14". Because MetricState now keys only by this hash and keeps the first tag dictionary, one of those time series is aggregated under the other's attributes instead of producing a separate data point.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@zacharycmontoya @link04 I explicitly switched to using culture-invariant exporting here - I'm wondering if we should do the same in OtlpMetricsSerializer, or whether we should keep the culture-dependant behaviour instead? WDYT?
There was a problem hiding this comment.
I think the choice to do culture-invariant is fine for now. Eventually, we should mimic the behavior of opentelemetry-dotnet to emit tags that keep their original primitive type (a tag with an integer value is emitted as an OTLPIntValue) but for now we just need to be consistent between this key cache and the emitted key output.
| var hash = MetricTagsHash.Compute(tags); | ||
|
|
||
| if (_points.TryGetValue(tagSet, out var existingPoint)) | ||
| if (_points.TryGetValue(hash, out var existingPoint)) |
There was a problem hiding this comment.
Resolve hash collisions before reusing metric points
When two different metric attribute sets produce the same 64-bit FNV value, this lookup reuses the existing MetricPoint without comparing the actual tags, so subsequent measurements are aggregated under the first point's tag dictionary and the other time series is never exported separately. Because metric attribute keys and values can be arbitrary application strings, keep enough tag identity to disambiguate collisions (for example a collision bucket or equality check) instead of treating the hash as the sole identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I thought about this before, but wasn't sure if it was necessary tbh? 🤔 The chances of collisions are very low. With 2000 metric point, the chance of collision is ~1 in 9 trillion 😅 It doesn't seem worth worrying about that to me
b612781 to
36baa14
Compare
BenchmarksBenchmark execution time: 2026-07-01 10:15:51 Comparing candidate commit c5646d2 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 62 known flaky benchmarks, 64 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8834) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (73ms) : 69, 76
master - mean (70ms) : 67, 73
section Bailout
This PR (8834) - mean (76ms) : 72, 79
master - mean (74ms) : 73, 75
section CallTarget+Inlining+NGEN
This PR (8834) - mean (1,079ms) : 1031, 1128
master - mean (1,085ms) : 1022, 1148
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (113ms) : 106, 119
master - mean (109ms) : 105, 113
section Bailout
This PR (8834) - mean (111ms) : 108, 114
master - mean (110ms) : 108, 112
section CallTarget+Inlining+NGEN
This PR (8834) - mean (775ms) : 748, 803
master - mean (780ms) : 760, 801
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (97ms) : 93, 100
master - mean (97ms) : 94, 100
section Bailout
This PR (8834) - mean (101ms) : 96, 106
master - mean (97ms) : 96, 99
section CallTarget+Inlining+NGEN
This PR (8834) - mean (939ms) : 895, 982
master - mean (938ms) : 902, 974
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (95ms) : 90, 101
master - mean (98ms) : 92, 104
section Bailout
This PR (8834) - mean (95ms) : 93, 97
master - mean (97ms) : 94, 101
section CallTarget+Inlining+NGEN
This PR (8834) - mean (812ms) : 772, 852
master - mean (813ms) : 776, 850
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (201ms) : 196, 205
master - mean (202ms) : 197, 207
section Bailout
This PR (8834) - mean (204ms) : 201, 207
master - mean (207ms) : 203, 210
section CallTarget+Inlining+NGEN
This PR (8834) - mean (1,200ms) : 1156, 1243
master - mean (1,206ms) : 1160, 1253
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (289ms) : 283, 294
master - mean (290ms) : 283, 297
section Bailout
This PR (8834) - mean (289ms) : 285, 294
master - mean (290ms) : 285, 294
section CallTarget+Inlining+NGEN
This PR (8834) - mean (972ms) : 948, 997
master - mean (969ms) : 950, 989
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (283ms) : 277, 290
master - mean (283ms) : 276, 290
section Bailout
This PR (8834) - mean (283ms) : 278, 288
master - mean (283ms) : 279, 288
section CallTarget+Inlining+NGEN
This PR (8834) - mean (1,173ms) : 1133, 1214
master - mean (1,163ms) : 1129, 1196
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8834) - mean (282ms) : 275, 290
master - mean (280ms) : 275, 286
section Bailout
This PR (8834) - mean (282ms) : 277, 288
master - mean (281ms) : 275, 286
section CallTarget+Inlining+NGEN
This PR (8834) - mean (1,046ms) : 1000, 1091
master - mean (1,038ms) : 998, 1078
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
zacharycmontoya
left a comment
There was a problem hiding this comment.
LGTM, though I've noted an improvement that we need to make at a later time when emitting attributes on OTLP metrics
36baa14 to
1bfb0cb
Compare
…sts.cs Co-authored-by: Zach Montoya <[email protected]>
## Summary of changes Add configuration for controlling cardinality of OTEL metrics ## Reason for change Currently we have unbounded cardinality of OTEL metrics collected using the `System.Diagnostics.Metrics` namespace. However, this can have implications for memory use, and in accordance [to the spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#cardinality-limits), this PR adds a default 200 point limit, which can be overridden by `DD_METRICS_OTEL_MAX_CARDINALITY`. ## Implementation details - Add `DD_METRICS_OTEL_MAX_CARDINALITY` with a default value of 2000 - Limit the maximum cardinality of points per-meter - Additional points received after exceeding the limit are stored in an [overflow point](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#overflow-attribute) - The cardinality is for the lifetime of the app; it doesn't reset each flush period ## Test coverage Added a bunch of unit tests to demonstrate the behaviour ## Other details Designed to be used in conjunction with - #8834 Addresses - https://datadoghq.atlassian.net/browse/APMSP-3500 - https://datadoghq.atlassian.net/browse/APMSP-3503 --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Summary of changes
Improve the performance of OTEL metrics collection
Reason for change
We need to add constraints to the cardinality of OTEL metrics collection. As part of that work, noticed some easy performance wins:
GetOrCreatePoint()As this is all .NET 6+, we can use the more efficient APIs without worrying about earlier TFMs for a change 🎉
Implementation details
ConcurrentDictionary<>that takes astateparameter to eliminate the closureTagSet, and instead use aulonghash as the keyStringBuilderstringbytes directly using our FNV hash typeTest coverage
This is just a perf improvement, so tests should be unchanged. Wrote a small benchmark to validate the changes. The following are the .NET 6.0 results, .NET 10 was similar.
Given we're talking about a 4us worst case slow down for 30x100 char tags, but with a redution in 18KB allocation, I think this is preferable
Benchmark Details
Other details
Prereq for cardinality limiting of OTel metrics