Skip to content

Stop using Tracer.Instance if we don't need to #7744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: andrew/settings/6-fix-telemetry-reporting
Choose a base branch
from
+28 −25
Open

Stop using Tracer.Instance if we don't need to #7744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
850536a
Make TracerSettings dependency explicit
andrewlock Oct 30, 2025
71c8119
Update DSM to stop using the global tracer instance
andrewlock Oct 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/DbScopeFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ static bool HasDbType(Span span, string dbType)
}

public static bool TryGetIntegrationDetails(
HashSet<string> disabledAdoNetCommandTypes,
string? commandTypeFullName,
[NotNullWhen(true)] out IntegrationId? integrationId,
[NotNullWhen(true)] out string? dbType)
Expand Down Expand Up @@ -172,7 +173,7 @@ public static bool TryGetIntegrationDetails(
return true;
default:
string commandTypeName = commandTypeFullName.Substring(commandTypeFullName.LastIndexOf(".", StringComparison.Ordinal) + 1);
if (IsDisabledCommandType(commandTypeName))
if (IsDisabledCommandType(commandTypeName, disabledAdoNetCommandTypes))
{
integrationId = null;
dbType = null;
Expand Down Expand Up @@ -200,14 +201,14 @@ _ when commandTypeName.EndsWith(commandSuffix) =>
}
}

internal static bool IsDisabledCommandType(string commandTypeName)
internal static bool IsDisabledCommandType(string commandTypeName, HashSet<string> disabledAdoNetCommandTypes)
{
if (string.IsNullOrEmpty(commandTypeName))
{
return false;
}

var disabledTypes = Tracer.Instance.Settings.DisabledAdoNetCommandTypes;
var disabledTypes = disabledAdoNetCommandTypes;

if (disabledTypes is null || disabledTypes.Count == 0)
{
Expand Down Expand Up @@ -245,7 +246,7 @@ static Cache()
{
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))
Copy link
Member Author

@andrewlock andrewlock Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@bouwkast bouwkast Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:sorry: pretty sure this was me

Copy link
Contributor

@zacharycmontoya zacharycmontoya Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@zacharycmontoya zacharycmontoya Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@andrewlock andrewlock Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
// cache values for this TCommand type
DbTypeName = dbTypeName;
Expand Down Expand Up @@ -275,7 +276,7 @@ static Cache()

// if command.GetType() != typeof(TCommand), we are probably instrumenting a method
// defined in a base class like DbCommand and we can't use the cached values
if (TryGetIntegrationDetails(commandType.FullName, out var integrationId, out var dbTypeName))
if (TryGetIntegrationDetails(tracer.Settings.DisabledAdoNetCommandTypes, commandType.FullName, out var integrationId, out var dbTypeName))
{
var operationName = $"{dbTypeName}.query";
var tagsFromConnectionString = GetTagsFromConnectionString(command);
Expand Down
6 changes: 1 addition & 5 deletions tracer/src/Datadog.Trace/DataStreamsMonitoring/DataStreamsContextPropagator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,8 @@ internal class DataStreamsContextPropagator
/// </summary>
/// <param name="context">A <see cref="PathwayContext"/> value that will be propagated into <paramref name="headers"/>.</param>
/// <param name="headers">A <see cref="IHeadersCollection"/> to add new headers to.</param>
/// <param name="isDataStreamsLegacyHeadersEnabled">Are legacy DSM headers enabled</param>
/// <typeparam name="TCarrier">Type of header collection</typeparam>
public void Inject<TCarrier>(PathwayContext context, TCarrier headers)
where TCarrier : IBinaryHeadersCollection
=> Inject(context, headers, Tracer.Instance.Settings.IsDataStreamsLegacyHeadersEnabled);

// Internal for testing
internal void Inject<TCarrier>(PathwayContext context, TCarrier headers, bool isDataStreamsLegacyHeadersEnabled)
where TCarrier : IBinaryHeadersCollection
{
Expand Down
4 changes: 3 additions & 1 deletion tracer/src/Datadog.Trace/DataStreamsMonitoring/DataStreamsManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ internal class DataStreamsManager
private static readonly AsyncLocal<PathwayContext?> LastConsumePathway = new(); // saves the context on consume checkpointing only
private readonly ConcurrentDictionary<string, RateLimiter> _schemaRateLimiters = new();
private readonly IDisposable _updateSubscription;
private readonly bool _isLegacyDsmHeadersEnabled;
private long _nodeHashBase; // note that this actually represents a `ulong` that we have done an unsafe cast for
private bool _isEnabled;
private bool _isInDefaultState;
Expand All @@ -39,6 +40,7 @@ public DataStreamsManager(
{
UpdateNodeHash(tracerSettings.Manager.InitialMutableSettings);
_isEnabled = writer is not null;
_isLegacyDsmHeadersEnabled = tracerSettings.IsDataStreamsLegacyHeadersEnabled;
_writer = writer;
_isInDefaultState = tracerSettings.IsDataStreamsMonitoringInDefaultState;
_updateSubscription = tracerSettings.Manager.SubscribeToChanges(updates =>
Expand Down Expand Up @@ -128,7 +130,7 @@ public void InjectPathwayContext<TCarrier>(PathwayContext? context, TCarrier hea
return;
}

DataStreamsContextPropagator.Instance.Inject(context.Value, headers);
DataStreamsContextPropagator.Instance.Inject(context.Value, headers, _isLegacyDsmHeadersEnabled);
}

public void TrackBacklog(string tags, long value)
Expand Down
18 changes: 10 additions & 8 deletions tracer/test/Datadog.Trace.ClrProfiler.Managed.Tests/DbScopeFactoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ internal void TryGetIntegrationDetails_CorrectNameGenerated(Type commandType, st
var command = (IDbCommand)Activator.CreateInstance(commandType)!;
command.CommandText = DbmCommandText;

bool result = DbScopeFactory.TryGetIntegrationDetails(command.GetType().FullName, out var actualIntegrationId, out var actualDbType);
bool result = DbScopeFactory.TryGetIntegrationDetails([], command.GetType().FullName, out var actualIntegrationId, out var actualDbType);
Assert.True(result);
Assert.Equal(expectedIntegrationName, actualIntegrationId.ToString());
Assert.Equal(expectedDbType, actualDbType);
Expand All @@ -698,12 +698,13 @@ internal void TryGetIntegrationDetails_CorrectNameGenerated(Type commandType, st
[Fact]
internal void TryGetIntegrationDetails_FailsForKnownCommandType()
{
bool result = DbScopeFactory.TryGetIntegrationDetails("InterceptableDbCommand", out var actualIntegrationId, out var actualDbType);
var defaultDisabledCommands = new TracerSettings().DisabledAdoNetCommandTypes;
bool result = DbScopeFactory.TryGetIntegrationDetails(defaultDisabledCommands, "InterceptableDbCommand", out var actualIntegrationId, out var actualDbType);
Assert.False(result);
Assert.False(actualIntegrationId.HasValue);
Assert.Null(actualDbType);

bool result2 = DbScopeFactory.TryGetIntegrationDetails("ProfiledDbCommand", out var actualIntegrationId2, out var actualDbType2);
bool result2 = DbScopeFactory.TryGetIntegrationDetails(defaultDisabledCommands, "ProfiledDbCommand", out var actualIntegrationId2, out var actualDbType2);
Assert.False(result2);
Assert.False(actualIntegrationId2.HasValue);
Assert.Null(actualDbType2);
Expand All @@ -712,18 +713,18 @@ internal void TryGetIntegrationDetails_FailsForKnownCommandType()
[Fact]
internal void TryGetIntegrationDetails_FailsForKnownCommandTypes_AndUserDefined()
{
Tracer.Configure(TracerSettings.Create(new Dictionary<string, object> { { ConfigurationKeys.DisabledAdoNetCommandTypes, "SomeFakeDbCommand" } }));
bool result = DbScopeFactory.TryGetIntegrationDetails("InterceptableDbCommand", out var actualIntegrationId, out var actualDbType);
var disabledCommandTypes = TracerSettings.Create(new() { { ConfigurationKeys.DisabledAdoNetCommandTypes, "SomeFakeDbCommand" } }).DisabledAdoNetCommandTypes;
bool result = DbScopeFactory.TryGetIntegrationDetails(disabledCommandTypes, "InterceptableDbCommand", out var actualIntegrationId, out var actualDbType);
Assert.False(result);
Assert.False(actualIntegrationId.HasValue);
Assert.Null(actualDbType);

bool result2 = DbScopeFactory.TryGetIntegrationDetails("ProfiledDbCommand", out var actualIntegrationId2, out var actualDbType2);
bool result2 = DbScopeFactory.TryGetIntegrationDetails(disabledCommandTypes, "ProfiledDbCommand", out var actualIntegrationId2, out var actualDbType2);
Assert.False(result2);
Assert.False(actualIntegrationId2.HasValue);
Assert.Null(actualDbType2);

bool result3 = DbScopeFactory.TryGetIntegrationDetails("SomeFakeDbCommand", out var actualIntegrationId3, out var actualDbType3);
bool result3 = DbScopeFactory.TryGetIntegrationDetails(disabledCommandTypes, "SomeFakeDbCommand", out var actualIntegrationId3, out var actualDbType3);
Assert.False(result3);
Assert.False(actualIntegrationId3.HasValue);
Assert.Null(actualDbType3);
Expand All @@ -740,7 +741,8 @@ internal void TryGetIntegrationDetails_FailsForKnownCommandTypes_AndUserDefined(
[InlineData("Custom.DB.Command", "AdoNet", "db")]
internal void TryGetIntegrationDetails_CustomCommandType(string commandTypeFullName, string integrationId, string expectedDbType)
{
DbScopeFactory.TryGetIntegrationDetails(commandTypeFullName, out var actualIntegrationId, out var actualDbType);
var defaultDisabledCommands = new TracerSettings().DisabledAdoNetCommandTypes;
DbScopeFactory.TryGetIntegrationDetails(defaultDisabledCommands, commandTypeFullName, out var actualIntegrationId, out var actualDbType);
Assert.Equal(integrationId, actualIntegrationId?.ToString());
Assert.Equal(expectedDbType, actualDbType);
}
Expand Down
14 changes: 8 additions & 6 deletions tracer/test/Datadog.Trace.Tests/DataStreamsMonitoring/DataStreamsContextPropagatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ namespace Datadog.Trace.Tests.DataStreamsMonitoring
{
public class DataStreamsContextPropagatorTests
{
[Fact]
public void CanRoundTripPathwayContext()
[Theory]
[CombinatorialData]
Copy link
Collaborator

@bouwkast bouwkast Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃

public void CanRoundTripPathwayContext(bool isDataStreamsLegacyHeadersEnabled)
{
var oneMs = TimeSpan.FromMilliseconds(1);
var headers = new TestHeadersCollection();
Expand All @@ -25,7 +26,7 @@ public void CanRoundTripPathwayContext()
DateTimeOffset.UtcNow.AddSeconds(-5).ToUnixTimeNanoseconds(),
DateTimeOffset.UtcNow.ToUnixTimeNanoseconds());

DataStreamsContextPropagator.Instance.Inject(context, headers);
DataStreamsContextPropagator.Instance.Inject(context, headers, isDataStreamsLegacyHeadersEnabled);

var extracted = DataStreamsContextPropagator.Instance.Extract(headers);

Expand Down Expand Up @@ -86,16 +87,17 @@ public void Extract_WhenBothHeadersPresent_PrefersBase64Header()
extractedContext.Value.EdgeStart.Should().NotBe(binaryContext.EdgeStart);
}

[Fact]
public void InjectedHeaders_HaveCorrectFormat()
[Theory]
[CombinatorialData]
public void InjectedHeaders_HaveCorrectFormat(bool isDataStreamsLegacyHeadersEnabled)
{
var headers = new TestHeadersCollection();
var context = new PathwayContext(
new PathwayHash(0x12345678),
0x1122334455667788,
unchecked((long)0x99AABBCCDDEEFF00));

DataStreamsContextPropagator.Instance.Inject(context, headers);
DataStreamsContextPropagator.Instance.Inject(context, headers, isDataStreamsLegacyHeadersEnabled);

headers.Values.Should().ContainKey(DataStreamsPropagationHeaders.PropagationKeyBase64);
var base64HeaderValueBytes = headers.Values[DataStreamsPropagationHeaders.PropagationKeyBase64];
Expand Down
Loading