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

Make suppress instrumentation APIs less prone to introducing bugs #1067

2 changes: 1 addition & 1 deletion docs/trace/building-your-own-exporter/MyExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public override Task<ExportResult> ExportAsync(
// telemetry should do so inside SuppressInstrumentation
// scope. This suppresses telemetry from
// exporter's own code to avoid live-loop situation.
using var scope = Sdk.SuppressInstrumentation.Begin();
using var scope = SuppressInstrumentationScope.Begin();

foreach (var activity in batch)
{
Expand Down
3 changes: 2 additions & 1 deletion src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
match the spec
([#1013](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1013))
* Added `SuppressInstrumentationScope` API
([#988](https://github.com/open-telemetry/opentelemetry-dotnet/pull/988))
([#988](https://github.com/open-telemetry/opentelemetry-dotnet/pull/988)
[#1067](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1067))
* Changed `BroadcastActivityProcessor` to `FanOutActivityProcessor`
([#1015](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1015))
* Changed `TracerProviderBuilder` and `TracerProviderSdk` design to simply the
Expand Down
7 changes: 5 additions & 2 deletions src/OpenTelemetry/Sdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@ namespace OpenTelemetry
/// </summary>
public static class Sdk
{
public static readonly SuppressInstrumentationScope SuppressInstrumentation = new SuppressInstrumentationScope(false);

private static readonly TimeSpan DefaultPushInterval = TimeSpan.FromSeconds(60);

/// <summary>
/// Gets a value indicating whether instrumentation is suppressed (disabled).
/// </summary>
public static bool SuppressInstrumentation => SuppressInstrumentationScope.IsSuppressed;
Copy link
Member Author

@alanwest alanwest Aug 12, 2020

Choose a reason for hiding this comment

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

There's more to the API now, but I think it is simpler to understand by decoupling the disposable object from the boolean check. Should we apply AggresiveInlining on these APIs?

Copy link
Member

Choose a reason for hiding this comment

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

I guess AggresiveInlining is not going to help here :)


/// <summary>
/// Creates MeterProvider with the configuration provided.
/// Configuration involves MetricProcessor, Exporter and push internval.
Expand Down
24 changes: 12 additions & 12 deletions src/OpenTelemetry/SuppressInstrumentationScope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ internal SuppressInstrumentationScope(bool value = true)
Slot.Set(value);
}

public static implicit operator bool(SuppressInstrumentationScope unused) => Slot.Get();
internal static bool IsSuppressed => Slot.Get();

/// <summary>
/// Begins a new scope in which instrumentation is suppressed (disabled).
Expand All @@ -43,20 +43,20 @@ internal SuppressInstrumentationScope(bool value = true)
/// This is typically used to prevent infinite loops created by
/// collection of internal operations, such as exporting traces over HTTP.
/// <code>
/// public override async Task&lt;ExportResult&gt; ExportAsync(
/// IEnumerable&lt;Activity&gt; batch,
/// CancellationToken cancellationToken)
/// {
/// using (Sdk.SuppressInstrumentation.Begin())
/// {
/// // Instrumentation is suppressed (i.e., Sdk.SuppressInstrumentation == true)
/// }
/// public override async Task&lt;ExportResult&gt; ExportAsync(
/// IEnumerable&lt;Activity&gt; batch,
/// CancellationToken cancellationToken)
/// {
/// using (SuppressInstrumentationScope.Begin())
/// {
/// // Instrumentation is suppressed (i.e., Sdk.SuppressInstrumentation == true)
/// }
///
/// // Instrumentation is not suppressed (i.e., Sdk.SuppressInstrumentation == false)
/// }
/// // Instrumentation is not suppressed (i.e., Sdk.SuppressInstrumentation == false)
/// }
/// </code>
/// </remarks>
public IDisposable Begin(bool value = true)
public static IDisposable Begin(bool value = true)
{
return new SuppressInstrumentationScope(value);
}
Expand Down
6 changes: 4 additions & 2 deletions test/OpenTelemetry.Tests/SuppressInstrumentationTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ public class SuppressInstrumentationTest
[Fact]
public static void UsingSuppressInstrumentation()
{
using (var scope = Sdk.SuppressInstrumentation.Begin())
Assert.False(Sdk.SuppressInstrumentation);

using (var scope = SuppressInstrumentationScope.Begin())
{
Assert.True(Sdk.SuppressInstrumentation);

using (var innerScope = Sdk.SuppressInstrumentation.Begin())
using (var innerScope = SuppressInstrumentationScope.Begin())
{
innerScope.Dispose();

Expand Down