Skip to content
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.

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

Add ability to disable ActivitySources from being listened to #5795

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 24 additions & 6 deletions tracer/src/Datadog.Trace/Activity/ActivityListenerHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,34 @@ public static bool OnShouldListenTo<T>(T source)
{
if (source is IActivitySource activitySource)
{
if (handler.ShouldListenTo(sName, activitySource.Version) && HandlerBySource.TryAdd(sName, handler))
if (handler.ShouldListenTo(sName, activitySource.Version))
{
Log.Debug("ActivityListenerHandler: {SourceName} will be handled by {Handler}.", sName, handler);
return true;
if (handler is DisableActivityHandler)
Copy link
Member

Choose a reason for hiding this comment

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

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
}

{
Log.Information("ActivityListenerHandler: The .NET Tracer will not listen to {SourceName}.", sName);
return false;
}

if (HandlerBySource.TryAdd(sName, handler))
{
Log.Debug("ActivityListenerHandler: {SourceName} will be handled by {Handler}.", sName, handler);
return true;
}
}
}
else if (handler.ShouldListenTo(sName, null) && HandlerBySource.TryAdd(sName, handler))
else if (handler.ShouldListenTo(sName, null))
{
Log.Debug("ActivityListenerHandler: {SourceName} will be handled by {Handler}.", sName, handler);
return true;
if (handler is DisableActivityHandler)
{
Log.Information("ActivityListenerHandler: The .NET Tracer will not listen to {SourceName}.", sName);
return false;
}

if (HandlerBySource.TryAdd(sName, handler))
{
Log.Debug("ActivityListenerHandler: {SourceName} will be handled by {Handler}.", sName, handler);
return true;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,17 @@ namespace Datadog.Trace.Activity.Handlers
{
internal static class ActivityHandlersRegister
{
// Disable Activity Handler does not listen to the activity source at all.
internal static readonly DisableActivityHandler DisableHandler = new();
Comment on lines +12 to +13
Copy link
Member

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// Ignore Activity Handler catches existing integrations that also emits activities.
internal static readonly IgnoreActivityHandler IgnoreHandler = new();

// Activity handlers in order, the first handler where ShouldListenTo returns true will always handle that source.
internal static readonly IActivityHandler[] Handlers =
{
DisableHandler,

IgnoreHandler,

// Azure Service Bus handlers
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// <copyright file="DisableActivityHandler.cs" company="Datadog">
// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
// </copyright>

#nullable enable

using System.Collections.Generic;
using System.Text.RegularExpressions;
using Datadog.Trace.Activity.DuckTypes;
using Datadog.Trace.Sampling;

namespace Datadog.Trace.Activity.Handlers
{
internal class DisableActivityHandler : IActivityHandler
{
private List<Regex>? _disabledSourceNameGlobs = null;
private bool _disableAll = false;

public void ActivityStarted<T>(string sourceName, T activity)
where T : IActivity
{
// do nothing; this should not be called
}

public void ActivityStopped<T>(string sourceName, T activity)
where T : IActivity
{
// do nothing; this should not be called
}

/// <summary>
/// Determines whether <see cref="DisableActivityHandler"/> will "listen" to <paramref name="sourceName"/>.
/// <para>
/// Note that "listen" in this case means that the created ActivityListener will not subscribe to the ActivitySource.
/// </para>
/// </summary>
/// <returns><see langword="true"/> when the Tracer will disable the ActivitySource; otherwise <see langword="false"/></returns>
public bool ShouldListenTo(string sourceName, string? version)
{
if (_disableAll)
{
return true; // "*" was specified as a pattern, short circuit to disable all
}

_disabledSourceNameGlobs ??= PopulateGlobs();
if (_disabledSourceNameGlobs.Count == 0)
{
return false; // no glob patterns specified, sourceName will not be disabled
}

foreach (var regex in _disabledSourceNameGlobs)
{
if (regex.IsMatch(sourceName))
{
return true; // disable ActivitySource of "sourceName" from being listened to by the tracer
}
}

// sources were specified to be disabled, but this sourceName didn't match any of them
return false; // sourceName will _not_ be disabled
}

private List<Regex> PopulateGlobs()
{
var globs = new List<Regex>();
var toDisable = Tracer.Instance.Settings.DisabledActivitySources;
if (toDisable is null || toDisable.Length == 0)
{
return globs;
}

foreach (var disabledSourceNameGlob in toDisable)
{
// HACK: using RegexBuilder here even though it isn't _really_ for this
var globRegex = RegexBuilder.Build(disabledSourceNameGlob, SamplingRulesFormat.Glob, RegexBuilder.DefaultTimeout);
// handle special case where a "*" pattern will be null
if (globRegex is null)
{
_disableAll = true;
return [];
}

globs.Add(globRegex);
}

return globs;
}
}
}
7 changes: 7 additions & 0 deletions tracer/src/Datadog.Trace/Configuration/ConfigurationKeys.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ internal static partial class ConfigurationKeys
/// <seealso cref="TracerSettings.DisabledIntegrationNames"/>
public const string DisabledIntegrations = "DD_DISABLED_INTEGRATIONS";

/// <summary>
/// Configuration key for a list of ActivitySource names (supports globbing) that will be disabled.
/// Default is empty (all ActivitySources will be subscribed to by default).
/// Supports multiple values separated with commas.
/// </summary>
public const string DisabledActivitySources = "DD_TRACE_DISABLED_ACTIVITY_SOURCES";

/// <summary>
/// Configuration key for enabling or disabling default Analytics.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ internal ImmutableTracerSettings(TracerSettings settings, bool unusedParamNotToU
SpanSamplingRules = settings.SpanSamplingRules;
_globalSamplingRate = settings.GlobalSamplingRateInternal;
IntegrationsInternal = new ImmutableIntegrationSettingsCollection(settings.IntegrationsInternal, settings.DisabledIntegrationNamesInternal);
DisabledActivitySources = settings.DisabledActivitySources;
_headerTags = new ReadOnlyDictionary<string, string>(settings.HeaderTagsInternal);
GrpcTagsInternal = new ReadOnlyDictionary<string, string>(settings.GrpcTagsInternal);
IpHeader = settings.IpHeader;
Expand Down Expand Up @@ -338,6 +339,12 @@ internal ImmutableTracerSettings(TracerSettings settings, bool unusedParamNotToU
[GeneratePublicApi(PublicApiUsage.ImmutableTracerSettings_Integrations_Get)]
internal ImmutableIntegrationSettingsCollection IntegrationsInternal { get; }

/// <summary>
/// Gets the comma separated string of disabled ActivitySources that will not be handled at all.
/// </summary>
/// <seealso cref="ConfigurationKeys.DisabledActivitySources"/>
internal string[] DisabledActivitySources { get; }

/// <summary>
/// Gets the global tags, which are applied to all <see cref="Span"/>s.
/// </summary>
Expand Down
10 changes: 10 additions & 0 deletions tracer/src/Datadog.Trace/Configuration/TracerSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ _ when x.ToBoolean() is { } boolean => boolean,

DisabledIntegrationNamesInternal = new HashSet<string>(disabledIntegrationNames, StringComparer.OrdinalIgnoreCase);

var disabledActivitySources = config.WithKeys(ConfigurationKeys.DisabledActivitySources).AsString();

DisabledActivitySources = !string.IsNullOrEmpty(disabledActivitySources) ? TrimSplitString(disabledActivitySources, commaSeparator) : [];

IntegrationsInternal = new IntegrationSettingsCollection(source, unusedParamNotToUsePublicApi: false);

ExporterInternal = new ExporterSettings(source, _telemetry);
Expand Down Expand Up @@ -604,6 +608,12 @@ _ when x.ToBoolean() is { } boolean => boolean,
[ConfigKey(ConfigurationKeys.DisabledIntegrations)]
internal HashSet<string> DisabledIntegrationNamesInternal { get; set; }

/// <summary>
/// Gets the names of disabled ActivitySources.
/// </summary>
/// <seealso cref="ConfigurationKeys.DisabledActivitySources"/>
internal string[] DisabledActivitySources { get; }

/// <summary>
/// Gets or sets the transport settings that dictate how the tracer connects to the agent.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public NetActivitySdkTests(ITestOutputHelper output)
public async Task SubmitsTraces()
{
SetEnvironmentVariable("DD_TRACE_OTEL_ENABLED", "true");
SetEnvironmentVariable("DD_TRACE_DISABLED_ACTIVITY_SOURCES", "Disabled.By.ExactMatch,*.By.Glob*");

using (var telemetry = this.ConfigureTelemetry())
using (var agent = EnvironmentHelper.GetMockAgent())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@
"DD_IAST_TRUNCATION_MAX_VALUE_LENGTH": "iast_truncation_max_value_length",
"DD_IAST_DB_ROWS_TO_TAINT": "iast_db_rows_to_taint",
"DD_IAST_COOKIE_FILTER_PATTERN": "iast_cookie_filter_pattern",
"DD_TRACE_DISABLED_ACTIVITY_SOURCES": "dd_trace_disabled_activity_sources",
"DD_TRACE_STARTUP_LOGS": "trace_startup_logs_enabled",
"DD_MAX_LOGFILE_SIZE": "trace_log_file_max_size",
"DD_TRACE_LOGGING_RATE": "trace_log_rate",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ namespace NetActivitySdk;
public static class Program
{
private static ActivitySource _source;
private static ActivitySource _ignored;
private static ActivitySource _disabledGlob;
private static ActivitySource _disabledExact;

private static string SpanLinkTraceId1;
private static string SpanLinkTraceId2;
Expand All @@ -19,12 +22,15 @@ public static async Task Main(string[] args)
{
Console.WriteLine($"SpanId 1: {SpanLinkSpanId1} SpanId 2: {SpanLinkSpanId2}");
_source = new ActivitySource("Samples.NetActivitySdk");
_ignored = new ActivitySource("Microsoft.AspNetCore"); // this is an ignored source
_disabledGlob = new ActivitySource("Disabled.By.GlobPattern");
_disabledExact = new ActivitySource("Disabled.By.ExactMatch");

var activityListener = new ActivityListener
{
//ActivityStarted = activity => Console.WriteLine($"{activity.DisplayName} - Started"),
ActivityStopped = activity => PrintSpanStoppedInformation(activity),
ShouldListenTo = _ => true,
ShouldListenTo = source => { return !source.Name.Contains("Disabled"); }, // return true for all except Disabled ones
Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData
};

Expand All @@ -51,6 +57,8 @@ public static async Task Main(string[] args)

RunManuallyUpdatedStartTime(); // 3 spans (50 total)

RunIgnoredAndDisabledSources(); // 0 spans (50 total)

await Task.Delay(1000);
}

Expand Down Expand Up @@ -208,6 +216,22 @@ private static void RunManuallyUpdatedStartTime()
}
}

private static void RunIgnoredAndDisabledSources()
{
using var ignoredSpan = _ignored.StartActivity("Ignored - SHOULD NOT BE IN SNAPSHOT") ?? throw new InvalidOperationException("Ignored Activity was null - was it disabled?");
using var disabledSpanGlob = _disabledGlob.StartActivity("DisabledGlob - SHOULD BE NULL AND NOT IN SNAPSHOT");
if (disabledSpanGlob is not null)
{
throw new InvalidOperationException("DisabledGlob Activity wasn't null");
}

using var disabledSpanExact = _disabledExact.StartActivity("DisabledExact - SHOULD BE NULL AND NOT IN SNAPSHOT");
if (disabledSpanExact is not null)
{
throw new InvalidOperationException("DisabledExact Activity wasn't null");
}
}

private static void RunActivityUpdate()
{
using var errorSpan = _source.StartActivity("ErrorSpan");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@

"DD_DOTNET_TRACER_HOME": "$(SolutionDir)shared\\bin\\monitoring-home",
"DD_TRACE_OTEL_ENABLED": "true",
"DD_TRACE_DEBUG": "true"
"DD_TRACE_DEBUG": "true",
"DD_TRACE_DISABLED_ACTIVITY_SOURCES": "Disabled.By.ExactMatch,*.By.Glob*"
},
"nativeDebugging": false
},
Expand Down
Loading