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

Conversation

reyang
Copy link
Member

@reyang reyang commented Jul 31, 2020

This is a follow up PR on #948. It helps us to move closer to the resolution for #893 (comment) and the 2nd point of #809 (comment).

Changes

With this change, SDK consumers (e.g. exporters) can use:

using OpenTelemetry;

System.Console.WriteLine($"{Sdk.SuppressInstrumentation}"); // this gives you false by default
Sdk.SuppressInstrumentation = true;
System.Console.WriteLine($"{Sdk.SuppressInstrumentation}");

var previous = Sdk.SuppressInstrumentation; // do we want some syntactic sugar such like using/dispose?
Sdk.SuppressInstrumentation = true;
// export via HTTP
Sdk.SuppressInstrumentation = previous;

For API consumers, I guess we don't want to expose this as a public API, so what folks would do (e.g. in instrumentation):

using OpenTelemetry.Context;

class FoobarInstrumentation
{
    internal static readonly RuntimeContextSlot<bool> SuppressInstrumentationSlot;
    FoobarInstrumentation()
    {
        try {
            SuppressInstrumentationSlot = RuntimeContext.GetSlot("otel.suppress_instrumentation");
        } catch (Exception) {
            // instrumentation initialization failed, swallow the exception to prevent type load error.
        }
    }

    // no set as we don't expect instrumentation to change this flag
    public static bool SuppressInstrumentation
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        get
        {
            return SuppressInstrumentationSlot == null ? false : SuppressInstrumentationSlot.Get();
        }
    }
}


if (FoobarInstrumentation.SuppressInstrumentation)
{
    // skip
}

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public API reviewed

@reyang reyang requested a review from a team July 31, 2020 17:28
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.

@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #960 into master will increase coverage by 0.21%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   68.24%   68.46%   +0.21%     
==========================================
  Files         218      218              
  Lines        5955     5961       +6     
  Branches      969      969              
==========================================
+ Hits         4064     4081      +17     
+ Misses       1623     1611      -12     
- Partials      268      269       +1     
Impacted Files Coverage Δ
...emetry.Api/Context/AsyncLocalRuntimeContextSlot.cs 50.00% <ø> (+50.00%) ⬆️
...rc/OpenTelemetry.Api/Context/DistributedContext.cs 76.92% <ø> (ø)
...Telemetry.Api/Context/DistributedContextBuilder.cs 69.23% <ø> (ø)
...elemetry.Api/Context/RemotingRuntimeContextSlot.cs 0.00% <ø> (ø)
...metry.Api/Context/ThreadLocalRuntimeContextSlot.cs 0.00% <ø> (ø)
...lemetryProtocol/Implementation/TimestampHelpers.cs 100.00% <ø> (ø)
...OpenTelemetry/Metrics/BoundCounterMetricSdkBase.cs 100.00% <ø> (ø)
src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs 93.33% <ø> (ø)
src/OpenTelemetry/Sdk.cs 85.05% <50.00%> (-2.90%) ⬇️
src/OpenTelemetry.Api/Context/RuntimeContext.cs 44.44% <75.00%> (+44.44%) ⬆️
... and 6 more

{
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

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.


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

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 ❤️

@alanwest
Copy link
Member

alanwest commented Aug 1, 2020

@reyang In your sample code in the PR description you have

// no set as we don't expect instrumentation to change this flag

Though, it seems like this will be desirable. If some DB library uses HttpClient under the covers then it seems that it will need to be up to the DB library to enable suppression if that's the desired effect.

@reyang
Copy link
Member Author

reyang commented Aug 1, 2020

@reyang In your sample code in the PR description you have

// no set as we don't expect instrumentation to change this flag

Though, it seems like this will be desirable. If some DB library uses HttpClient under the covers then it seems that it will need to be up to the DB library to enable suppression if that's the desired effect.

Definitely. Another example is a library that keep polling the backend (e.g. in Microsoft there is something called SignalR), where folks only want high level logical operation rather than millions of HTTP 304.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants