Stop using Tracer.Instance if we don't need to
#7744
+3,991
−1,532
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
andrewlock
added
area:tracer
andrewlock
commented
Oct 30, 2025
| CommandType = typeof(TCommand); | ||
|
|
||
| if (TryGetIntegrationDetails(CommandType.FullName, out var integrationId, out var dbTypeName)) | ||
| if (TryGetIntegrationDetails(Tracer.Instance.Settings.DisabledAdoNetCommandTypes, CommandType.FullName, out var integrationId, out var dbTypeName)) |
Member
Author
There was a problem hiding this comment.
This one is actually kind of annoying. As it's in a static cctor, we can't pass it in anywhere, which means there's one DbScopeFactoryTests test which will use this and end up initializing the global tracer. Oh well.
Collaborator
There was a problem hiding this comment.
:sorry: pretty sure this was me
Contributor
There was a problem hiding this comment.
Hmm. I understand that we want to minimize work but I feel like we should calculate all DbCommands regardless of config (i.e. no dependency here) and then we do the config check on the hot path, which allows this setting to be hot-reloadable or "config at runtime" ready
Contributor
There was a problem hiding this comment.
Concretely, I'm suggesting that we remove the TryGetIntegrationDetails call in the static constructor of Cache<TCommand> and refactor the other call in CreateDbCommandScope a bit. Perhaps in a separate PR
Member
Author
There was a problem hiding this comment.
I can see arguments both ways tbh.
The only downside to @zacharycmontoya's proposal would be more work to do for custom DbCommands (ones that we don't explicitly know about), which are also marked as disabled initially, and which customers have marked as disabled. The question I would have is whether that's ok - e.g. if they're disabled because we're erroring out literally running TryGetIntegrationDetails, and that's why they disabled it, then that could be a problem. If it's just an optimization, then yes, it probably is fine.
That said, this actually is technically already hot-reloadable. The DisabledAdoNetCommandTypes setting passed in here is used to decide whether to populate the cache for custom types, but if we don't populate the cache, then we run TryGetIntegrationDetails() and check DisabledAdoNetCommandTypes at runtime anyway. So if this changes, then the result changes, it will just never be cached in that scenario.
So on that basis, I think removing the TryGetIntegrationDetails has a small amount of risk (if customer disabled adonet due to errors - not sure if that's really possible, but I can believe it), means a small bit more work (calculating values which will never be used), and doesn't change how hot-reloadable we are in general (though if we made it hot reloadable, we'd have to rethink how caching works). The plus side is it removes the static access to Tracer.Instance which is better for tests.
So yeah, I'm torn 🤷♂️ I'd say separate PR either way, given the questions, as the current behaviour in this PR is identical to the existing. But it's worth thinking about
|
bouwkast
approved these changes
Oct 30, 2025
| CommandType = typeof(TCommand); | ||
|
|
||
| if (TryGetIntegrationDetails(CommandType.FullName, out var integrationId, out var dbTypeName)) | ||
| if (TryGetIntegrationDetails(Tracer.Instance.Settings.DisabledAdoNetCommandTypes, CommandType.FullName, out var integrationId, out var dbTypeName)) |
Collaborator
There was a problem hiding this comment.
:sorry: pretty sure this was me
| [Fact] | ||
| public void CanRoundTripPathwayContext() | ||
| [Theory] | ||
| [CombinatorialData] |
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7744) 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 (7744) - mean (75ms) : 69, 81
master - mean (76ms) : 70, 83
section Bailout
This PR (7744) - mean (78ms) : 73, 84
master - mean (79ms) : 73, 85
section CallTarget+Inlining+NGEN
This PR (7744) - mean (1,056ms) : 1006, 1105
master - mean (1,067ms) : 976, 1158
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 (7744) - mean (118ms) : 111, 125
master - mean (118ms) : 111, 125
section Bailout
This PR (7744) - mean (119ms) : 113, 126
master - mean (118ms) : 111, 125
section CallTarget+Inlining+NGEN
This PR (7744) - mean (764ms) : 732, 796
master - mean (758ms) : 720, 796
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7744) - mean (105ms) : 100, 110
master - mean (105ms) : 98, 111
section Bailout
This PR (7744) - mean (107ms) : 101, 113
master - mean (107ms) : 100, 114
section CallTarget+Inlining+NGEN
This PR (7744) - mean (722ms) : 684, 761
master - mean (719ms) : 677, 762
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7744) - mean (105ms) : 99, 111
master - mean (106ms) : 98, 114
section Bailout
This PR (7744) - mean (108ms) : 99, 117
master - mean (105ms) : 98, 112
section CallTarget+Inlining+NGEN
This PR (7744) - mean (697ms) : 669, 725
master - mean (690ms) : 661, 720
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 (7744) - mean (196ms) : 192, 200
master - mean (195ms) : 192, 198
section Bailout
This PR (7744) - mean (201ms) : 197, 204
master - mean (198ms) : 196, 201
section CallTarget+Inlining+NGEN
This PR (7744) - mean (1,169ms) : 1104, 1234
master - mean (1,129ms) : 1071, 1188
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 (7744) - mean (291ms) : 278, 304
master - mean (280ms) : 276, 285
section Bailout
This PR (7744) - mean (286ms) : 279, 292
master - mean (281ms) : 276, 287
section CallTarget+Inlining+NGEN
This PR (7744) - mean (934ms) : 900, 968
master - mean (924ms) : 880, 969
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7744) - mean (277ms) : 270, 283
master - mean (274ms) : 267, 280
section Bailout
This PR (7744) - mean (282ms) : 269, 296
master - mean (275ms) : 270, 279
section CallTarget+Inlining+NGEN
This PR (7744) - mean (911ms) : 875, 948
master - mean (902ms) : 860, 944
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7744) - mean (279ms) : 271, 286
master - mean (273ms) : 267, 278
section Bailout
This PR (7744) - mean (279ms) : 271, 287
master - mean (273ms) : 269, 276
section CallTarget+Inlining+NGEN
This PR (7744) - mean (853ms) : 805, 901
master - mean (839ms) : 813, 865
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
zacharycmontoya
approved these changes
Oct 30, 2025
andrewlock
force-pushed
the
andrew/settings/6a-stop-using-tracer-instance
branch
from
8a3aee6 to
753da8d
Compare
andrewlock
force-pushed
the
andrew/settings/5-remove-mutablesettings
branch
from
1b1f494 to
f94e51b
Compare
andrewlock
force-pushed
the
andrew/settings/6a-stop-using-tracer-instance
branch
from
753da8d to
3a96423
Compare
andrewlock
force-pushed
the
andrew/settings/5-remove-mutablesettings
branch
from
f94e51b to
75937f5
Compare
andrewlock
force-pushed
the
andrew/settings/6a-stop-using-tracer-instance
branch
from
3a96423 to
f01f8c6
Compare
andrewlock
force-pushed
the
andrew/settings/5-remove-mutablesettings
branch
from
75937f5 to
8111379
Compare
andrewlock
force-pushed
the
andrew/settings/6a-stop-using-tracer-instance
branch
from
f01f8c6 to
4fd8303
Compare
andrewlock
force-pushed
the
andrew/settings/5-remove-mutablesettings
branch
from
8111379 to
177c2a0
Compare
Also: - slight refactor of LogFormatter to reduce some allocation - ignore "previous" when creating DirectLogSubmissionManager (seeing as that won't be a thing soon)
…n't respond to changes I left it like this because the debugger already doesn't respond to changes like other services do
- Move statsd instance creation to separate factory - Create a StatsdManager to handle automatic updating in response to setting changes - Always create a statsd instance, as it's hard to know if we're _ever_ going to need one, and reduces some of the compexity
… reconfiguration is not allowed
…s though, and doesn't respond to changes
This isn't necessary with the current design, and it causes issues today
Make sure we can't dispose a stats consumer that's in use (as it will throw) Rework to use a "lease" mechanism to track usages Make passing in a statsmanager required
The statsd client does sync-over-async in the flush and dispose paths, which can lead to deadlocks and thread exhaustion. To work around that, we push the dispose to happen on a thread-pool thread instead, in the background
… config changes (#7796) ## Summary of changes A fix for #7724 to handle telemetry reporting in dynamic config "reset" scenarios ## Reason for change The system tests for #7724 were failing in some dynamic configuration scenarios. Specifically, the tests were sending remote config _without_ any configuration values "i.e. 'reset to use defaults'" and were waiting a telemetry update. However, we never sent it, because there was "no telemetry to record". Note that we _did_ correctly apply the new configuration, we just didn't report the telemetry correctly, primarily due to limitations in the telemetry protocol. This PR adds a fix for that, and will be merged into #7724. ## Implementation details The solution is to "remember" the telemetry from the default mutable configuration values, _without_ any dynamic sources, and "replay" this telemetry when we update telemetry. This feels kind of hacky, but it's something I suspected we might need to do, and had been avoiding up to this point because we do a "full reconfigure" anyway. ## Test coverage Added a specific unit test that mimics the behaviour of the system-test (i.e. an "empty" dynamic config response) and confirms the telemetry is recorded as expected ## Other details https://datadoghq.atlassian.net/browse/LANGPLAT-819 Part of a config stack - #7522 - #7525 - #7530 - #7532 - #7543 - #7544 - #7721 - #7722 - #7695 - #7723 - #7724 - #7796 👈 Unlike other PRs in the stack, I'll merge this directly into #7724 to fix the tests there, just thought I'd keep this separate for easier reviewing
andrewlock
force-pushed
the
andrew/settings/6a-stop-using-tracer-instance
branch
from
efe95e4 to
0e2c7d3
Compare
andrewlock
force-pushed
the
andrew/settings/5-remove-mutablesettings
branch
from
447027a to
9db67e4
Compare
Base automatically changed from
andrew/settings/5-remove-mutablesettings
to
master
November 26, 2025 18:21
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
Updates a couple of places where we're calling
Tracer.Instancewhere we don't need toReason for change
In one of my other PRs I accidentally broke something that should only have affected integration tests, but a bunch of unit tests broke. It highlighted where they were using
Tracer.Instanceand setting the global tracer for tests. In other places, it revealed that code that tests that looked independent of other tests really wasn't...Calling
Tracer.Instancepotentially does a lot of work, as it initializes the tracer. It's also hard to follow if we're making static calls out in places we don't need to. So just pass through the settings we want instead.Implementation details
Instead of calling
Tracer.Instance.Settings, use the value ofTracerorTracerSettingsthat's already available wherever possible. It makes the tests cleaner too.Test coverage
Same coverage, just a bit cleaner
Other details
Included as part of the config stack, just because I already refactored some of this code, and can't be bothered to faff with merge conflicts:
https://datadoghq.atlassian.net/browse/LANGPLAT-819
MutableSettingsfromTracerSettings#7522MutableSettingson dynamic config changes #7525DefaultServiceNametoMutableSettings#7530PerTraceSettings.GetServiceName()#7532TracerSettingsto useMutableSettingswhere appropriate #7543IsIntegrationEnabled(),IsErrorStatusCode(), andGetIntegrationAnalyticsSampleRate()#7544DictionaryExtensions.SequenceEqual#7722SettingsManagerfor managing mutable settings and ExporterSettings #7695TracerSettingswhich can change at runtime #7723MutableSettingsandExporterSettingswithout rebuilding #7724Tracer.Instanceif we don't need to #7744 👈