Make SuppressInstrumentation an IDisposable

[alanwest]

Implementing the suggestion from @reyang and 👍'ed by @CodeBlanch and myself in #960 to introduce using syntactic sugar 🍬 for SuppressInstrumentation.

Before I go too far, I wanted to have a quick discussion for what kind of behavior we'd like in the event of a nested situation like the following where Dispose is explicitly called:

        [Fact]
        public static void UsingSuppressInstrumentation_Nested()
        {
            Assert.False(SuppressInstrumentation.IsSuppressed);
            using (var scope = SuppressInstrumentation.Begin())
            {
                Assert.True(SuppressInstrumentation.IsSuppressed);
                using (var innerScope = SuppressInstrumentation.Begin())
                {
                    Assert.True(SuppressInstrumentation.IsSuppressed);

                    // In order for the next Assert to pass, this Dispose would have to NoOp and be deferred
                    scope.Dispose();
                    Assert.True(SuppressInstrumentation.IsSuppressed);

                    // If we deferred the parent scope's Dispose from above, then it would also need to
                    // be triggered for a dispose here for the next Assert to pass.
                    innerScope.Dispose();
                    Assert.False(SuppressInstrumentation.IsSuppressed);
                }
                Assert.False(SuppressInstrumentation.IsSuppressed);
            }
        }

The current logic is simplistic and would not pass this test. Though, if people agree this is the correct flow we'd like to expect, then I can implement it with a mechanism where the inner scope signals the parent that it is ok to dispose. However, I just want to make sure things aren't becoming more complicated than people would hope for.

[reyang]

This is a bit different from what I was originally thinking - I was exploring something on the RuntimeContext instead of SuppressInstrumentation.

So instead of having:

var foo = RuntimeContext.GetValue<int>("foo");
var bar = RuntimeContext.GetValue<string>("bar");
try
{
    RuntimeContext.SetValue("foo", 1);
    RuntimeContext.SetValue("bar", "hello");
    // do something
}
finally
{
    RuntimeContext.SetValue("foo", foo);
    RuntimeContext.SetValue("bar", bar);
}

One could use (not sure if people would like With or WithValue) (and this means we got to do some trick to make sure both foo and bar got restored properly):

using (RuntimeContext.With("foo", 1).With("bar", "hello"))
{
    // do something
}

And for the flag (I don't know if it is possible in C# to have the following syntax - and meanwhile still have Sdk.SuppressInstrumentation evaluates to Boolean - just trying to push for simplicity):

using (Sdk.SuppressInstrumentation(true))
{
    // do something
}

[alanwest]

Hmm, I see. These are interesting ideas. Though both of them still raise the question of what the behavior should be if a using is used and Dispose is explicitly called prior to the end of the using scope. I might be overthinking things (I do that) - though just looking to be defensive. I'm currently looking to see if there is a common practice here. Like in logging frameworks where nested scopes are common.

Though, I could also be ok with just taking the simple approach, because in practice this may not likely arise.

[CodeBlanch]

I kind of like what @alanwest has. I would probably add "Scope" (or "Context") into the name, a la:

using (SuppressInstrumentationScope.Begin())
{
   await ExportAsync(batch).ConfigureAwait(false);
}

I think for the common case we don't need to pass true? Meaning, I can't think of a use case for setting it as false.

Now if SuppressInstrumentationScope was just wrapping RuntimeContext.With("otel.suppress_instrumentation", true) that would be really neat.

// In order for the next Assert to pass, this Dispose would have to NoOp and be deferred

What you are doing is (essentially):

            using (var scope = SuppressInstrumentation.Begin())
                using (var innerScope = SuppressInstrumentation.Begin())
                    scope.Dispose();
                    innerScope.Dispose();

I would simply say, unsupported 😄 Have to dispose them in the correct order.

            using (var scope = SuppressInstrumentation.Begin())
                using (var innerScope = SuppressInstrumentation.Begin())
                    innerScope.Dispose();
                    scope.Dispose();

That should work fine.

[alanwest]

I do like the unsupported 😄 approach. Can we put emojis in the documentation?

I think for the common case we don't need to pass true? Meaning, I can't think of a use case for setting it as false.

I was also imagining this to be true. Though also trying to stretch my brain 🧠 in search of use cases I may not be thinking of. @reyang, was there a use case you were thinking of?

[CodeBlanch]

I do like the unsupported 😄 approach. Can we put emojis in the documentation?

I'm being serious! hah I don't think the expectation on these scope/context type of things is that you can dispose them out of order. I'm thinking about Activity.Current. It has the same issue. If you dispose/stop a parent, it sets its parent as Activity.Current. If someone then does Activity.Current.Stop thinking it's the child, havoc ensues. Same idea for log scopes.

[reyang]

I think for the common case we don't need to pass true? Meaning, I can't think of a use case for setting it as false.

I was also imagining this to be true. Though also trying to stretch my brain 🧠 in search of use cases I may not be thinking of. @reyang, was there a use case you were thinking of?

I don't have a use case at all. It was mainly coming from natural language. For example using(SuppressInstrumentation) makes sense literally, the same for using(SuppressInstrumentation(true)), however using(SuppressInstrumentation.Begin()) seems weird literally as folks would start to wonder Where is SuppressInstrumentation.End()?.

[CodeBlanch]

It reads really naturally to me, but I'm working in a codebase that uses a lot of log scopes. Or its cousin, the change token. Basically a newer pattern in .NET where you return IDisposable as the mechanism for calling stop/end/cancel. I really like it 🤷

[reyang]

It reads really naturally to me, but I'm working in a codebase that uses a lot of log scopes. Or its cousin, the change token. Basically a newer pattern in .NET where you return IDisposable as the mechanism for calling stop/end/cancel. I really like it 🤷

Then go for it 💯 😄

[reyang]

I do like the unsupported 😄 approach. Can we put emojis in the documentation?

FYI open-telemetry/opentelemetry-cpp#209 (comment).

@alanwest @cijothomas @CodeBlanch

[alanwest]

I do like the unsupported 😄 approach. Can we put emojis in the documentation?

I'm being serious! hah I don't think the expectation on these scope/context type of things is that you can dispose them out of order.

Yea me, too! Sorry if I sounded sarcastic 😅. I completely agree that it make sense not to support out-of-order disposal. I figured this was a case of over-thinking, so I always like to just stop and see what others think.

It reads really naturally to me, but I'm working in a codebase that uses a lot of log scopes. Or its cousin, the change token. Basically a newer pattern in .NET where you return IDisposable as the mechanism for calling stop/end/cancel. I really like it 🤷

Then go for it 💯 😄

Sounds like a decent consensus here. I appreciate the thoughts around natural language, though I also agree that there is enough of a pattern and practice in .NET for the Begin() approach to be familiar with folk.

I'll get this PR cleaned up.

[reyang]

Consider implicit operator?

public sealed class SuppressInstrumentationScope : IDisposable
{
    private readonly bool value;
    public static implicit operator bool(SuppressInstrumentationScope _) => SuppressInstrumentationRuntimeContextSlot.Get();

    public SuppressInstrumentationScope(bool value = true)
    {
        this.value = value;
        SuppressInstrumentationRuntimeContextSlot.Set(value);
    }

    public void Dispose()
    {
        SuppressInstrumentationRuntimeContextSlot.Set(this.value);
    }

    public IDisposable Begin(bool value = true)
    {
        return new SuppressInstrumentationScope(value);
    }
}

public readonly static SuppressInstrumentationScope SuppressInstrumentation = new SuppressInstrumentationScope(false);

[reyang]

Then from consumption side:

if (Sdk.SuppressInstrumentation)
{
    // do something
}

using (Sdk.SuppressInstrumentation.Begin())
{
    // do something
}

[alanwest]

Interesting, I would not have thought of this approach!

[alanwest]

Ok pushed up something that works.

[reyang]

Given this is inside a class SuppressInstrumentationScope, we can probably simplify the name to ContextSlot or even Slot.

[reyang]

Suggested change

	public static implicit operator bool(SuppressInstrumentationScope x) => SuppressInstrumentationRuntimeContextSlot.Get();
	public static implicit operator bool(SuppressInstrumentationScope unused) => SuppressInstrumentationRuntimeContextSlot.Get();

[reyang]

Or other name that explains the intention.

[alanwest]

😄 agree unused is better than x. I changed it. Though this did leave me wondering if people would prefer to be able to use _ in this case. I think it's pretty common practice, though I imagine allowing it means allowing any variable to start with a _ which I'm guessing we don't want. No biggie unused is great.

[reyang]

I like _ and initially I was trying to use it, then caught by a style cop 😄

[reyang]

LGTM with some suggestions.

[cijothomas]

PLease address the small comments about naming which Reiley already left.
Can you also add this to changelog.md?

[codecov]

Codecov Report

Merging #988 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
+ Coverage   68.47%   68.55%   +0.07%     
==========================================
  Files         220      221       +1     
  Lines        6002     6010       +8     
  Branches      983      984       +1     
==========================================
+ Hits         4110     4120      +10     
+ Misses       1619     1617       -2     
  Partials      273      273              

Impacted Files	Coverage Δ
src/OpenTelemetry/Sdk.cs	88.09% <100.00%> (+3.03%)	⬆️
src/OpenTelemetry/SuppressInstrumentationScope.cs	100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs	80.00% <0.00%> (-13.34%)	⬇️
...emetry.Api/Context/AsyncLocalRuntimeContextSlot.cs	100.00% <0.00%> (+50.00%)	⬆️

[cijothomas]

@alanwest conflict here :-)

[alanwest]

You're a mergin' machine, @cijothomas! Thanks for shepherding so many PRs through!