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

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Aug 12, 2020

Refactoring our SuppressInstrumentation APIs to be less likely to cause problems.

Prior to this PR it was possible to do:

using (var scope = Sdk.SuppressInstrumentation)
{
    if (Sdk.SuppressInstrumentation) ... // false
}

while what was intended was:

using (var scope = Sdk.SuppressInstrumentation.Begin())
{
    if (Sdk.SuppressInstrumentation) ... // true
}

The former would have caused the static SuppressInstrumentationScope instance to be disposed, and furthermore it would not have actually suppressed instrumentation as intended.

Now the usage is:

using (var scope = Sdk.BeginSuppressInstrumentationScope())
{
    if (Sdk.SuppressInstrumentation) ... // true
}

@reyang @CodeBlanch

@alanwest alanwest requested a review from a team August 12, 2020 23:02
/// <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 :)

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM, that's a good improvement to avoid pitfalls.

@reyang
Copy link
Member

reyang commented Aug 12, 2020

Need to update the CHANGELOG.

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #1067 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1067   +/-   ##
=======================================
  Coverage   77.40%   77.40%           
=======================================
  Files         220      220           
  Lines        6080     6080           
=======================================
  Hits         4706     4706           
  Misses       1374     1374           
Impacted Files Coverage Δ
src/OpenTelemetry/Sdk.cs 94.73% <100.00%> (ø)
src/OpenTelemetry/SuppressInstrumentationScope.cs 100.00% <100.00%> (ø)

/// }
/// </code>
/// </remarks>
public static SuppressInstrumentationScope BeginSuppressInstrumentationScope(bool value = true) => SuppressInstrumentationScope.Begin(value);
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 return should just be IDisposable. It's not like you can do anything with the class. More closely matches log scope that way too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! Good call.

@@ -70,5 +43,10 @@ public void Dispose()
this.disposed = true;
}
}

internal static SuppressInstrumentationScope Begin(bool value = true)
Copy link
Member

Choose a reason for hiding this comment

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

How do you feel about leaving this in?

public static IDisposable Begin(bool value = true)

Because I kind of like this way better:
using (var scope = SuppressInstrumentationScope.Begin())

But I'm OK also having the helper too 😁

There's no more danger of using (SuppressInstrumentationScope) because the static is gone right?

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 feel great about this. I actually decided to remove the helper in favor of this idea. Though, happy to revert and put the helper back if others find it useful. I personally like your idea better.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas cijothomas merged commit 0802d1e into open-telemetry:master Aug 14, 2020
@alanwest alanwest deleted the alanwest/safer-suppress-instrumentation branch August 24, 2020 17:35
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