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 Sdk.SuppressInstrumentation #960

Merged
merged 7 commits into from
Aug 1, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api/Context/AsyncLocalRuntimeContextSlot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// </copyright>

#if !NET452
using System.Runtime.CompilerServices;
using System.Threading;

namespace OpenTelemetry.Context
Expand All @@ -38,12 +39,14 @@ public AsyncLocalRuntimeContextSlot(string name)
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override T Get()
{
return this.slot.Value;
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override void Set(T value)
{
this.slot.Value = value;
Expand Down
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api/Context/RemotingRuntimeContextSlot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#if NET452
using System.Collections;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.Remoting.Messaging;

namespace OpenTelemetry.Context
Expand Down Expand Up @@ -50,6 +51,7 @@ public RemotingRuntimeContextSlot(string name)
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override T Get()
{
var wrapper = CallContext.LogicalGetData(this.Name) as BitArray;
Expand All @@ -63,6 +65,7 @@ public override T Get()
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override void Set(T value)
{
var wrapper = new BitArray(0);
Expand Down
21 changes: 19 additions & 2 deletions src/OpenTelemetry.Api/Context/RuntimeContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace OpenTelemetry.Context
{
Expand All @@ -41,7 +42,8 @@ public sealed class RuntimeContext
/// </summary>
/// <param name="name">The name of the context slot.</param>
/// <typeparam name="T">The type of the underlying value.</typeparam>
public static void RegisterSlot<T>(string name)
/// <returns>The slot registered.</returns>
public static RuntimeContextSlot<T> RegisterSlot<T>(string name)
{
lock (Slots)
{
Expand All @@ -52,10 +54,23 @@ public static void RegisterSlot<T>(string name)

var type = ContextSlotType.MakeGenericType(typeof(T));
var ctor = type.GetConstructor(new Type[] { typeof(string) });
Slots[name] = ctor.Invoke(new object[] { name });
var slot = (RuntimeContextSlot<T>)ctor.Invoke(new object[] { name });
Slots[name] = slot;
return slot;
}
}

/// <summary>
/// Get a registered slot from a given name.
/// </summary>
/// <param name="name">The name of the context slot.</param>
/// <typeparam name="T">The type of the underlying value.</typeparam>
/// <returns>The slot previously registered.</returns>
public static RuntimeContextSlot<T> GetSlot<T>(string name)
{
return (RuntimeContextSlot<T>)Slots[name];
}

/*
public static void Apply(IDictionary<string, object> snapshot)
{
Expand Down Expand Up @@ -86,6 +101,7 @@ public static IDictionary<string, object> Snapshot()
/// <param name="name">The name of the context slot.</param>
/// <param name="value">The value to be set.</param>
/// <typeparam name="T">The type of the value.</typeparam>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void SetValue<T>(string name, T value)
{
var slot = (RuntimeContextSlot<T>)Slots[name];
Expand All @@ -98,6 +114,7 @@ public static void SetValue<T>(string name, T value)
/// <param name="name">The name of the context slot.</param>
/// <typeparam name="T">The type of the value.</typeparam>
/// <returns>The value retrieved from the context slot.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T GetValue<T>(string name)
{
var slot = (RuntimeContextSlot<T>)Slots[name];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// limitations under the License.
// </copyright>

using System.Runtime.CompilerServices;
using System.Threading;

namespace OpenTelemetry.Context
Expand All @@ -37,12 +38,14 @@ public ThreadLocalRuntimeContextSlot(string name)
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override T Get()
{
return this.slot.Value;
}

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public override void Set(T value)
{
this.slot.Value = value;
Expand Down
52 changes: 50 additions & 2 deletions src/OpenTelemetry/Sdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using OpenTelemetry.Context;
using OpenTelemetry.Metrics;
using OpenTelemetry.Metrics.Export;
using OpenTelemetry.Trace;
Expand All @@ -32,7 +34,53 @@ namespace OpenTelemetry
/// </summary>
public static class Sdk
{
private static TimeSpan defaultPushInterval = TimeSpan.FromSeconds(60);
private static readonly TimeSpan DefaultPushInterval = TimeSpan.FromSeconds(60);

private static readonly RuntimeContextSlot<bool> SuppressInstrumentationRuntimeContextSlot = RuntimeContext.RegisterSlot<bool>("otel.suppress_instrumentation");
Copy link
Member Author

@reyang reyang Jul 31, 2020

Choose a reason for hiding this comment

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

This would help the exporters/processors to quickly look up the suppress_instrumentation flag without a hash lookup.


/// <summary>
/// Gets or sets a value indicating whether automatic telemetry
/// collection in the current context should be suppressed (disabled).
/// Default value: False.
/// </summary>
/// <remarks>
/// Set <see cref="SuppressInstrumentation"/> to <see langword="true"/>
/// when you want to turn off automatic telemetry collection.
/// 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)
/// {
/// var currentSuppressionPolicy = Sdk.SuppressInstrumentation;
/// Sdk.SuppressInstrumentation = true;
/// try
/// {
/// await this.SendBatchActivityAsync(batch, cancellationToken).ConfigureAwait(false);
/// return ExportResult.Success;
/// }
/// finally
/// {
/// Sdk.SuppressInstrumentation = currentSuppressionPolicy;
/// }
/// }
/// </code>
/// </remarks>
public static bool SuppressInstrumentation
Copy link
Member

Choose a reason for hiding this comment

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

  • We should have XML comments here. Important spot :) Not sure exactly how to word it though?
		/// <summary>
		/// Gets or sets a value indicating whether or not <see cref="Activity"/>Activity</see> object collection in the current context should be suppressed (dsiabled). Default value: False.
		/// </summary>
		/// <remarks>
		/// Set <see cref="SuppressInstrumentation"/> to <see langword="true"/> when you want to turn off automatic <see cref="Activity"/> collection.
		/// This is typically done to prevent infinte loops created by collection of internal operations, such as exporting spans over HTTP.
		/// <code>
		///    public override async Task&lt;ExportResult&gt; ExportAsync(IEnumerable&lt;Activity&gt; batchActivity, CancellationToken cancellationToken)
		///    {
		///       Sdk.SuppressInstrumentation = true;
		///       try
		///       {
		///          await this.SendBatchActivityAsync(batchActivity, cancellationToken).ConfigureAwait(false);
		///          return ExportResult.Success;
		///       }
		///       finally
		///       {
		///          Sdk.SuppressInstrumentation = false;
		///       }
		///    }
		/// </code>
		/// </remarks>
  • Should context be in the prop name? SuppressInstrumentationOnCurrentContext? Trying to hint to users what it is doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • We should have XML comments here. Important spot :) Not sure exactly how to word it though?

Good catch, will add it.

  • Should context be in the prop name? SuppressInstrumentationOnCurrentContext? Trying to hint to users what it is doing.

Not sure, the proposed name seems too long 😅
Perhaps the XML comment should address this?

Copy link
Member

Choose a reason for hiding this comment

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

What if it wasn't exposed as a bool? Something more like log scope: public IDisposable SuppressInstrumentation()
Which is to say, return an object that sets the bool to true on ctor, sets it to fault on dispose?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is what @reyang was proposing in the PR description. I think I do like the disposable idea. I'm currently playing with this implementation and I thing this would make things read nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, @alanwest is helping on that part in a separate PR ❤️

{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get
{
return SuppressInstrumentationRuntimeContextSlot.Get();
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
set
{
SuppressInstrumentationRuntimeContextSlot.Set(value);
}
}

/// <summary>
/// Creates MeterProvider with the configuration provided.
Expand Down Expand Up @@ -60,7 +108,7 @@ public static MeterProvider CreateMeterProvider(Action<MeterBuilder> configure)
meterRegistry,
metricProcessor,
metricExporter,
meterBuilder.MetricPushInterval == default(TimeSpan) ? defaultPushInterval : meterBuilder.MetricPushInterval,
meterBuilder.MetricPushInterval == default(TimeSpan) ? DefaultPushInterval : meterBuilder.MetricPushInterval,
cancellationTokenSource);

var meterProviderSdk = new MeterProviderSdk(metricProcessor, meterRegistry, controller, cancellationTokenSource);
Expand Down