Add ability to disable ActivitySources from being listened to
#5795
Conversation
This file contains 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
github-actions
bot
added
the
area:tracer
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-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 shown 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). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5795) - mean (70ms) : 68, 73
. : milestone, 70,
master - mean (70ms) : 67, 72
. : milestone, 70,
section CallTarget+Inlining+NGEN
This PR (5795) - mean (1,110ms) : 1086, 1134
. : milestone, 1110,
master - mean (1,110ms) : 1085, 1135
. : milestone, 1110,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5795) - mean (108ms) : 104, 113
. : milestone, 108,
master - mean (109ms) : 105, 112
. : milestone, 109,
section CallTarget+Inlining+NGEN
This PR (5795) - mean (767ms) : 755, 779
. : milestone, 767,
master - mean (771ms) : 757, 786
. : milestone, 771,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5795) - mean (92ms) : 90, 94
. : milestone, 92,
master - mean (92ms) : 89, 95
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (5795) - mean (722ms) : 709, 735
. : milestone, 722,
master - mean (727ms) : 710, 744
. : milestone, 727,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5795) - mean (190ms) : 186, 193
. : milestone, 190,
master - mean (189ms) : 186, 192
. : milestone, 189,
section CallTarget+Inlining+NGEN
This PR (5795) - mean (1,202ms) : 1176, 1227
. : milestone, 1202,
master - mean (1,201ms) : 1179, 1222
. : milestone, 1201,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5795) - mean (274ms) : 269, 279
. : milestone, 274,
master - mean (274ms) : 269, 278
. : milestone, 274,
section CallTarget+Inlining+NGEN
This PR (5795) - mean (938ms) : 918, 959
. : milestone, 938,
master - mean (943ms) : 926, 960
. : milestone, 943,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5795) - mean (264ms) : 260, 268
. : milestone, 264,
master - mean (263ms) : 259, 267
. : milestone, 263,
section CallTarget+Inlining+NGEN
This PR (5795) - mean (926ms) : 906, 946
. : milestone, 926,
master - mean (924ms) : 908, 940
. : milestone, 924,
|
Benchmarks Report for tracer 🐌Benchmarks for #5795 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 | 1.181 | 543.13 | 641.64 |
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.175 | 479.65 | 408.23 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 480ns | 0.291ns | 1.13ns | 0.00816 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 543ns | 0.459ns | 1.78ns | 0.00788 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 685ns | 0.476ns | 1.84ns | 0.0918 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 575ns | 0.29ns | 1.12ns | 0.00978 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 757ns | 0.357ns | 1.38ns | 0.00949 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 908ns | 0.464ns | 1.8ns | 0.105 | 0 | 0 | 658 B |
| #5795 | StartFinishSpan |
net6.0 | 408ns | 0.141ns | 0.548ns | 0.00797 | 0 | 0 | 576 B |
| #5795 | StartFinishSpan |
netcoreapp3.1 | 641ns | 0.713ns | 2.76ns | 0.00789 | 0 | 0 | 576 B |
| #5795 | StartFinishSpan |
net472 | 686ns | 0.533ns | 2.07ns | 0.0918 | 0 | 0 | 578 B |
| #5795 | StartFinishScope |
net6.0 | 564ns | 0.219ns | 0.818ns | 0.0097 | 0 | 0 | 696 B |
| #5795 | StartFinishScope |
netcoreapp3.1 | 733ns | 0.418ns | 1.62ns | 0.00934 | 0 | 0 | 696 B |
| #5795 | StartFinishScope |
net472 | 849ns | 0.679ns | 2.63ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 661ns | 0.901ns | 3.37ns | 0.00967 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 917ns | 0.471ns | 1.76ns | 0.00917 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.19μs | 0.895ns | 3.35ns | 0.104 | 0 | 0 | 658 B |
| #5795 | RunOnMethodBegin |
net6.0 | 601ns | 0.256ns | 0.992ns | 0.00982 | 0 | 0 | 696 B |
| #5795 | RunOnMethodBegin |
netcoreapp3.1 | 976ns | 1.15ns | 4.46ns | 0.00929 | 0 | 0 | 696 B |
| #5795 | RunOnMethodBegin |
net472 | 1.13μs | 0.561ns | 2.17ns | 0.104 | 0 | 0 | 658 B |
Datadog ReportBranch report: ✅ 0 Failed, 362976 Passed, 2071 Skipped, 14h 55m 57.67s Total Time |
bouwkast
force-pushed
the
steven/disable-otel-integrations
branch
from
87fbe4e to
eda5838
Compare
bouwkast
changed the title
ActivitySources from being listened to
bouwkast
force-pushed
the
steven/disable-otel-integrations
branch
from
eda5838 to
df96756
Compare
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5795) (11.089M) : 0, 11089168
master (11.252M) : 0, 11251784
benchmarks/2.9.0 (11.081M) : 0, 11080577
section Automatic
This PR (5795) (7.359M) : 0, 7359058
master (7.466M) : 0, 7465753
benchmarks/2.9.0 (7.732M) : 0, 7732233
section Trace stats
master (7.766M) : 0, 7765791
section Manual
master (11.114M) : 0, 11114354
section Manual + Automatic
This PR (5795) (6.826M) : 0, 6826031
master (6.846M) : 0, 6845872
section DD_TRACE_ENABLED=0
master (10.389M) : 0, 10388612
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5795) (9.725M) : 0, 9724710
master (9.539M) : 0, 9539093
benchmarks/2.9.0 (9.798M) : 0, 9798067
section Automatic
This PR (5795) (6.440M) : 0, 6439529
master (6.648M) : 0, 6647799
section Trace stats
master (6.855M) : 0, 6855301
section Manual
master (9.525M) : 0, 9525425
section Manual + Automatic
This PR (5795) (6.293M) : 0, 6292650
master (6.161M) : 0, 6160600
section DD_TRACE_ENABLED=0
master (8.832M) : 0, 8832117
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5795) (9.994M) : 0, 9994176
master (10.064M) : 0, 10064242
benchmarks/2.9.0 (10.067M) : 0, 10067315
section Automatic
This PR (5795) (6.655M) : 0, 6654718
master (6.841M) : 0, 6841333
benchmarks/2.9.0 (7.552M) : 0, 7552193
section Trace stats
master (7.427M) : 0, 7426854
section Manual
master (10.084M) : 0, 10084210
section Manual + Automatic
This PR (5795) (6.140M) : 0, 6139715
master (6.348M) : 0, 6347556
section DD_TRACE_ENABLED=0
master (9.577M) : 0, 9576577
|
| /// Default is empty (all ActivitySources will be subscribed to by default). | ||
| /// Supports multiple values separated with commas. | ||
| /// </summary> | ||
| public const string DisabledActivitySources = "DD_DISABLED_ACTIVITY_SOURCES"; |
There was a problem hiding this comment.
We probably want to use the standard DD_TRACE_ prefix here.
(Don't follow DD_DISABLED_INTEGRATIONS above: that's a very old setting from before we had naming conventions. 😅)
Suggested change
| public const string DisabledActivitySources = "DD_DISABLED_ACTIVITY_SOURCES"; | |
| public const string DisabledActivitySources = "DD_TRACE_DISABLED_ACTIVITY_SOURCES"; |
There was a problem hiding this comment.
Don't forget to add this to the configuration telemetry intake's allow-list.
There was a problem hiding this comment.
https://github.com/DataDog/dd-go/pull/148984
Thanks! Changed the name and made that PR (draft at the moment)
Sorry, I didn't accept your suggestion, got carried away locally :)
lucaspimentel
approved these changes
Aug 20, 2024
bouwkast
force-pushed
the
steven/disable-otel-integrations
branch
from
df96756 to
e958ed5
Compare
lucaspimentel
approved these changes
Sep 5, 2024
bouwkast
commented
Sep 17, 2024
tracer/src/Datadog.Trace/Activity/Handlers/DisableActivityHandler.cs
Outdated
Show resolved
Hide resolved
This allows for users to pass in a comma separated list of Activity Source names (glob-pattern supported) that should not be listened to by the Datadog Tracer.
bouwkast
force-pushed
the
steven/disable-otel-integrations
branch
from
c1eafd0 to
f4e4f36
Compare
andrewlock
approved these changes
Oct 28, 2024
| { | ||
| Log.Debug("ActivityListenerHandler: {SourceName} will be handled by {Handler}.", sName, handler); | ||
| return true; | ||
| if (handler is DisableActivityHandler) |
There was a problem hiding this comment.
Rather than checking the type of the handler here, I wonder if it makes more sense to change the return from ShouldListenTo to be an enum instead e.g. something like this (I hate all of these names though!)
public enum ListenState
{
Listen,
Skip, // don't listen, but allow something else to listen
Deny, // don't listen, don't allow anything else to listen
}
Comment on lines
+12
to
+13
| // Disable Activity Handler does not listen to the activity source at all. | ||
| internal static readonly DisableActivityHandler DisableHandler = new(); |
There was a problem hiding this comment.
Is there any value in having a separate DisableActivityHandler vs IgnoreActivityHandler 🤔 Given that the IgnoreActivityHandler causes behaviour changes that it sounds like we don't want, would it make sense to replace IgnoreActivityHandler?
i.e. should we allow people to disable handlers, but also change all our existing "ignored" handlers to be disabled? 🤔
There was a problem hiding this comment.
So for the "Ignored" Activities we do go and update them for context propagation so they are ignored in the sense that we don't create a Datadog span for them, but we still do need to do things as we listen to the source. I think we want to keep that.
The "Disabled" toggles that listener off so we don't know anything about them.
There was a problem hiding this comment.
OK, cool, then I guess my question is: what are the implications if someone disables an activity that's in the "middle" of the trace? What about if it's a "root" activity? Will that mess up our tracing, or will we just have a gap (which is fine) but everything else in terms or parenting etc would work ok? I guess that's somewhat covered by the tests, but it's not entirely clear to me
There was a problem hiding this comment.
Those are good questions
I haven't really looked at this stuff in a long time so I don't know off the top of my head, but I will add test cases to the sample project to test this out.
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
This adds the ability for
ActivitySources to be disabled so that they won't be listened to by the .NET Tracer.Reason for change
Currently there is no way to disable an
ActivitySourceif OpenTelemetry is enabled in the .NET Tracer without entirely disabling OpenTelemetry support. TheIgnoreActivityHandleris insufficient as it will still listen to theActivitySourceso they will createActivityobjects.Implementation details
DD_TRACE_DISABLED_ACTIVITY_SOURCESthat accepts a comma-separated list ofActivitySourcenames that supports the glob-pattern."DD_TRACE_DISABLED_ACTIVITY_SOURCES": "SomeSourceName,*SomeGlobPattern*"DisableActivityHandlerthat will determine based on the above environment variable whether or not a givenActivitySourceshould be listened to.Test coverage
NetActivitySdksample project with an ignoredActivitySourceand some that are disabled.Other details
When merged, get this merged: https://github.com/DataDog/dd-go/pull/148984
Fixes #5740