[tracing] add support for DiagnosticSource (and Quartz) in .NET Framework
#7687
+731
−175
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
chojomok
changed the title
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7687) 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 (7687) - mean (69ms) : 67, 70
master - mean (68ms) : 67, 70
section Bailout
This PR (7687) - mean (72ms) : 71, 73
master - mean (72ms) : 71, 74
section CallTarget+Inlining+NGEN
This PR (7687) - mean (1,007ms) : 967, 1047
master - mean (1,013ms) : 948, 1078
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 (7687) - mean (106ms) : 103, 109
master - mean (105ms) : 103, 108
section Bailout
This PR (7687) - mean (107ms) : 106, 108
master - mean (107ms) : 105, 108
section CallTarget+Inlining+NGEN
This PR (7687) - mean (757ms) : 738, 777
master - mean (735ms) : 667, 804
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7687) - mean (93ms) : 91, 95
master - mean (93ms) : 91, 95
section Bailout
This PR (7687) - mean (94ms) : 93, 96
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7687) - mean (716ms) : 697, 736
master - mean (708ms) : 671, 745
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7687) - mean (92ms) : 90, 94
master - mean (92ms) : 90, 94
section Bailout
This PR (7687) - mean (93ms) : 92, 94
master - mean (93ms) : 92, 94
section CallTarget+Inlining+NGEN
This PR (7687) - mean (633ms) : 617, 650
master - mean (634ms) : 617, 650
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 (7687) - mean (198ms) : 192, 203
master - mean (194ms) : 190, 198
section Bailout
This PR (7687) - mean (198ms) : 194, 202
master - mean (197ms) : 193, 201
section CallTarget+Inlining+NGEN
This PR (7687) - mean (1,130ms) : 1070, 1191
master - mean (1,112ms) : 1054, 1170
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 (7687) - mean (278ms) : 272, 285
master - mean (276ms) : 272, 281
section Bailout
This PR (7687) - mean (283ms) : 276, 290
master - mean (277ms) : 274, 280
section CallTarget+Inlining+NGEN
This PR (7687) - mean (945ms) : 928, 963
master - mean (925ms) : 883, 967
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7687) - mean (271ms) : 265, 277
master - mean (270ms) : 266, 274
section Bailout
This PR (7687) - mean (271ms) : 267, 276
master - mean (270ms) : 267, 273
section CallTarget+Inlining+NGEN
This PR (7687) - mean (939ms) : 914, 963
master - mean (931ms) : 898, 964
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7687) - mean (270ms) : 264, 275
master - mean (269ms) : 264, 275
section Bailout
This PR (7687) - mean (270ms) : 265, 274
master - mean (270ms) : 266, 274
section CallTarget+Inlining+NGEN
This PR (7687) - mean (829ms) : 808, 851
master - mean (822ms) : 807, 837
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2025-12-30 21:15:35 Comparing candidate commit 69c36c8 in PR branch Found 9 performance improvements and 6 performance regressions! Performance is the same for 161 metrics, 10 unstable metrics. scenario:Benchmarks.Trace.ActivityBenchmark.StartStopWithChild net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark netcoreapp3.1
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net472
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishTwoScopes netcoreapp3.1
|
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 1 occurrences of : - Name: internal,
- Resource: Quartz.Job.Execute,
+ Name: quartz.job.execute,
+ Resource: execute exceptionJob,
[...]
+ Error: 1,
[...]
+ error.msg: Expected InvalidOperationException thrown,
+ error.stack:
[...]
+ error.type: Quartz.JobExecutionException,
[...]
- job.group: group1,
- job.name: helloJob,
- job.type: QuartzSampleApp.Jobs.HelloJob,
+ job.group: group2,
+ job.name: exceptionJob,
+ job.type: QuartzSampleApp.Jobs.ExceptionJob,
[...]
- otel.status_code: STATUS_CODE_UNSET,
+ otel.status_code: STATUS_CODE_ERROR,
[...]
- trigger.group: group1,
- trigger.name: helloTrigger,
+ trigger.group: group2,
+ trigger.name: exceptionTrigger,
1 occurrences of : - Name: internal,
- Resource: Quartz.Job.Execute,
+ Name: quartz.job.execute,
+ Resource: execute helloJob,
[...]
- Error: 1,
[...]
- error.msg: Expected InvalidOperationException thrown,
- error.stack:
[...]
- error.type: Quartz.JobExecutionException,
[...]
- job.group: group2,
- job.name: exceptionJob,
- job.type: QuartzSampleApp.Jobs.ExceptionJob,
+ job.group: group1,
+ job.name: helloJob,
+ job.type: QuartzSampleApp.Jobs.HelloJob,
[...]
- otel.status_code: STATUS_CODE_ERROR,
+ otel.status_code: STATUS_CODE_UNSET,
[...]
- trigger.group: group2,
- trigger.name: exceptionTrigger,
+ trigger.group: group1,
+ trigger.name: helloTrigger,
|
chojomok
force-pushed
the
mohammad/diagnosticobserver_netframework_support
branch
from
6cbdd75 to
05709b9
Compare
…netstandard2.0.verified.txt
chojomok
changed the title
| namespace Datadog.Trace.DiagnosticListeners | ||
| { | ||
| /// <summary> | ||
| /// This is code written by Cursor to do the reflection needed for DiagnosticListener. |
Member
There was a problem hiding this comment.
Not a blocker, but I don't think we need to starting adding comments everywhere we generated code with AI assistance.
Member
There was a problem hiding this comment.
Agreed, but I'm not sure we need this type at all 🤔 As far as I can see we already have almost the exact same code in ActivityListener - could/should we unify those?
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Quartz/QuartzCommon.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Quartz/QuartzCommon.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Note for reviewers: The only difference between the tracer/test/snapshots/QuartzTestsV3NETCOREAPP3X.verified.txt and tracer/test/snapshots/QuartzTestsV3NETFRAMEWORK.verified.txt files that will be checked in is the stack trace content stored in the error.stack tag. Everything else is identical
Collaborator
Author
There was a problem hiding this comment.
Thank you for the call out Zach!
I've discussed adding some scrubbing so the snapshots can be the same. Please let me know if this something we'd like to do.
Contributor
There was a problem hiding this comment.
Up to you! If this test becomes flaky then it will definitely be a priority to scrub it, otherwise I don't think it's necessary for this PR
Collaborator
Author
There was a problem hiding this comment.
I'll just leave it as is until we see the test becoming flaky
Contributor
There was a problem hiding this comment.
Note for reviewers: The only difference between the new tracer/test/snapshots/QuartzTestsV3NETCOREAPP3X.verified.txt snapshot and the existing tracer/test/snapshots/QuartzTestsV3.verified.txt (all newer runtimes running against Quartz v3) is that the NETCOREAPP3X snapshot does not have a span.kind: internal tag since System.Diagnostics.Activity does not have a Kind property before .NET 5. The rest of the span contents are identical.
tracer/src/Datadog.Trace/DiagnosticListeners/DiagnosticManager.cs
Outdated
Show resolved
Hide resolved
andrewlock
reviewed
Nov 12, 2025
| <type fullname="System.Diagnostics.ActivityIdFormat" /> | ||
| <type fullname="System.Diagnostics.ActivitySpanId" /> | ||
| <type fullname="System.Diagnostics.ActivityTraceId" /> | ||
| <type fullname="System.Diagnostics.DiagnosticListener" /> |
Member
There was a problem hiding this comment.
This automatic removal is actually a problem, as it means the type could be trimmed from the target application, even though we need it 🤔 Overall, I think that can be "fixed" by not relying on reflection in .NET Core (which i think we shouldn't do anyway)
Collaborator
Author
There was a problem hiding this comment.
this has been reverted now with adding the directive to use the built in class for !NETFRAMEWORK.
| namespace Datadog.Trace.DiagnosticListeners | ||
| { | ||
| /// <summary> | ||
| /// This is code written by Cursor to do the reflection needed for DiagnosticListener. |
Member
There was a problem hiding this comment.
Agreed, but I'm not sure we need this type at all 🤔 As far as I can see we already have almost the exact same code in ActivityListener - could/should we unify those?
Comment on lines
+48
to
+50
| try | ||
| { | ||
| // Get the DiagnosticListener type |
Member
There was a problem hiding this comment.
I don't think we should be using reflection here in .NET Core + .NET Standard when these types are already available 🤔 Can we #if this so that we only take the expensive path in .NET Framework, and we use the optimized version elsewhere?
Collaborator
Author
There was a problem hiding this comment.
@zacharycmontoya and I worked on adding the directive to use the builtin class instead of doing reflection from !NETFRAMEWORK.
Collaborator
Author
There was a problem hiding this comment.
I'm going to look into make the factory class more generic so it can be used for ActivityListener.
Collaborator
Author
There was a problem hiding this comment.
@andrewlock - made a factory class that generates the IL for a static and a instance Subscribe call. There is some reusability in the no-op methods needed in the reflection that I move to methods.
Please let me know your feedback!
lucaspimentel
changed the title
DiagnosticSource (and Quartz) in .NET Framework
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Quartz/QuartzCommon.cs
Show resolved
Hide resolved
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Quartz/QuartzCommon.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Quartz/QuartzCommon.cs
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
To create a diagnostic observer for .NET Framework without requiting any additional dependencies.
Relying on Reflection to create an Observer instead of using the already included DiagnosticSource to have a unified method across both Core and Framework.
There is also enhancements to the spans created from OTEL for < .NET 5.0 to match the spans created versions above .NET 5.
Reason for change
To broaden support for Tracing Integrations that use the DiagnosticObserver class. Currently it's used by
Implementation details
DiagnosticSource
I'll go over all the changes and the reasoning behind this.
DiagnosticManager
IObserver<DiagnosticListener>toIObserver<IDiagnosticListener>IDiagnosticListenerDiagnosticObserver
public virtual IDisposable SubscribeIfMatch(DiagnosticListener diagnosticListener)topublic virtual IDisposable SubscribeIfMatch(IDiagnosticListener diagnosticListener)DiagnosticListenerObserverFactory
DiagnosticListenerAllListenersdefined in theDiagnosticListenerclass, but the type for this method ispublic static IObservable<System.Diagnostics.DiagnosticListener> AllListeners { get; }, creating a circular dependency forDiagnosticListener._allListenersSubscription = DiagnosticListener.AllListeners.Subscribe(this);thisin the object in the subscribe method, we can't directly callAllListeners.Subscribe, we need to ducktyped. ButDiagnosticListener.AllListenersis a static property - there's no instance to ducktype.DiagnosticListenertype without directly referencing itAllListenerspropertyGeneric Invariance Problem (The Main Issue )
This is the bigger challenge. Here's the type mismatch:
DISCLAIMER: The following explanation is from Cursor which I used to have it explain why the original method of using ducktyping didn't work - this information an be wrong but this is what I observed during testing.
Generic interfaces in C# are invariant - meaning
IObserver<DiagnosticListener>andIObserver<IDiagnosticListener>are completely incompatible types, even thoughIDiagnosticListeneris a ducktype ofDiagnosticListener.When you tried to subscribe directly:
The runtime threw:
Unable to cast object of type 'DiagnosticManager' to type 'System.IObserver<System.Diagnostics.DiagnosticListener>'Why Ducktyping Alone Couldn't Solve This:
Even if we ducktyped the observable, the
Subscribemethod signature still expects the exact generic typeIObserver<DiagnosticListener>. Ducktyping can't bridge generic variance - it's a compile-time/runtime limitation of how generics work in .NET.The Solution: Dynamic Type Generation
We used
Reflection.Emitto dynamically create a type at runtime that:IObserver<DiagnosticListener>using the actual runtime typeDiagnosticListenerobjects (the real type)IDiagnosticListenerDiagnosticManagerDiagnosticListenerObserverFactory - AI Generated Code
Test coverage
Other details