[Metrics] Fixing sync-over-async patterns in DogStatsD Client Fork#8265
Conversation
NamedPipeTransport.SendBuffer() called WriteAsync().Wait() which can deadlock under thread pool starvation. Replace with synchronous Write(). Add DisposeAsync() through the StatsD disposal chain (AsynchronousWorker -> StatsBufferize -> StatsdData -> DogStatsdService -> StatsdManager) so shutdown awaits worker completion instead of blocking with .Wait(). TracerManager.RunShutdownTasksAsync now awaits StatsdManager.DisposeAsync instead of fire-and-forget Task.Run disposal. Made-with: Cursor
BenchmarksBenchmark execution time: 2026-03-13 18:33:00 Comparing candidate commit cfaf58c in PR branch Found 8 performance improvements and 10 performance regressions! Performance is the same for 159 metrics, 15 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs net6.0
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.HttpClientBenchmark.SendAsync net6.0
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog net472
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.NLogBenchmark.EnrichedLog net6.0
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan netcoreapp3.1
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8265) 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 (8265) - mean (77ms) : 74, 79
master - mean (76ms) : 74, 79
section Bailout
This PR (8265) - mean (81ms) : 79, 84
master - mean (81ms) : 78, 83
section CallTarget+Inlining+NGEN
This PR (8265) - mean (1,112ms) : 1070, 1155
master - mean (1,114ms) : 1073, 1155
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 (8265) - mean (120ms) : 117, 123
master - mean (120ms) : 116, 124
section Bailout
This PR (8265) - mean (121ms) : 118, 124
master - mean (122ms) : 119, 126
section CallTarget+Inlining+NGEN
This PR (8265) - mean (775ms) : 755, 796
master - mean (775ms) : 751, 800
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8265) - mean (106ms) : 102, 110
master - mean (107ms) : 103, 110
section Bailout
This PR (8265) - mean (107ms) : 105, 109
master - mean (109ms) : 106, 111
section CallTarget+Inlining+NGEN
This PR (8265) - mean (739ms) : 701, 777
master - mean (732ms) : 704, 760
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8265) - mean (105ms) : 102, 107
master - mean (107ms) : 96, 118
section Bailout
This PR (8265) - mean (106ms) : 104, 109
master - mean (107ms) : 104, 109
section CallTarget+Inlining+NGEN
This PR (8265) - mean (676ms) : 654, 698
master - mean (673ms) : 654, 693
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 (8265) - mean (195ms) : 191, 199
master - mean (199ms) : 189, 210
section Bailout
This PR (8265) - mean (198ms) : 194, 203
master - mean (202ms) : 196, 208
section CallTarget+Inlining+NGEN
This PR (8265) - mean (1,156ms) : 1093, 1218
master - mean (1,166ms) : 1102, 1231
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 (8265) - mean (279ms) : 272, 286
master - mean (281ms) : 273, 290
section Bailout
This PR (8265) - mean (280ms) : 274, 285
master - mean (283ms) : 273, 293
section CallTarget+Inlining+NGEN
This PR (8265) - mean (906ms) : 873, 940
master - mean (913ms) : 882, 943
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8265) - mean (273ms) : 267, 279
master - mean (273ms) : 267, 279
section Bailout
This PR (8265) - mean (274ms) : 270, 279
master - mean (274ms) : 268, 281
section CallTarget+Inlining+NGEN
This PR (8265) - mean (917ms) : 893, 941
master - mean (921ms) : 895, 948
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8265) - mean (272ms) : 266, 278
master - mean (279ms) : 271, 286
section Bailout
This PR (8265) - mean (271ms) : 268, 275
master - mean (280ms) : 272, 288
section CallTarget+Inlining+NGEN
This PR (8265) - mean (825ms) : 801, 850
master - mean (833ms) : 810, 856
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4582aa43d
ℹ️ 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".
Move context.Cancel = true before the Interlocked early-return guard so repeated SIGTERM signals are also suppressed. Previously, only the first signal set Cancel; subsequent signals returned early without canceling, allowing the OS default handler to kill the process while shutdown hooks were still running. Each PosixSignalContext is a new instance per signal delivery, so Cancel must be set on every invocation to prevent termination. Made-with: Cursor
The previous reordering of context.Cancel was incorrect - the handler is unregistered inside RunShutdownTasks() anyway, making the reorder irrelevant. Reverting to original ordering. Made-with: Cursor
…g/dd-trace-dotnet into maximo/fix-statsd-sync-over-async
| /// Flushes all metrics without blocking on worker tasks. | ||
| /// </summary> | ||
| public void Dispose() | ||
| public async Task DisposeAsync() |
There was a problem hiding this comment.
Just to clarify, the vendored statsdclient code is already not idempotent right? i.e. if we'd re-run the vendoring tool prior to this PR, we would already have got different code. If so, then this change is probably fine, but it would be nice to have a follow up so that these changes are persisted across re-renderings 🤔 Not a blocker IMO though
| if (_statsdData is not null) | ||
| { | ||
| await _statsdData.DisposeAsync().ConfigureAwait(false); | ||
| _statsdData = null; | ||
| } |
There was a problem hiding this comment.
Super nit, not sure that it's really necessary but hey, why not be safe 😅
| if (_statsdData is not null) | |
| { | |
| await _statsdData.DisposeAsync().ConfigureAwait(false); | |
| _statsdData = null; | |
| } | |
| if (Interlocked.Exchange(ref _statsdData, null!) is { } statsd) | |
| { | |
| await statsd.DisposeAsync().ConfigureAwait(false); | |
| } |
There was a problem hiding this comment.
EDIT: looks like a bunch of other statsd dispose pattern is like this so meh
|
|
||
| tracerManager.DirectLogSubmission.Formatter.Tags.Should().Be("key2:value2"); | ||
|
|
||
| await tracerManager.ShutdownAsync(); |
There was a problem hiding this comment.
oooh, good catch on this, I wonder if we have anywhere else that's doing this 🤔
| { | ||
| var clientCount = 0; | ||
| using var manager = new StatsdManager(new TracerSettings(), (_, _, _) => | ||
| var manager = new StatsdManager(new TracerSettings(), (_, _, _) => |
There was a problem hiding this comment.
Ideally we should do this, as it ensures we cleanup even if the test fails. But I'm actually not sure if it works in all TFMs 😅
| var manager = new StatsdManager(new TracerSettings(), (_, _, _) => | |
| await using var manager = new StatsdManager(new TracerSettings(), (_, _, _) => |
There was a problem hiding this comment.
Looked into this and found that StatsdManager doesn't implement IAsyncDisposable so it wouldn't work on all TFMs, I can try adding a note to look in a follow up PR.
…kip TerminationSignalTests - TracerManager.RunShutdownTasksAsync: use fire-and-forget ContinueWith for StatsD disposal. On master the old sync Dispose() was already fire-and-forget internally via Task.Run(). DisposeAsync flushes buffers and drains worker threads which can take several seconds — longer than the window between repeated termination signals on .NET 10. Added explanatory comment. - StatsdManager.StatsdClientHolder.Dispose: added comment explaining why disposal is pushed to a background thread (flushes outstanding data). - DynamicInstrumentation.Dispose: fire-and-forget DisposeAsync replaces the old SafeDisposal.Add(_dogStats) which called sync Dispose(). Cannot await here because Dispose() is synchronous and callers hold locks. - Skip TerminationSignalTests: there are timing issues in the shutdown after adding DisposeAsync — the AsynchronousWorker drain race can exceed the gap between repeated SIGTERMs on .NET 10. Pre-existing issue in vendored StatsD. Made-with: Cursor
bouwkast
left a comment
There was a problem hiding this comment.
Thanks!
Just a couple of follow ups but they may have already been addressed/answered
| namespace Datadog.Trace.DogStatsd; | ||
|
|
||
| internal interface IStatsdManager : IDisposable | ||
| internal interface IStatsdManager |
There was a problem hiding this comment.
Just wondering why we don't implement IAsyncDisposable? Maybe a TFM issue (can't search it right now 😛)
Not a issue from what I see just curious
| @@ -3,13 +3,14 @@ | |||
| // This file was automatically generated by the UpdateVendors tool. | |||
There was a problem hiding this comment.
I think I saw it somewhere but we should probably do something in a followup PR that disables re-vendoring of this to make sure we don't break it
## Summary of changes Enable runtime metrics by default on .NET 6+ using the Diagnostics listener for services where the config is unset otherwise continue to use EventListener, and default existing explicit users to Diagnostics on .NET 8+ since there is no loss of ASP.NET Core Metrics. ## Reason for change Runtime metrics are currently opt-in. The EventListener/EventPipe implementation has known runtime bugs: shutdown crashes (dotnet/runtime#103480) and CPU/memory leaks (dotnet/runtime#111368). The Diagnostics listener avoids these and has comparable or better performance in our tests. See [RFC: Enabling .NET Runtime Metrics by Default — Option A](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.bs3q57sbzfh3). ## Implementation details Configuration logic for `DD_RUNTIME_METRICS_ENABLED`: - If explicitly set, use that value - If not set, default to `true` on .NET 6+ Configuration logic for `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED`: - If explicitly set, use that value - If not set: - .NET 8+: defaults to `true` (full metric coverage including ASP.NET Core meters) - .NET 6/7 with `DD_RUNTIME_METRICS_ENABLED` not set: defaults to `true` - .NET 6/7 with `DD_RUNTIME_METRICS_ENABLED=true`: defaults to `false` (keeps EventListener to preserve ASP.NET Core EventCounter metrics) The .NET 8 check uses `Environment.Version.Major >= 8`. Also fixes `RuntimeMetricsWriter` to dispose `DiagnosticsMetricsRuntimeMetricsListener` on .NET 6+ (`MeterListener` is safe to dispose, unlike `EventListener`). The runtime metrics collector selected depends on the runtime version and whether `DD_RUNTIME_METRICS_ENABLED` was explicitly configured: | Runtime | `DD_RUNTIME_METRICS_ENABLED` | Collector used | |---|---|---| | .NET 8+ | not set / invalid | Diagnostics (default) | | .NET 8+ | `true` | Diagnostics (default) | | .NET 6/7 | not set / invalid | Diagnostics (default) | | .NET 6/7 | `true` (explicit) | EventListener (preserves ASP.NET Core EventCounter metrics not yet available via Diagnostics on < .NET 8) | | Any | `false` | Disabled | | Any | `OTEL_METRICS_EXPORTER=none` | Disabled (takes precedence over the default) | **Note for .NET 6/7 users** who explicitly set `DD_RUNTIME_METRICS_ENABLED=true` and want to opt into the new Diagnostics-based collector (at the cost of losing ASP.NET Core EventCounter metrics): also set `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED=true`. ## Test coverage - Updated conditional `[InlineData]` in `RuntimeMetricsEnabled` test to expect `true` when unset on .NET 6+. - Added `RuntimeMetrics_DefaultsToDignosticsOnNet6Plus_WhenNotExplicitlySet`. - Added `RuntimeMetrics_ExplicitEnable_RespectsExplicitDiagnosticsFlag` (explicit `true`/`false` cases). - Added `RuntimeMetrics_ExplicitEnable_DefaultsToDiagnosticsOnNet8Plus` (net8.0 TFM). - Added `RuntimeMetrics_ExplicitEnable_DefaultsToEventListenerOnNet6And7` (net6.0/net7.0 TFMs). - Added `RuntimeMetrics_ExplicitDisable_OverridesDefault`. - Added RuntimeMetrics_InvalidValue_TreatedAsUnset_DefaultsToDiagnostics (codifies that invalid values like DD_RUNTIME_METRICS_ENABLED=blah are treated as unset, not as explicitly configured) ## Other details Part of [Enabling .NET Runtime Metrics by Default](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.hz8udk7cv9zf). Should land after the PRs below are merged which include necessary fixes: - #8265 - #8266 ### Moving forward After this lands, monitor: 1. **Telemetry**: `DD_RUNTIME_METRICS_ENABLED` and `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED` usage to track adoption of the new defaults. 2. **Support tickets**: Customers reporting missing metrics on .NET 6/7 (`contention_time`, ASP.NET Core counters, `compacting_gc` tag) after upgrading. 3. **Future**: Consider defaulting `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED=true` for all .NET 6+ in a later release once the .NET 6/7 EventCounter gap is documented and communicated.
## Summary of changes - Un-skip `TerminationSignalTests` - Run `TerminationSignalTests` on more TFMs - Fix bug with multiple-termination signals on .NET 10 - Wait for dogstatsd to finish flushing on exit ## Reason for change As part of the recent runtime metrics work (#8265), we skipped the `TerminationSignalTests`, as we found they were flaky with the new changes on .NET 10 (and failed outright if we properly `await`-ed for dogstatsd to flush on exit). Ultimately, this comes down to essentially a race condition in the new termination behaviour on .NET 10. Previously, we had this for .NET 10: - On startup, register for POSIX signals - When a posix signal fires, unregister the signals, and start shutdown/flush etc - After a small delay (100ms) in the test, send another posix signal - As we have already unregistered our handler, the app goes straight to shutdown, and we don't finish flushing. This doesn't affect <.NET 10, because POSIX is handled by the runtime, and they queue handling of the subsequent signals while the first one is ongoing. ## Implementation details The fix is essentially two changes: - Don't un-register the POSIX handlers - On subsequent POSIX signals wait for the first handler to complete before exiting This only affects the .NET 10 path, as the POSIX handlers only fire explicitly on those paths. As an aside, this allows enabling the statsd flush on shutdown. ## Test coverage I unskipped the currently-skipped test, and expanded the `TerminationSignalTests` to cover .NET 8+, instead of just .NET 10, so that we know we're _actually_ getting the same behaviour in both cases. (.NET 8 was a somewhat arbitrary choice, we could expand it further if we wanted to, but doesn't necessarily seem worth it to me). I then tested that in CI and saw it fail, before making the fix. ## Other details There's one "interesting" change of behaviour in terms of `Cancel`. 🤖 is adamant we _Shouldn't_ cancel the default signal handling on the first handler, because we _want_ the "default signal handling" to kick in _after_ we've run our shutdown hooks. I'm not entirely sure if that's true or not tbh, but it doesn't seem to affect the tests one way or another so I'm guessing, meh? 🤷♂️ --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
* [Version Bump] 3.40.0 (#8280) The following files were found to be modified (as expected) - [x] docs/CHANGELOG.md - [x] .azure-pipelines/ultimate-pipeline.yml - [x] profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/CMakeLists.txt - [x] profiler/src/ProfilerEngine/Datadog.Profiler.Native.Windows/Resource.rc - [x] profiler/src/ProfilerEngine/Datadog.Profiler.Native/dd_profiler_version.h - [x] profiler/src/ProfilerEngine/Datadog.Linux.ApiWrapper/CMakeLists.txt - [x] profiler/src/ProfilerEngine/ProductVersion.props - [x] shared/src/Datadog.Trace.ClrProfiler.Native/CMakeLists.txt - [x] shared/src/Datadog.Trace.ClrProfiler.Native/Resource.rc - [x] shared/src/msi-installer/WindowsInstaller.wixproj - [x] shared/src/native-src/version.h - [x] tracer/build/artifacts/dd-dotnet.sh - [x] tracer/build/_build/Build.cs - [x] tracer/samples/AutomaticTraceIdInjection/MicrosoftExtensionsExample/MicrosoftExtensionsExample.csproj - [x] tracer/samples/AutomaticTraceIdInjection/Log4NetExample/Log4NetExample.csproj - [x] tracer/samples/AutomaticTraceIdInjection/NLog40Example/NLog40Example.csproj - [x] tracer/samples/AutomaticTraceIdInjection/NLog45Example/NLog45Example.csproj - [x] tracer/samples/AutomaticTraceIdInjection/NLog46Example/NLog46Example.csproj - [x] tracer/samples/AutomaticTraceIdInjection/SerilogExample/SerilogExample.csproj - [x] tracer/samples/ConsoleApp/Alpine3.10.dockerfile - [x] tracer/samples/ConsoleApp/Alpine3.9.dockerfile - [x] tracer/samples/ConsoleApp/Debian.dockerfile - [x] tracer/samples/OpenTelemetry/Debian.dockerfile - [x] tracer/samples/WindowsContainer/Dockerfile - [x] tracer/src/Datadog.Trace.ClrProfiler.Managed.Loader/Startup.cs - [x] tracer/src/Datadog.Tracer.Native/CMakeLists.txt - [x] tracer/src/Datadog.Tracer.Native/dd_profiler_constants.h - [x] tracer/src/Datadog.Tracer.Native/Resource.rc - [x] tracer/src/Directory.Build.props - [x] tracer/src/Datadog.Trace/TracerConstants.cs @DataDog/apm-dotnet Co-authored-by: bouwkast <[email protected]> * Add an `IArrayPool<char>` implementation for vendored Newtonsoft.JSON (#8228) ## Summary of changes Adds a simple `IArrayPool<char>` for use by Newtonsoft.JSON, and uses it everywhere we can ## Reason for change Newtonsoft.JSON fundamentally works with .NET's `char` type (UTF-16), (as opposed to System.Text.Json which works with UTF-8 where it can). To do so, it needs to create a bunch of `char[]` instances to use as buffers. The `JsonTextReader` and `JsonTextWriter` abstractions allow plugging in an `IArrayPool<char>` implementation, and luckily this matches (pretty much exactly) the API exposed by the `ArrayPool` in corelib (+ vendored), so it's easy to implement. This should help alleviate some GC pressure, as we currently do a fair amount of serializing and deserializing. ## Implementation details Pretty simple: - Implement `IArrayPool<char>` using `ArrayPool<char>.Shared` - Get 🤖 to find everywhere that we could use it (`JsonTextReader` and `JsonTextWriter`) and initialize - Fix some cases where these weren't being disposed. > [!WARNING] > It's important that we _do_ dispose these, so that the arrays are correctly returned to the pool, so that we don't leak memory There are actually other places we can update too, as this PR doesn't cover the common `JsonConvert.Serialize()` etc, but I'll follow up with those in a separate PR. ## Test coverage All the existing tests should pass. I worked on this as part of general perf work on remote config, so the results are a bit fuzzy (as I can't remember if it includes the savings from #8226 as well), but the results are pretty conclusive, especially for big payloads 😅 | Method | size | Mean | Error | Allocated | | ---------------------------- | ----- | ----------: | ------------: | ---------: | | DeserializeResponse_Original | Small | 22.71 ns | 2,076.771 ns | 23.83 KB | | DeserializeResponse_Updated | Small | 13.70 us | 0.186 us | 17.23 KB | | | | | | | | DeserializeResponse_Original | Big | 1,953.04 us | 58,665.219 ns |2,343.26 KB | | DeserializeResponse_Updated | Big | 614.46 us | 11.988 us | 252.37 KB | ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-940 Discovered this while exploring remote config optimizations * Use `IArrayPool` in more places for JSON (#8236) ## Summary of changes Adds JSON helper APIs to ensure we use the array pool where possible ## Reason for change There are various "helper" APIs, which are wrappers around Newtonsoft.JSON's `JsonSerializer` and `JsonReader`/`JsonWriter` APIs. In #8228 we updated the explicit usages to use an array pool implementation for `JsonReader`/`JsonWriter` calls, but they're used internally without a pool in the helper cases. This PR creates alternative implementations which _do_ use the pool, updates existing code to use them, and adds the existing APIs to the "banned API" list. > There's another possible approach, in which we update the vendored code to _always_ use the array pool. I was torn as to which is the better option, and went for this approach in the end, but I'm not wedded to it, so happy to take the alternative approach if people think it's preferable? ## Implementation details - Use 🤖 to find all the potential places that we need to convert. - Create "array pool" versions of the APIs - Update the call sites to use the new APIs - Add the original APIs to the list of "banned" APIs to avoid using them accidentally in the future ## Test coverage Covered by all our existing tests, added some unit tests to confirm the new tests behave as expected ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-940 Discovered this while exploring remote config optimizations, but should help lots of areas. * Remote configuration tests and performance improvements (#8237) ## Summary of changes - Add tests for `RcmSubscriptionManager` and `RemoteConfigurationPath` - Replace regex with string comparison in `RemoteConfigurationPath` ## Reason for change - We were missing unit tests for remote config stuff, and I want to improve it without breaking things - The `RemoteConfigurationPath` is running a `Regex` on every file listed in the remote config response (which happens every 5s), but it's a simple pattern that can be easily directly implemented ## Implementation details - Used 🤖 to generate bunch of tests, and verified they are really how we want things to work - High level tests for `RcmSubscriptionManager` - Tests for `RemoteConfigurationPath` covering changes in this PR - More 🤖 in the conversion, but it's relatively simple, once you decode the allowed patterns from the Regex 😄 ## Test coverage Unit tests in this PR cover compatibility with the existing implementation. Simple benchmarking for the regex improvements: | Method | Runtime | Mean | Error | Allocated | | ---------------------------------------- | ------------------ | -------: | ------: | --------: | | RemoteConfigurationPathFromPath_Original | .NET 10.0 | 181.5 ns | 2.51 ns | 768 B | | RemoteConfigurationPathFromPath_Updated | .NET 10.0 | 54.8 ns | 3.90 ns | 152 B | | | | | | | | RemoteConfigurationPathFromPath_Original | .NET 6.0 | 204.0 ns | 2.64 ns | 768 B | | RemoteConfigurationPathFromPath_Updated | .NET 6.0 | 66.4 ns | 1.13 ns | 152 B | | | | | | | | RemoteConfigurationPathFromPath_Original | .NET Core 2.1 | 296.9 ns | 4.09 ns | 872 B | | RemoteConfigurationPathFromPath_Updated | .NET Core 2.1 | 82.2 ns | 2.21 ns | 160 B | | | | | | | | RemoteConfigurationPathFromPath_Original | .NET Core 3.1 | 281.0 ns | 3.79 ns | 768 B | | RemoteConfigurationPathFromPath_Updated | .NET Core 3.1 | 72.8 ns | 1.90 ns | 152 B | | | | | | | | RemoteConfigurationPathFromPath_Original | .NET Framework 4.8 | 326.7 ns | 2.11 ns | 875 B | | RemoteConfigurationPathFromPath_Updated | .NET Framework 4.8 | 110.0 ns | 1.76 ns | 160 B | <details><summary>Benchmarking code</summary> <p> ```csharp [MemoryDiagnoser, GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory), CategoriesColumn] public class RemoteConfigBenchmark { private string _pathToTest; private string _result; [GlobalSetup] public void GlobalSetup() { _pathToTest = "datadog/2/ASM_FEATURES/ASM_FEATURES-third/testname"; } [Benchmark] public string RemoteConfigurationPathFromPath_Original() { var result = OriginalRemoteConfigurationPath.FromPath(_pathToTest); _result = result.Id; return result.Path; } [Benchmark] public string RemoteConfigurationPathFromPath_Updated() { var result = RemoteConfigurationPath.FromPath(_pathToTest); _result = result.Id; return result.Path; } } ``` </p> </details> ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-940 All part of the Remote Config perf stack * Reduce payload size sent by agent in remote config (#8238) ## Summary of changes Stop sending the fixed `RootVersion = 1` with every remote config request ## Reason for change Currently we're sending a fixed value of `RootVersion = 1` for all our remote config requests, but doing so causes the agent to repeatedly send us all the root certificates, significantly increasing the payload size, because it thinks we haven't seen them. Sending the "final" version, acknowledges it, and stops all the extra data, saving ~35kB per call. ## Implementation details - Deserialize the roots in `GetRcmResponse`, leaving them as base64 encoded `string`s (which is how they are sent in the payload) - When processing the response, de-encode the _last_ root object, and deserialize it - Using the `Base64DecodingStream` introduced in #8226 to avoid extra allocations - Using the `IArrayPool` from #8228 - Introduces a `MinimalTufRoot` (in contrast to `TufRoot`) so that we only materialize what we need (the `roots.signed.version` key) This implementation avoids ~35kB per call for subsequent remote config requests. ## Test coverage Added unit test, and did manual test with the real agent, to confirm the expected behaviour (reduction in data sent) ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-940 All part of the Remote Config perf stack * Exclude Otel for MongoDb 3.7 (#8279) ## Summary of changes Adds MongoDB.Driver to the Activity ignore handler, to avoid duplicate instrumentations ## Reason for change MongoDB .NET driver v3.7.0 [adds support for OpenTelemetry](https://github.com/mongodb/mongo-csharp-driver/releases/tag/v3.7.0), but this results in duplicate instrumentations for our MongoDb instrumentation. You can see this in play in [the test-package-version PR](https://github.com/DataDog/dd-trace-dotnet/pull/8278) here An AI analysis (shown in full below), has the following summary: **Cannot replace custom instrumentation with OTel spans.** Key blockers: 1. **No query body** (`mongodb.query`) — This is the most valuable tag for debugging and is completely absent from OTel spans 2. **Wrong span type** (`http` instead of `mongodb`) — Would break DB categorization in Datadog UI 3. **Wrong span name** (`client.request` instead of `mongodb.query`) — Loses MongoDB-specific identification 4. **No separate service name** in SchemaV0 — Breaks service map topology 5. **Doubled span count** — 30 extra spans per trace adds overhead There's a bunch of other tag differences, some of which may or may-not be a problem, but the fact they don't tag the query body likely precludes us deferring to the otel approach, as it would be crucial lacking information. <details><summary>AI analysis comparing custom instrumentation to OTel</summary> <p> # MongoDB 3.7.0 OTel Instrumentation Analysis ## Context MongoDB.Driver 3.7.0 adds built-in OpenTelemetry instrumentation. When used with our existing dd-trace-dotnet custom instrumentation, this creates duplicate/overlapping spans. We need to determine whether the OTel spans can replace our custom instrumentation, or whether we should disable them. This analysis is based on the diff at `mongodb.diff`, which compares the SchemaV0 snapshot for the `3_0` package version (before) against the new output with MongoDB 3.7.0 (after). --- ## Span Count Change | | Before (3.0) | After (3.7.0) | |---|---|---| | App spans (internal) | 4 | 4 | | DD `mongodb.query` spans | 16 | 15 | | OTel Level 1 spans (logical operation) | 0 | **15** | | OTel Level 2 spans (wire command) | 0 | **15** | | **Total** | **20** | **49** | The span count more than doubles: **+30 new OTel spans** (15 L1 + 15 L2). Note: the "before" count has 16 mongodb.query spans vs 15 in "after". The initial `find` that was directly under Main() (Id_2) now has an OTel L1 parent (Id_6), and there's one fewer mongodb.query span for the `countDocuments` operations - actually looking more carefully, the count is the same (15 DD spans each for the 3 groups of sync/async x 5 operations + 1 initial find = 16... let me recount). Actually both before and after have 15 `mongodb.query` spans - same count. The diff just renumbers IDs. --- ## New Span Hierarchy ### Before (2-tier): ``` App span (e.g., "sync-calls") └── mongodb.query (our custom instrumentation) ``` ### After (4-tier): ``` App span (e.g., "sync-calls") └── OTel L1 - "client.request" (logical operation) └── mongodb.query (our custom instrumentation, RE-PARENTED) └── OTel L2 - "client.request" (wire protocol command) ``` Our `mongodb.query` spans are now sandwiched between two OTel spans. The OTel L1 span becomes the parent of our span, and our span becomes the parent of the OTel L2 span. --- ## Two Levels of OTel Spans ### OTel Level 1 — Logical Operation Spans | Property | Value | |---|---| | Name | `client.request` | | Resource | `<operation> <db>.<collection>` (e.g., `find test-db.employees`) | | Service | `Samples.MongoDB` (app service name, NOT separate) | | Type | `http` (incorrect for a DB span!) | | span.kind | `client` | **Tags:** - `db.collection.name`: e.g., `employees` - `db.namespace`: e.g., `test-db` - `db.operation.name`: e.g., `find`, `delete`, `insert`, `countDocuments` - `db.operation.summary`: e.g., `find test-db.employees` - `db.system.name`: `mongodb` - `otel.library.name`: `MongoDB.Driver` - `otel.library.version`: `3.7.0` - `otel.status_code`: `STATUS_CODE_OK` **No metrics.** No host/port info at this level. ### OTel Level 2 — Wire Protocol Command Spans | Property | Value | |---|---| | Name | `client.request` | | Resource | `<command>` (e.g., `find`, `delete`, `aggregate`) — **just the command, no db name** | | Service | `Samples.MongoDB` (app service name) | | Type | `http` (incorrect for a DB span!) | | span.kind | `client` | **Tags:** - `db.collection.name`: e.g., `employees` - `db.command.name`: e.g., `find`, `delete`, `insert`, `aggregate` - `db.mongodb.lsid`: Session ID (BSON) - `db.namespace`: e.g., `test-db` - `db.query.summary`: e.g., `find test-db.employees` - `db.system.name`: `mongodb` - `network.transport`: `tcp` - `server.address`: e.g., `mongo` - `otel.library.name`: `MongoDB.Driver` - `otel.library.version`: `3.7.0` - `otel.status_code`: `STATUS_CODE_OK` **Metrics:** - `db.mongodb.driver_connection_id`: e.g., `3.0` - `db.mongodb.server_connection_id`: e.g., `7.0` - `server.port`: e.g., `27017.0` --- ## Tag-by-Tag Comparison: OTel Spans vs DD Custom Spans ### DD Custom `mongodb.query` span tags: | Tag | DD Value | OTel L1 Equivalent | OTel L2 Equivalent | |---|---|---|---| | `component` | `MongoDb` | **MISSING** | **MISSING** | | `db.name` | `test-db` | `db.namespace` = `test-db` | `db.namespace` = `test-db` | | `mongodb.collection` | `employees` | `db.collection.name` = `employees` | `db.collection.name` = `employees` | | `mongodb.query` | Full BSON query JSON | **MISSING** | **MISSING** | | `out.host` | `mongo` | **MISSING** | `server.address` = `mongo` | | `out.port` | `27017` | **MISSING** | `server.port` = `27017.0` (metric) | | `span.kind` | `client` | `client` | `client` | | `_dd.base_service` | `Samples.MongoDB` | N/A | N/A | ### DD Custom span properties: | Property | DD Value | OTel L1 | OTel L2 | |---|---|---|---| | Name | `mongodb.query` | `client.request` | `client.request` | | Resource | `<op> <db>` (e.g., `find test-db`) | `<op> <db>.<coll>` (e.g., `find test-db.employees`) | `<command>` (e.g., `find`) | | Service | `Samples.MongoDB-mongodb` (SchemaV0) | `Samples.MongoDB` | `Samples.MongoDB` | | Type | `mongodb` | `http` | `http` | ### Tags present in OTel but NOT in DD custom spans: - `db.operation.name` / `db.command.name` - `db.operation.summary` / `db.query.summary` - `db.system.name` - `db.mongodb.lsid` (session ID - L2 only) - `network.transport` (L2 only) - `server.address` (L2 only — similar to `out.host`) - `otel.library.name`, `otel.library.version` - `otel.status_code` ### Metrics present in OTel but NOT in DD custom spans: - `db.mongodb.driver_connection_id` (L2 only) - `db.mongodb.server_connection_id` (L2 only) - `server.port` (L2 only — similar to `out.port`) --- ## Operation Name Mapping: `countDocuments` vs `aggregate` Notable: The OTel L1 span uses the **logical operation name** `countDocuments`, while our DD custom span (and OTel L2) use the **wire protocol command** `aggregate`. This is because MongoDB implements `countDocuments()` as an aggregate pipeline internally. The OTel L1 span provides the more user-friendly logical name. --- ## Critical Gaps if Replacing DD Custom with OTel ### 1. **`mongodb.query` tag — MISSING from OTel** The full BSON query body is the most significant tag in our custom instrumentation. **Neither OTel level provides this.** The OTel spans only include `db.query.summary` (e.g., `find test-db.employees`) which is just the operation + namespace, not the actual query filter/pipeline. ### 2. **Span Type — `http` instead of `mongodb`** Both OTel span levels use Type `http`, while our custom spans correctly use Type `mongodb`. This affects categorization in the Datadog UI (DB queries vs HTTP calls). ### 3. **Span Name — `client.request` instead of `mongodb.query`** Generic OTel name vs our specific operation name. ### 4. **Service Name — No separate service (SchemaV0)** In SchemaV0, our custom spans use `Samples.MongoDB-mongodb` (separate service), while OTel spans use the app service name `Samples.MongoDB`. This means no dedicated MongoDB service in the service map. ### 5. **Resource Name — Different format** - DD: `find test-db` (operation + database) - OTel L1: `find test-db.employees` (operation + db.collection — **more specific, arguably better**) - OTel L2: `find` (just the command — less useful) ### 6. **`component` tag — MISSING from OTel** Our DD spans set `component: MongoDb`. OTel spans don't have this. --- ## What OTel Does Better 1. **Collection name in resource**: `find test-db.employees` is more informative than `find test-db` 2. **Logical operation names**: `countDocuments` instead of `aggregate` (L1 only) 3. **Connection metadata**: `driver_connection_id`, `server_connection_id`, `network.transport` 4. **Session tracking**: `db.mongodb.lsid` 5. **Semantic conventions**: Uses standard OTel DB semantic conventions (`db.namespace`, `db.system.name`, etc.) --- ## Summary / Recommendation **Cannot replace custom instrumentation with OTel spans.** Key blockers: 1. **No query body** (`mongodb.query`) — This is the most valuable tag for debugging and is completely absent from OTel spans 2. **Wrong span type** (`http` instead of `mongodb`) — Would break DB categorization in Datadog UI 3. **Wrong span name** (`client.request` instead of `mongodb.query`) — Loses MongoDB-specific identification 4. **No separate service name** in SchemaV0 — Breaks service map topology 5. **Doubled span count** — 30 extra spans per trace adds overhead **Recommended approach: Disable the OTel instrumentation** for MongoDB 3.7.0+ to maintain the existing behavior. This likely means either: - Suppressing the MongoDB.Driver OTel ActivitySource at startup - Or adjusting the integration to detect and skip OTel span creation when our instrumentation is active If keeping both is desired for any reason, the OTel spans should at minimum be filtered out from the test snapshots, and ideally suppressed at runtime to avoid the 2.5x span multiplication. </p> </details> ## Implementation details Add `MongoDB.Driver` to the activity ignore list. ## Test coverage Bumped the tests to run with 3.7.0, so should be covered ## Other details I do wonder if this is definitely the approach we _should_ be taking, but let's take that offline * Fix Datadog.Trace.Annotations dependency version in Datadog.AzureFunctions nuspec (#8285) ## Summary of changes The `BuildAzureFunctionsNuget` target used `.SetVersion(Version)` which appears to pass a `-p:Version` global MSBuild property, which overrode the version of the Annotations project in the generated nuspec for the Datadog.AzureFunctions package. ## Reason for change In 3.39.0 we realized that this `SetVersion` overrides the version of dependent projects when we build and pack them in CI, intention of the `SetVersion` call was to ease local development / debugging. ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> This was used for Build-AzureFunctionsNuget.ps1 to help with local development / debugging for generating versions to avoid NuGet caching but had the side effect of changing the version of the Annotations in the nuspec <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. --> * Retry ChromeDriver startup in Selenium tests (#8284) <!-- dd-meta {"pullId":"5d5a8ab7-cda5-48c4-85e8-2eee4bebe9fa","source":"chat","resourceId":"edb8418c-6513-4707-a649-1a74f8d8cc5a","workflowId":"1a1e3ffe-b6c3-4088-82e5-f2474a241011","codeChangeId":"1a1e3ffe-b6c3-4088-82e5-f2474a241011","sourceType":"action_platform_custom_agent"} --> ## Summary of changes Improve resilience of the Selenium CI integration path against transient Chrome startup crashes by retrying the ChromeDriver initialization. ``` System.InvalidOperationException: session not created: Chrome failed to start: crashed. (session not created: DevToolsActivePort file doesn't exist) (The process started from chrome location C:\Users\AzDevOps\.cache\selenium\chrome\win64\146.0.7680.66\chrome.exe is no longer running, so ChromeDriver is assuming that Chrome has crashed.) ``` ## Reason for change We observed failures that appear to conflict when tests start close together in CI. A longer pause between ChromeDriver startup retries reduces pressure on shared CI resources and gives Chrome more time to recover between attempts. ## Implementation details - Existing behavior retained: - ChromeDriver creation is retried up to 3 total attempts. - Startup exceptions (`InvalidOperationException` and `WebDriverException`) are retried. - Partially initialized driver instances are disposed before retry. - Scope remains minimal and targeted: only ChromeDriver initialization is retried. ## Test coverage ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. --> --- PR by Bits [View session in Datadog](https://app.datadoghq.com/code/edb8418c-6513-4707-a649-1a74f8d8cc5a) Comment @datadog to request changes Co-authored-by: datadog-datadog-prod-us1-2[bot] <261164178+datadog-datadog-prod-us1-2[bot]@users.noreply.github.com> * [Test Package Versions Bump] Updating package versions (#8278) Updates the package versions for integration tests. Co-authored-by: andrewlock <[email protected]> * More remote config performance improvements (#8241) ## Summary of changes A variety of minor performance improvements to remote config ## Reason for change I did some initial benchmarking of remote config, as well as running a test app with ASM/Debugger enabled (which use RCM), and the results weren't great. Given we make RCM requests every 5s, smallish changes here should add up, though the real-world effect will be tricky to gauge. ## Implementation details Most of the individual changes are small. In summary: - Making work lazy where possible (delaying creating collections) - Don't re-create dictionaries inside loops if not required - Avoid creating empty dictionaries and collections for no-op RCM responses - Avoid creating empty collections in JSON objects when they're not in the JSON - Cache things that change rarely - The `ExtraServicesProvider` will rarely see new service names, so cache the array (as the collection is append-only) - Don't create a new request each time, just update any values that may have changed - Cache the capabilities array which will rarely change - There's actually a potential threading bug around the use of `BigInteger`. We probably could/should use `ulong` instead but that I'll look at that in a separate PR - Use abstractions introduced earlier in the stack (e.g. #8226, #8228) Additionally, I did a little bit of cleanup: - Remove unused members from the `IRcmSubscriptionManager` interface - Add `#nullable enable` to the RCM types (and fix nullability where required) ## Test coverage Mostly covered by existing unit tests, also did some manual testing. Finally, ran a few benchmarks, but it's a bit tricky to check reliably. This benchmark is benchmarking `_manager.SendRequest(_rcmTracer, _ => _steadyStateResponseTask)` and passing the same response every time. In practice, the response changes every time, so this isn't strictly representative, but with the changes to request caching, this should actually mean our improvements are _better_ than the original would be. The regression in the .NET duration is curious, but I'm not massively concerned, and the allocations are obviously down a lot at least | Method | Runtime | Mean | Error | Allocated | | ------------------------ | ------------------ | ----------: | -----------: | --------: | | PollSteadyState_Original | .NET 10.0 | 14,235.8 ns | 275.09 ns | 12.04 KB | | PollSteadyState_Updated | .NET 10.0 | 20,472.0 ns | 1,352.567 ns | 3.90 KB | | | | | | | | PollSteadyState_Original | .NET 6.0 | 33,915.7 ns | 3,339.42 ns | 12.27 KB | | PollSteadyState_Updated | .NET 6.0 | 14,979.7 ns | 447.784 ns | 3.99 KB | | | | | | | | PollSteadyState_Original | .NET Core 2.1 | 21,488.2 ns | 365.39 ns | 12.85 KB | | PollSteadyState_Updated | .NET Core 2.1 | 17,847.2 ns | 693.510 ns | 4.04 KB | | | | | | | | PollSteadyState_Original | .NET Core 3.1 | 18,925.2 ns | 304.16 ns | 12.23 KB | | PollSteadyState_Updated | .NET Core 3.1 | 15,407.7 ns | 313.26 ns | 4.01 KB | | | | | | | | PollSteadyState_Original | .NET Framework 4.8 | 20,946.8 ns | 320.06 ns | 14.21 KB | | PollSteadyState_Updated | .NET Framework 4.8 | 16,635.2 ns | 266.526 ns | 4.40 KB | ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-940 All part of the Remote Config perf stack. I think this is probably about the end of it for now * Rename telemetry `GetData` methods to `GetIncrementalData` (#8269) ## Summary of changes Renames `GetData` to `GetIncrementalData` to differentiate from `GetFullData()` ## Reason for change In #8227 it was flagged that `GetData()` and `GetFullData()` are easy to confuse. Renaming `GetData` to `GetIncrementalData` should solve it. ## Implementation details Deterministic rename (thank you IDEs, take that AI) ## Test coverage All covered by existing * WCF cleanup and trace context extraction (#8263) ## Summary of changes Follow on fixes from WCF improvements made in #7842 ## Reason for change @zacharycmontoya and I identified some things to fix while working on the above PR, but deferred fixing them till later. And now is that time! We flagged two main things: - We're using a weak table currently for storing the scopes. That's fine, but we always dispose the scope, we should just remove the object from the table at the same time to avoid accessing a closed span (and actively reducing the size of the table) - We should skip extracting WCF message headers if there's an active scope. Previously we were only doing that if we were working with http headers While trying to create a repro for the second point (by using [the Otel WCF instrumentation](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/a1c22d0dac923b564ae7d2cfa0d27c479e65455c/src/OpenTelemetry.Instrumentation.Wcf/README.md)) 🤖 discovered a different issue, whereby the WCF server `wcf.request` spans were incorrectly parented under the manual span instead, of the OTel WCF client span `dotnet_wcf.client.request` for `NetTcpBinding` (when `DD_TRACE_OTEL_ENABLED=true` and OpenTelemetry WCF client instrumentation was active). The latter issue was initially related to how we interact with the Otel instrumentation in the sample, but switching to "pure" OTel for propagation still revealed a parenting problem, because we were never extracting the OTel headers. > Note that this latter point _only_ impacts the `NetTcpBinding` because with HTTP we handle all the propagation as expected. ## Implementation details There are 3 fixes: - (minor) Remove scope from weak table when we dispose it. - (minor) Don't try to extract propagation context in server-side WCF if there's already an active scope - couldn't repro, but it makes sense - (fix) Also check the W3C trace context namespace for headers injected ## Test coverage To increase coverage, reworked the `Samples.Wcf` to allow optionally using the Otel instrumentation: - Switch to combinatorial data (makes it easier to add another boolean) - Optionally allow setting up wcf client-side Otel instrumentation - When enabled, we configure a `TraceProvider`, add the `TelemetryEndpointBehavior`, and _stop_ manually injecting our headers into the message - When disabled, the sample is identical to before - Update the snapshots - Rename the existing snapshots to include the additional `useOtelClientInstrumentation=False` suffix - Generate new snapshots for `useOtelClientInstrumentation=True` - Make the fix, and then update the snapshots in an additional commit, to demonstrate the change ## Other details Heavily used Claude Code on the testing and implementation. We knew about the first two issues, and it discovered the third when trying to follow our suggestion for a repro * Detect whether an app has been trimmed (#8281) ## Summary of changes Detects whether an app has been published with trimming and whether our Trimming.xml file has been used. ## Reason for change Using trimming and _not_ using our trimming file can cause all sorts of strange errors. It would be useful for support (and for informing customers) if an app _is_ using trimming and is _not_ using our trimming file ## Implementation details Uses three `Type`s in the BCL as "probes" for whether an app is trimmed: - `System.Resources.ResourceWriter, System.Resources.Writer` - `System.IO.IsolatedStorage.IsolatedStorageScope, System.IO.IsolatedStorage` - `System.Net.NetworkInformation.PingCompletedEventArgs, System.Net.Ping` These are intentionally somewhat obtuse types that we would _expect_ to be trimmed. Settled on them by iterating with 🤖 but we could certainly change these. The overall approach is - Try to load the first two types. These are manually specified in our trimming file, so if we _fail_ to load _either_ one, then we know we _are_ in a trimmed app, _and_ they didn't add our file - Try to load the third type, which _isn't_ in our trimming file. If it loads, we're _not_ trimmed at all. If it doesn't load, we're in a trimmed app, but they _did_ add our file. If we detect the bad situation, we add a warning to the logs. Either way, we tag the telemetry error logs with `trim:err/yes/no` where: - `err` is trimmed and didn't add our file - `yes` is trimmed but they added our file - `no` not trimmed ## Test coverage - Updated unit tests - Added integration test about telemetry error logs in app trimming scenario [Pushed an initial test](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=197054&view=logs&j=038b8080-1d19-502b-3685-9d5eff966aef&t=7b04c0a4-d19c-5f7e-a67e-3b6a219d2507&l=40) without adding anything to the trimming file, and confirmed that we tagged as `err` and write the error log (which correctly caused the integration tests and trimming smoke tests to fail). ## Other details The main consideration is the performance impact of loading these extra types, from obtuse assemblies, on the hot path of app load. Each of the assemblies is ~80kb (I swapped from System.Net.Mail because it's ~5x as big), but then there's the dependency tree too... I considered using an ACL and unloading, but as I understand it, that wouldn't necessarily _actually_ unload them, seeing as they're part of the shared framework, but I confess I'm trusting the AI on that one 😅 --------- Co-authored-by: Copilot <[email protected]> * Allow generating a snapshot of the MSI contents (#8270) ## Summary of changes Adds a "snapshot generator" for the MSI contents ## Reason for change We've discussed updating/switching to newer versions of Wix, and we want to make sure we don't regress anything ## Implementation details Uses the _WixToolset.Dtf.WindowsInstaller_ nuget package to read the contents of the MSI. We then scrub values which we expect to change (version numbers, filesizes etc) and dump the values out as a yaml file (could have done any format, we already had a transient reference to yamldotnet, I just made it explicit) ## Test coverage Tested with a couple of MSIs from master, and they pass. ## Other details I considered an alternative, where we try to understand the _impact_ of installing the MSI, which on the surface is what we _really_ care about, but seemed like a much harder prospect 😅 * Remove unnecessary use of duck typing in HTTP integrations (#8275) ## Summary of changes Removes unnecessary use of duck typing in HTTP integrations ## Reason for change We're currently always using duck typing in the various HTTP integrations, but these types are always unconditionally available in .NET Core (and we already reference those assemblies), so the duck typing is an unnecessary layer that will reduce performance for no benefit. ## Implementation details `#if` our way to glory on the HTTP (and one of the gRPC) integrations. If we're on .NET Core, these types are available, so we just use them ## Test coverage Just a refactoring, so all the existing tests should cover this change. _Hopefully_ we'll see some tiny movement on the benchmarks, but I don't hold out a huge amount of hope for that. Either way, I think this change can only be an improvement, so is worth it. ## Other details Deleted the excessive comments on our integrations, seeing as they're just noise and don't tell us much (we exclude them by default on new integrations now) One very interesting point - we _can't_ reference the "well known types" _directly_ in the integrations, because this causes failures when there are multiple Assembly Load Contexts. This is kinda surprising, but something to bear in mind, and something we potentially need to look into elsewhere too... * Update Wix to 5.x.x (#8268) ## Summary of changes Updates our MSI project to use [Wix 5.x.x](https://docs.firegiant.com/wix/whatsnew/#whats-new-in-wix-v5) instead of Wix 3 ## Reason for change [Wix 3 was deprecated a year ago](https://docs.firegiant.com/wix/wix3/), and is generally clunky and hard to use, as it relies on a global install + .NET Framework 3.5. The newer versions of Wix use newer SDK-style projects, are deployed as nuget packages, and can just be built with a normal `dotnet build` - Wix 4: Quite a big change - Wix 5: Pretty much back-compatible with 4 - Wix 6: [Shifted licensing model](https://docs.firegiant.com/wix/whatsnew/#open-source-maintenance-fee) - we need to look into this if we want to upgrade further. - Wix 7: As above ## Implementation details This was entirely 🤖 driven, but [there's also a .NET tool](https://docs.firegiant.com/wix/whatsnew/#convert-wix-authoring-from-the-command-line) to help with the conversion. _Mostly_ the changes are just "annoying", e.g. moving values from being element text to a `Value` property, etc. ## Test coverage At the end of the day, the generated MSI is _essentially_ the same as confirmed by the snapshots created in #8270. The changes all appear to be benign changes in hashing algorithms, or renaming of wix properties. What's more, I tested the install, and it _looks_ the same (and works), and the MSI tests all pass, which is obviously the important thing! 😄 <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/54995470-b846-419c-9f18-e07c1daae127" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/0997801f-7143-4c25-88d4-d66ad2404968" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/63381b12-f312-4b45-9ae3-d2c77b23d377" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/320758a9-f0b7-42e8-a1db-4c8b4b8514ad" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/65e1fa04-f02e-42bf-8fcd-efe704000901" /> ## Other details The removal of the `Win64="yes"` and `Win64="$(var.Win64)"` attributes were the main thing I was unsure about. There _is_ [a `Bitness` attribute now](https://docs.firegiant.com/wix/schema/wxs/component/), with values `default`, `always32`, or `always64`, which is pretty much equivalent. However, seeing as we _only_ produce an x64 installer, and not an x86 installer, I think this is essentially just legacy cruft which is ok to remove. We _might_ regret that choice if/when we need an arm64 installer, but I think we'll need to look at everything again at that point anyway, so I don't think it's worth worrying about 😄 --------- Co-authored-by: Claude <[email protected]> * (CI) Migrating benchmark-serverless CI job to short-lived tokens (#8219) ## Summary of changes Migrated from use of Gitlab personal access token to short-lived token (using authanywhere) when cloning the serverless-tools repo for the benchmark-serverless CI job. ## Reason for change Keeping up-to-date with current policies about tokens, avoiding token expiry issues. ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. --> * Add _dd.svc_src meta tag to track service name source (#8274) ## Summary of changes Add `_dd.svc_src` meta tag to spans to track the source of the service name. When an integration overrides the default service name (e.g., schema V0 without `removeClientServiceNames`), the tag is set to the integration name (e.g., `"redis"`, `"kafka"`, `"http-client"`). When a service name mapping is configured via `DD_TRACE_SERVICE_MAPPING`, the tag is set to `"opt.service_mapping"`. When the default service name is used, no tag is emitted. Jira: https://datadoghq.atlassian.net/browse/APMLP-1015 RFC: https://docs.google.com/document/d/11OnbVYMDK-c5D-_V4QfOvL0Pc0z5oFQFGY3xSI-W7xk/edit?tab=t.0 ## Reason for change .NET equivalent of [dd-trace-java#10607](https://github.com/DataDog/dd-trace-java/pull/10607). Service name source attribution lets the backend know which component set the service name on each span. ## Implementation details - **Tag constant**: `Tags.ServiceNameSource = "_dd.svc_src"` and `SpanContext.ServiceNameSource` property. - **`ServiceNameMetadata`**: Encapsulates resolved service name and source attribution. Returned by all schema `GetServiceNameMetadata()` methods and `PerTraceSettings.GetServiceNameMetadata()`. - **Schema-level source**: `DatabaseSchema`, `MessagingSchema`, and `ClientSchema` each use a `Resolve` helper that determines the source: `"opt.service_mapping"` when from `DD_TRACE_SERVICE_MAPPING`, the integration key when service name ≠ default, or `null` otherwise. - **`PerTraceSettings.GetServiceNameMetadata()`**: For AdoNet and AWS integrations that resolve service names dynamically, returns `"opt.service_mapping"` for mapped names, the integration key for suffixed names, or `null` for default. - **Integration callsites**: ~30 files updated to pass `serviceNameSource`. Server-side integrations using the default service name are unchanged. ## Test coverage - **`DatabaseSchemaTests`**, **`MessagingSchemaTests`**, **`ClientSchemaTests`**: Tests for source attribution. - **Snapshot files**: 130+ integration test snapshots and 3 smoke test snapshots updated. ## Follow-up items - V1 schema with `DD_TRACE_SERVICE_MAPPING` could emit `opt.service_mapping` (mapped name differs from default). V1 SpanMetadata rules and snapshots would need updating. Deferred to a follow-up PR. - Manual set services (`"m"` source) and client stats payload — separate PRs. ## Other details * Move running of smoke tests to Nuke (#8271) ## Summary of changes Instead of most of the logic for running smoke tests being embedded in the yaml and bash inside yaml, move the building and running of smoke tests into Nuke. ## Reason for change The previous design had some downsides: - Very tied to Azure Devops. If we want to migrate to gitlab at some point, this _should_ make it easier, because devops is "doing less" - Hard to run smoke tests locally. If you wanted to investigate a scenario, you'd have to decode all the docker, docker compose, and bash scripts that you needed to run to get something _resembling_ the test setup. - There was a lot of duplication, because it's hard to remove that in a clean way from some of the yaml without creating loads of fine-grained steps (which have their own difficulties). Moving to C# makes it easy to (for example) have try-catch blocks, custom retries etc - Bash in YAML is kind of ewww ## Implementation details I initially tried to implement this over a year ago, using TestContainers, but tl;dr; I ran into a bunch of limitations that I couldn't get past (APIs that we needed, which just didn't exist, differences between windows/linux etc), so I abandoned it. Until 🤖 made exploring these things easier again! The latest approach uses the https://github.com/dotnet/Docker.DotNet/ project, which provides a strongly-typed way to call the docker HTTP API (which is what TestContainers actually uses under the hood - it even uses this project). This made it _much_ easier to convert the explicit steps that we are doing currently in bash/yaml/docker-compose to being simple C# methods. At a high level, the implementation roughly follows what we have today, but it's tied much _less_ to the azure devops infrastructure, as we just run our Nuke tasks in the same way we do today (i.e. directly on the box for Windows, in a docker container for Linux). A high level overview: - The `GenerateVariables` stage still generates the matrix of variables, but it only needs to generate a _category_ (e.g. `LinuxX64Installer`/`WindowsFleetInstallerIis`), and an associated _scenario_ (the specific test, e.g. `ubuntu, .NET 10, .deb`). - Renamed the stages (and associated matricies) to make them more consistent e.g.. `smoke_<x64|arm64|win|macos>_<installer|nuget|fleet>_tests`. We can easily tweak this if we prefer - To run a test (e.g. locally) `build.ps1 RunArtifactSmokeTests -SmokeTestCategory "LinuxX64Installer" -SmokeTestScenario "someScenario"` - All of the work for building the images, building/pulling the test agent/running the smoke tests/running crash tests/Doing snapshot verification is handled by Nuke. We have automatic retries around all the parts that could fail (i.e. anything docker or HTTP related) That also means we can delete various things - All the old stages in the pipelin - The old run-snapshot-test.yml - The entries in the docker-compose (the test-agent is actually still used in a few places, so those stay) Also includes a few tiny tweaks and cleanup (commented in the files as appropriate) ## Test coverage The same hopefully!? I've run the full sweet of tests several times, and spot checked various of the tests to make sure everything looks ok, and as far as I can tell, it does! Also temporarily [modified the snapshots ](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=196876&view=results) to confirm that causes everything to fail too ## Other details The _big_ one which I didn't/couldn't easily convert is the macos smoke tests. These are written _completely_ differently today, because they don't run in containers (which means we have to handle a whole bunch of different issues) and rather just duplicate a whole bunch of logic. It's _probably_ not worth the effort to port them into Nuke at the moment, but I'm open to doing it in a follow up if people feel one way or the other. The other thing is that I _didn't_ move the "downloading of artifacts" into the nuke job, though technically we could, and it would make running locally even easier. My reason for _not_ doing that was that it ties the nuke side to the azure devops side completely then, and if we rename an artifact in the yaml (for some reason) it's far more likely we'll forget it on the c# side. https://datadoghq.atlassian.net/browse/LANGPLAT-823 * [Metrics] Fixing sync-over-async patterns in DogStatsD Client Fork (#8265) ## Summary of changes Fix sync-over-async patterns in the vendored DogStatsD client and add async disposal for shutdown. ## Reason for change `NamedPipeTransport.SendBuffer()` calls `WriteAsync().Wait()` on `NamedPipeClientStream` which leads to sync-over-async that can deadlock under thread pool starvation. `AsynchronousWorker.Dispose()` also blocks with `worker.Wait()` on LongRunning tasks, making shutdown slow when the agent is unreachable. Part of [Enabling .NET Runtime Metrics by Default](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.7u0xyjk7jxq5). This should land before we enable Runtime Metrics by default since it hardens the shared DogStatsD client that runtime metrics uses. ## Implementation details - `NamedPipeTransport`: Replace `WriteAsync().Wait()` with synchronous `Write()`. This is a worker thread sending metrics, so sync is the right fit. - Add `DisposeAsync()` through the disposal chain: `AsynchronousWorker` -> `StatsBufferize` -> `StatsdData` -> `DogStatsdService` -> `StatsdManager`. - `StatsdClientHolder` uses a `TaskCompletionSource` to signal when async disposal finishes, bridging the sync `Release()` path and the async shutdown path. - `TracerManager.RunShutdownTasksAsync` now awaits `StatsdManager.DisposeAsync()` instead of synchronous `Dispose()`. ## Test coverage Existing tests still pass 🤞. ## Other details <!-- Fixes #issue --> * [Metrics] Send Diagnostics GC Pause Time as Counter instead of Timer (#8266) ## Summary of changes Send Diagnostics GC pause time as a Counter instead of a Timer. ## Reason for change The Diagnostics listener calls `GC.GetTotalPauseDuration()` which returns cumulative pause for the whole 10s interval. Sending that as a single `Timer()` inflates `.median`/`.max` compared to EventListener, which sends one `Timer()` per individual GC event. There's no public .NET API for per-GC pause duration without EventPipe. After consulting with the Agent Metrics team (#agent-metrics), the recommended approach is a separate Counter for total pause, with average computed at query time. See [RFC: Enabling .NET Runtime Metrics by Default — GC Pause Time](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.hz8udk7cv9zf). ## Implementation details - Add `GcPauseTimeTotal` constant (`runtime.dotnet.gc.pause_time.total`) to `MetricsNames`. - Replace `statsd.Timer(MetricsNames.GcPauseTime, ...)` with `statsd.Counter(MetricsNames.GcPauseTimeTotal, (long)totalPauseDelta)` in `DiagnosticsMetricsRuntimeMetricsListener`. - Average per-GC pause can be computed at query time: `pause_time.total / (gc.count.gen0 + gen1 + gen2)` in OOTB dashboard updated in a separate PR: https://github.com/DataDog/integrations-internal-core/pull/2823 ## Test coverage Updated `DiagnosticMetricsRuntimeMetricsListenerTests.MonitorGarbageCollections` to assert `Counter(GcPauseTimeTotal)` instead of `Timer(GcPauseTime)`. ## Other details Part of [Enabling .NET Runtime Metrics by Default](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.7u0xyjk7jxq5). * [Agent Skill] Rename to `analyze-azdo-build`, add retry support, and more (#8247) ## Summary of changes - ⭐ Rename skill from `troubleshoot-ci-build` to `analyze-azdo-build` - ⭐ Add support for retrying failed/canceled stages - Show Stage > Job > Task failures as a hierarchy (visual change only) - Add prerequisite install docs for `gh`, `az` CLI, and `azure-devops` extension ## Reason for change The CI build analysis skill needed several improvements: - The name `troubleshoot-ci-build` was generic; `analyze-azdo-build` is more specific - Failed stages/jobs/tasks were shown as flat lists, making it hard to trace failures through the pipeline hierarchy - API helper functions were duplicated and not reusable by other scripts - There was no way to retry failed stages without navigating the Azure DevOps UI - Prerequisites lacked install instructions, leaving users to figure it out themselves ## Implementation details - **Skill rename**: Moved `.claude/skills/troubleshoot-ci-build/` to `.claude/skills/analyze-azdo-build/` and updated the skill name/description - **Failure hierarchy**: Added `Get-FailureHierarchy` function that walks `parentId` chains in timeline records to build a Stage > Job > Task tree view - **Shared module** (`AzureDevOpsHelpers.psm1`): Extracted `Invoke-AzDevOpsApi`, `Get-BuildIdFromPR`, and new `Resolve-BuildId` into a reusable PowerShell module - **Retry script** (`Retry-AzureDevOpsFailedStages.ps1`): Retries failed/canceled stages via the Azure DevOps stages API; supports `-All`, `-Stage`, `-ForceRetryAllJobs`, `-WhatIf`, and interactive selection - **Prerequisite docs**: Added verify commands, one-liner installs (`winget`/`brew`), and official docs links for all CLI tools - **Tighten `allowed-tools`**: Replaced broad `Bash(pwsh:*)` permission with specific patterns for the three pwsh commands the skill uses (`pwsh -Version`, `Get-AzureDevOpsBuildAnalysis.ps1`, `Retry-AzureDevOpsFailedStages.ps1`) - Updated `SKILL.md` and `scripts-reference.md` documentation throughout ## Test coverage Tested against real Azure DevOps builds (`196471`, `196475`) with different failure patterns. ## Other details > *"I tried to retry my own failed stages once, but my therapist said that's called 'rumination'."* — Claude 🤖 * [Runtime Metrics] Enables Runtime Metrics by Default for .NET 6+ (#8267) ## Summary of changes Enable runtime metrics by default on .NET 6+ using the Diagnostics listener for services where the config is unset otherwise continue to use EventListener, and default existing explicit users to Diagnostics on .NET 8+ since there is no loss of ASP.NET Core Metrics. ## Reason for change Runtime metrics are currently opt-in. The EventListener/EventPipe implementation has known runtime bugs: shutdown crashes (dotnet/runtime#103480) and CPU/memory leaks (dotnet/runtime#111368). The Diagnostics listener avoids these and has comparable or better performance in our tests. See [RFC: Enabling .NET Runtime Metrics by Default — Option A](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.bs3q57sbzfh3). ## Implementation details Configuration logic for `DD_RUNTIME_METRICS_ENABLED`: - If explicitly set, use that value - If not set, default to `true` on .NET 6+ Configuration logic for `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED`: - If explicitly set, use that value - If not set: - .NET 8+: defaults to `true` (full metric coverage including ASP.NET Core meters) - .NET 6/7 with `DD_RUNTIME_METRICS_ENABLED` not set: defaults to `true` - .NET 6/7 with `DD_RUNTIME_METRICS_ENABLED=true`: defaults to `false` (keeps EventListener to preserve ASP.NET Core EventCounter metrics) The .NET 8 check uses `Environment.Version.Major >= 8`. Also fixes `RuntimeMetricsWriter` to dispose `DiagnosticsMetricsRuntimeMetricsListener` on .NET 6+ (`MeterListener` is safe to dispose, unlike `EventListener`). The runtime metrics collector selected depends on the runtime version and whether `DD_RUNTIME_METRICS_ENABLED` was explicitly configured: | Runtime | `DD_RUNTIME_METRICS_ENABLED` | Collector used | |---|---|---| | .NET 8+ | not set / invalid | Diagnostics (default) | | .NET 8+ | `true` | Diagnostics (default) | | .NET 6/7 | not set / invalid | Diagnostics (default) | | .NET 6/7 | `true` (explicit) | EventListener (preserves ASP.NET Core EventCounter metrics not yet available via Diagnostics on < .NET 8) | | Any | `false` | Disabled | | Any | `OTEL_METRICS_EXPORTER=none` | Disabled (takes precedence over the default) | **Note for .NET 6/7 users** who explicitly set `DD_RUNTIME_METRICS_ENABLED=true` and want to opt into the new Diagnostics-based collector (at the cost of losing ASP.NET Core EventCounter metrics): also set `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED=true`. ## Test coverage - Updated conditional `[InlineData]` in `RuntimeMetricsEnabled` test to expect `true` when unset on .NET 6+. - Added `RuntimeMetrics_DefaultsToDignosticsOnNet6Plus_WhenNotExplicitlySet`. - Added `RuntimeMetrics_ExplicitEnable_RespectsExplicitDiagnosticsFlag` (explicit `true`/`false` cases). - Added `RuntimeMetrics_ExplicitEnable_DefaultsToDiagnosticsOnNet8Plus` (net8.0 TFM). - Added `RuntimeMetrics_ExplicitEnable_DefaultsToEventListenerOnNet6And7` (net6.0/net7.0 TFMs). - Added `RuntimeMetrics_ExplicitDisable_OverridesDefault`. - Added RuntimeMetrics_InvalidValue_TreatedAsUnset_DefaultsToDiagnostics (codifies that invalid values like DD_RUNTIME_METRICS_ENABLED=blah are treated as unset, not as explicitly configured) ## Other details Part of [Enabling .NET Runtime Metrics by Default](https://docs.google.com/document/d/1tekvCvlOkn12pU3jLK-ePZ0nrynSW7p8IWgG-Dh7zwQ/edit?pli=1&tab=t.0#heading=h.hz8udk7cv9zf). Should land after the PRs below are merged which include necessary fixes: - https://github.com/DataDog/dd-trace-dotnet/pull/8265 - https://github.com/DataDog/dd-trace-dotnet/pull/8266 ### Moving forward After this lands, monitor: 1. **Telemetry**: `DD_RUNTIME_METRICS_ENABLED` and `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED` usage to track adoption of the new defaults. 2. **Support tickets**: Customers reporting missing metrics on .NET 6/7 (`contention_time`, ASP.NET Core counters, `compacting_gc` tag) after upgrading. 3. **Future**: Consider defaulting `DD_RUNTIME_METRICS_DIAGNOSTICS_METRICS_API_ENABLED=true` for all .NET 6+ in a later release once the .NET 6/7 EventCounter gap is documented and communicated. * Bump test dependencies and add test.final_status to benchmark exporter (#8301) ## Summary - Bump `DatadogTestCollector` from 0.0.52 to 0.0.53 - Bump `DatadogTestLogger` from 0.0.52 to 0.0.53 - Add `test.final_status` tag to the benchmark exporter (`DatadogExporter`) - Bump `timeitsharp` from 0.4.7 to 0.4.8 ## Test plan - [ ] CI passes with the updated packages - [ ] Benchmark tests emit `test.final_status` tag correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <[email protected]> * [Test Package Versions Bump] Updating package versions (#8306) Updates the package versions for integration tests. Co-authored-by: andrewlock <[email protected]> * Only allow creating hotfix branches from a tag (#8297) ## Summary of changes Ensures you're creating a hotfix branch from a tag ## Reason for change The "correct" usage is to create a hotfix from a previous normal release, or from a previous hotfix. The easy way to enforce that is to ensure you create from a tag ## Implementation details Check the reference passed in, like we do for other workflows ## Test coverage Tested it [here](https://github.com/DataDog/dd-trace-dotnet/actions/runs/23005198182) and it failed (as expected). * ServiceNameSource for inferred proxy (#8309) ## Summary of changes Pass explicit `serviceNameSource` for inferred proxy span factories so `_dd.svc_src` correctly identifies the integration that set the service name. ## Reason for change Per the service name source [RFC](https://docs.google.com/document/d/1c47iSTWxIOHMHfZTF2nT9xfyQaIBP9KJvI9sRn5SvpM/edit?tab=t.0), integration-driven service overrides must set `_dd.svc_src` to the integration name. The proxy factories were passing `serviceName` without `serviceNameSource`, leaving the tag unset. ## Implementation details - `AwsApiGatewaySpanFactory`: passes `serviceNameSource: "aws-apigateway"` - `AzureApiManagementSpanFactory`: passes `serviceNameSource: "azure-apim"` ## Test coverage Updated 30 snapshot files to include the new `_dd.svc_src` tag on inferred proxy spans. ## Other details Part of the `_dd.svc_src` RFC implementation. Manual instrumentation source (`"m"`) is handled in a separate PR. <!-- Fixes #{issue} --> <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. --> * [Profiler] Add unit tests for ManagedCodeCache (#8307) * Add some documentation on running system-tests (#8298) ## Summary of changes This adds a brief document on how to test PR changes in/against system-tests ## Reason for change Feedback on onboarding into the repository where it wasn't obvious how to run system-tests (it isn't great) ## Implementation details Searched for `system-tests.git` in `ultimate-pipeline.yaml` and outlined changing the branch Looked at the `docker_image_artifacts` label (introduced in https://github.com/DataDog/dd-trace-dotnet/pull/7337) and documented that ## Test coverage None! ## Other details <!-- Fixes #{issue} --> I haven't _really_ done this much so it may be wrong. We have some documentation on running locally but last time I did that (~6 mos ago) it didn't work 😭 <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. --> * Adjust Log levels in OltpExporter (#8133) ## Summary of changes Changes the log levels in `OtlpExporter` to skip telemetry for some errors (while making them go from Debug to Error) and swapping some Debug ## Reason for change Noticed a lot of new errors but they just seem to be transient networking errors. https://app.datadoghq.com/error-tracking/issue/443da854-fe18-11f0-ae87-da7ad0900002 Some of the current logs were either the wrong level or absent. ## Implementation details - Bad Request (400) - not retrying - swapped to Error from Warning - Add two `Log.ErrorSkipTelemetry("An error occurred while sending OTLP request to {AgentEndpoint}.` for when we get transient network issue - `Log.Error(ex, "Error sending OTLP request (attempt {Attempt})", (attempt + 1).ToString());` -> `Log.Debug<int>(ex, "Error sending OTLP request (attempt {Attempt})", attempt + 1);` along with removing the string allocation ## Test coverage ## Other details <!-- Fixes #{issue} --> <!-- ⚠️ Note: Where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. MergeQueue is NOT enabled in this repository. If you have write access to the repo, the PR has 1-2 approvals (see above), and all of the required checks have passed, you can use the Squash and Merge button to merge the PR. If you don't have write access, or you need help, reach out in the #apm-dotnet channel in Slack. --> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> * Refactor log directory resolution into smaller methods, add tests (#7255) ## Summary of changes Refactor `DatadogLoggingFactory` log directory resolution logic into smaller, testable methods and add unit tests. ## Reason for change The log directory resolution logic was a single large method mixing concerns: - config reading - default directory lookup - directory creation 😦 - fallback logic Breaking it apart makes each piece independently testable and easier to understand. ## Implementation details - Extract `GetDefaultLogDirectory()` — determines the _default_ log directory based on platform and Azure environment - Extract `GetProgramDataDirectory()` — resolves the `ProgramData` path on Windows (with Nano Server fallbacks) - Extract `TryCreateLogDirectory()` — attempts to _create_ a log directory, returning success/failure - Make extracted methods `internal` with `[TestingAndPrivateOnly]` for testability Simplify `GetLogDirectory()` to orchestrate the above methods with clear fallback chain: 1. `DD_TRACE_LOG_DIRECTORY` env var 2. `DD_TRACE_LOG_PATH` (deprecated, directory extracted from file path) 3. `GetDefaultLogDirectory()` (platform/Azure-aware default) 4. `Path.GetTempPath()` (last resort) ## Test coverage Flattened nested test classes (`FileLoggingConfiguration`, `RedactedLogConfiguration`, `SinkConfiguration`) into top-level methods with consistent `MethodUnderTest_Scenario_ExpectedBehavior` naming: | Old (nested class.method) | New (flat method) | |---|---| | `FileLoggingConfiguration.UsesLogDirectoryWhenItExists` | `GetConfiguration_WithLogDirectory_UsesLogDirectory` | | `FileLoggingConfiguration.UsesObsoleteLogDirectoryWhenAvailable` | `GetConfiguration_WithObsoleteLogPath_UsesObsoleteLogDirectory` | | `FileLoggingConfiguration.UsesEnvironmentFallBackWhenBothNull` | `GetConfiguration_WithNoLogDirectorySettings_UsesEnvironmentFallback` | | `FileLoggingConfiguration.CreatesLogDirectoryWhenItDoesntExist` | `GetConfiguration_WithNonExistentLogDirectory_CreatesDirectory` | | `RedactedLogConfiguration.WhenNoOrInvalidConfiguration_TelemetryLogsEnabled` | `GetConfiguration_WithNoOrInvalidTelemetryLogsSetting_EnablesErrorLogging` | | `RedactedLogConfiguration.WhenDisabled_TelemetryLogsDisabled` | `GetConfiguration_WithTelemetryLogsDisabled_DisablesErrorLogging` | | `SinkConfiguration.WhenNoSinksProvided_UsesFileSink` | `GetConfiguration_WithNoSinksSetting_UsesFileSink` | | `SinkConfiguration.WhenFileSinkIsIncluded_UsesFileSink` | `GetConfiguration_WithFileSinkIncluded_UsesFileSink` | | `SinkConfiguration.WhenFileSinkIsNotIncluded_DoesNotUseFileSink` | `GetConfiguration_WithFileSinkNotIncluded_DoesNotUseFileS…
Summary of changes
Fix sync-over-async patterns in the vendored DogStatsD client and add async disposal for shutdown.
Reason for change
NamedPipeTransport.SendBuffer()callsWriteAsync().Wait()onNamedPipeClientStreamwhich leads to sync-over-async that can deadlock under thread pool starvation.AsynchronousWorker.Dispose()also blocks withworker.Wait()on LongRunning tasks, making shutdown slow when the agent is unreachable.Part of Enabling .NET Runtime Metrics by Default. This should land before we enable Runtime Metrics by default since it hardens the shared DogStatsD client that runtime metrics uses.
Implementation details
NamedPipeTransport: ReplaceWriteAsync().Wait()with synchronousWrite(). This is a worker thread sending metrics, so sync is the right fit.DisposeAsync()through the disposal chain:AsynchronousWorker->StatsBufferize->StatsdData->DogStatsdService->StatsdManager.StatsdClientHolderuses aTaskCompletionSourceto signal when async disposal finishes, bridging the syncRelease()path and the async shutdown path.TracerManager.RunShutdownTasksAsyncnow awaitsStatsdManager.DisposeAsync()instead of synchronousDispose().Test coverage
Existing tests still pass 🤞.
Other details