-
Notifications
You must be signed in to change notification settings - Fork 833
[sdk] Provide better concurrency modes #5643
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
Changes from 1 commit
72a2553
226c19e
458faaf
67324bd
08201f2
608da98
638ad10
3bad975
fd36fc9
c5b6dd1
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 |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
namespace OpenTelemetry; | ||
|
||
/// <summary> | ||
/// Describes the concurrency mode of an OpenTelemetry extension. | ||
/// </summary> | ||
[Flags] | ||
public enum ConcurrencyModes : byte | ||
{ | ||
/* | ||
0 0 0 0 0 0 0 0 | ||
| | | | | | | | | ||
| | | | | | | +--- Reentrant | ||
| | | | | | +----- Multithreaded | ||
| | | | | +------- (reserved) | ||
| | | | +--------- (reserved) | ||
| | | +----------- (reserved) | ||
| | +------------- (reserved) | ||
| +--------------- (reserved) | ||
+----------------- Global | ||
*/ | ||
|
||
/// <summary> | ||
/// Reentrant, the component can be invoked recursively without resulting | ||
/// a deadlock or infinite loop. | ||
/// </summary> | ||
Reentrant = 0b1, | ||
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. We removed 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. My gut feeling is no. Multithreaded and Reentrant are closely related, the existing implementation (which uses 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. No strong feelings though, I can scope this out and add it later once we figured out how to correctly handle reentrancy. 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. Interesting. So if the attribute is not specified and defaults to 0 our implied default is essentially Should the defined public enum ConcurrencyModes : byte
{
/// <summary>
/// Nonreentrant, the component cannot be invoked recursively without resulting
/// in a deadlock or infinite loop.
/// </summary>
Nonreentrant = 0b1,
/// <summary>
/// Multithreaded, the component can be invoked concurrently across
/// multiple threads.
/// </summary>
Multithreaded = 0b10,
} 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.
Maybe. I personally consider this as a bug in the current SDK implementation.
I guess no, better to treat it as a bug, then figure out how to fix it in a non-breaking way. 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. My thinking - if the exporter doesn't have the attribute at all, fall back to the old (and buggy) behavior, if the exporter has the attribute, start to enforce the correct behavior. |
||
|
||
/// <summary> | ||
/// Multithreaded, the component can be invoked concurrently across | ||
/// multiple threads. | ||
/// </summary> | ||
Multithreaded = 0b10, | ||
|
||
/// <summary> | ||
/// Global, when combined with other flags, indicates that a per-instance | ||
/// synchronization is insufficient, a global synchronization is required | ||
/// across all instances of the component. | ||
/// </summary> | ||
Global = 0b10000000, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
namespace OpenTelemetry; | ||
|
||
/// <summary> | ||
/// An attribute for declaring the supported <see cref="ConcurrencyModes"/> of an OpenTelemetry extension. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)] | ||
public sealed class ConcurrencyModesAttribute : Attribute | ||
{ | ||
private readonly ConcurrencyModes supportedConcurrencyModes; | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="ConcurrencyModesAttribute"/> class. | ||
/// </summary> | ||
/// <param name="supported"><see cref="ConcurrencyModes"/>.</param> | ||
public ConcurrencyModesAttribute(ConcurrencyModes supported) | ||
{ | ||
this.supportedConcurrencyModes = supported; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the supported <see cref="ConcurrencyModes"/>. | ||
/// </summary> | ||
public ConcurrencyModes Supported => this.supportedConcurrencyModes; | ||
} |
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.
This is to show how the attributes can be used, once folks agree with the direction, I'll revert the example code and update the changelog.