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.

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

Jump to bottom

Add Sdk.SuppressInstrumentation #960

Merged
merged 7 commits into from
Aug 1, 2020
Merged

Add Sdk.SuppressInstrumentation #960

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7a29abf
add Sdk.SuppressInstrumentation
reyang Jul 31, 2020
a686324
simplify the code
reyang Jul 31, 2020
3911dc5
fix comments
reyang Jul 31, 2020
1f83935
add XML comment
reyang Aug 1, 2020
af61349
Merge branch 'master' into reyang/suppress-instrumentation
CodeBlanch Aug 1, 2020
ce02006
Merge branch 'master' into reyang/suppress-instrumentation
reyang Aug 1, 2020
0594fdd
Merge branch 'master' into reyang/suppress-instrumentation
CodeBlanch Aug 1, 2020
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
View file
Open in desktop
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
View file
Open in desktop
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
23 changes: 21 additions & 2 deletions src/OpenTelemetry.Api/Context/RuntimeContext.cs
View file
Open in desktop
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,25 @@ 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, or null if not found.</returns>
public static RuntimeContextSlot<T> GetSlot<T>(string name)
{
object slot;
Slots.TryGetValue(name, out slot);
return (RuntimeContextSlot<T>)slot;
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause a cast exception if the the TryGet fails?

Copy link
Member Author

@reyang reyang Jul 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it will. I expect consumers to call this once, and cache the slot for subsequent use (similar like what I did for the Sdk).

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps I should just use (RuntimeContextSlot<T>)Slots[name]?

Copy link
Member

Choose a reason for hiding this comment

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

I think that will throw a key not found. I'm just noting your comment
<returns>The slot previously registered, or null if not found.</returns>

Seems like the TryGetValue is a good way to go and just return null if that fails. Was that your intent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the code and also the PR description to show my current approach/thinking.

Copy link
Member

Choose a reason for hiding this comment

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

👍 ok yea I get it, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Also, the using idea is interesting - maybe we try on for size what you have here, and refactor if we like it?

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, I think we will have better understanding of how syntactical sugar will help exporters/processors/instrumentation when we start to add the code.

}

/*
public static void Apply(IDictionary<string, object> snapshot)
{
Expand Down Expand Up @@ -86,6 +103,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 +116,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
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api/Context/ThreadLocalRuntimeContextSlot.cs
View file
Open in desktop
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
23 changes: 21 additions & 2 deletions src/OpenTelemetry/Sdk.cs
View file
Open in desktop
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,24 @@ 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 •
edited
Loading

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.


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 +79,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