-
Notifications
You must be signed in to change notification settings - Fork 772
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
Changes from 1 commit
86f4ff4
4fb9e9b
aad2b79
5c06f25
d2c16dc
cd3c9d7
de46f2b
cfa0f0b
d6596d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,10 +29,37 @@ namespace OpenTelemetry | |
/// </summary> | ||
public static class Sdk | ||
{ | ||
public static readonly SuppressInstrumentationScope SuppressInstrumentation = new SuppressInstrumentationScope(false); | ||
|
||
private static readonly TimeSpan DefaultPushInterval = TimeSpan.FromSeconds(60); | ||
|
||
/// <summary> | ||
/// Gets a value indicating whether instrumentation is suppressed (disabled). | ||
/// </summary> | ||
public static bool SuppressInstrumentation => SuppressInstrumentationScope.IsSuppressed; | ||
|
||
/// <summary> | ||
/// Begins a new scope in which instrumentation is suppressed (disabled). | ||
/// </summary> | ||
/// <param name="value">Value indicating whether to suppress instrumentation.</param> | ||
/// <returns>Object to dispose to end the scope.</returns> | ||
/// <remarks> | ||
/// 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<ExportResult> ExportAsync( | ||
/// IEnumerable<Activity> batch, | ||
/// CancellationToken cancellationToken) | ||
/// { | ||
/// using (Sdk.BeginSuppressInstrumentationScope()) | ||
/// { | ||
/// // Instrumentation is suppressed (i.e., Sdk.SuppressInstrumentation == true) | ||
/// } | ||
/// | ||
/// // Instrumentation is not suppressed (i.e., Sdk.SuppressInstrumentation == false) | ||
/// } | ||
/// </code> | ||
/// </remarks> | ||
public static SuppressInstrumentationScope BeginSuppressInstrumentationScope(bool value = true) => SuppressInstrumentationScope.Begin(value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think return should just be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! Good call. |
||
|
||
/// <summary> | ||
/// Creates MeterProvider with the configuration provided. | ||
/// Configuration involves MetricProcessor, Exporter and push internval. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,34 +32,7 @@ internal SuppressInstrumentationScope(bool value = true) | |
Slot.Set(value); | ||
} | ||
|
||
public static implicit operator bool(SuppressInstrumentationScope unused) => Slot.Get(); | ||
|
||
/// <summary> | ||
/// Begins a new scope in which instrumentation is suppressed (disabled). | ||
/// </summary> | ||
/// <param name="value">Value indicating whether to suppress instrumentation.</param> | ||
/// <returns>Object to dispose to end the scope.</returns> | ||
/// <remarks> | ||
/// 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<ExportResult> ExportAsync( | ||
/// IEnumerable<Activity> batch, | ||
/// CancellationToken cancellationToken) | ||
/// { | ||
/// using (Sdk.SuppressInstrumentation.Begin()) | ||
/// { | ||
/// // Instrumentation is suppressed (i.e., Sdk.SuppressInstrumentation == true) | ||
/// } | ||
/// | ||
/// // Instrumentation is not suppressed (i.e., Sdk.SuppressInstrumentation == false) | ||
/// } | ||
/// </code> | ||
/// </remarks> | ||
public IDisposable Begin(bool value = true) | ||
{ | ||
return new SuppressInstrumentationScope(value); | ||
} | ||
internal static bool IsSuppressed => Slot.Get(); | ||
|
||
/// <inheritdoc/> | ||
public void Dispose() | ||
|
@@ -70,5 +43,10 @@ public void Dispose() | |
this.disposed = true; | ||
} | ||
} | ||
|
||
internal static SuppressInstrumentationScope Begin(bool value = true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you feel about leaving this in?
Because I kind of like this way better: But I'm OK also having the helper too 😁 There's no more danger of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
{ | ||
return new SuppressInstrumentationScope(value); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 :)