-
Notifications
You must be signed in to change notification settings - Fork 859
[OpenTelemetry] Add OnEnding span processor functionality #6617
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
base: main
Are you sure you want to change the base?
Conversation
85231aa to
e108c44
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6617 +/- ##
==========================================
+ Coverage 86.73% 86.76% +0.02%
==========================================
Files 258 259 +1
Lines 11958 11976 +18
==========================================
+ Hits 10372 10391 +19
+ Misses 1586 1585 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
This feature is still in development phase. It means that we cannot include it into stable releases for this package. You can make full search for Another thing for consideration: double check if it is not breaking change on binary level when you are introducing new class to the inheritance. |
f176ccd to
e1a2717
Compare
|
I think I've appropriately added the OTEL1005 label where needed, could I get some help rerunning the workflows to see if there are any other remaining issues? And regarding the following, is there a way to do so besides just running the build/tests? Thanks in advance
|
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
This change creates a new ExtendedBaseProcessor that allows users to implement onEnding functionality to their processor, as per the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#onending
|
Could I get some help with running the workflows and receiving a review? Thanks |
| #pragma warning disable CA1012 // Abstract types should not have public constructors | ||
| public abstract class ExtendedBaseProcessor<T> : BaseProcessor<T> | ||
| #pragma warning restore CA1012 // Abstract types should not have public constructors |
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 should be fixed by adding an explicit protected constructor.
|
|
||
| ## Overview | ||
|
|
||
| This is an experimental API for modifying spans before they end/close |
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 an experimental API for modifying spans before they end/close | |
| This is an experimental API for modifying spans before they end/close. |
| #### ExtendedBaseProcessor | ||
|
|
||
| The abstract class `ExtendedBaseProcessor` provides an extension of the | ||
| `BaseProcessor` that allows spans to be modified before they end as per the |
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.
| `BaseProcessor` that allows spans to be modified before they end as per the | |
| `BaseProcessor` class that allows spans to be modified before they end as per the |
|
|
||
| The abstract class `ExtendedBaseProcessor` provides an extension of the | ||
| `BaseProcessor` that allows spans to be modified before they end as per the | ||
| OpenTelemetry specification. It provides the `OnEnding` function that is called |
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.
| OpenTelemetry specification. It provides the `OnEnding` function that is called | |
| OpenTelemetry specification. It provides the `OnEnding` method that is called |
| `BaseProcessor` that allows spans to be modified before they end as per the | ||
| OpenTelemetry specification. It provides the `OnEnding` function that is called | ||
| during the span `End()` operation. The end timestamp MUST have been computed | ||
| (the `OnEnding` method duration is not included in the span duration). The Span |
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.
| (the `OnEnding` method duration is not included in the span duration). The Span | |
| (the `OnEnding` method duration is not included in the span duration). The `Span` |
| /// </summary> | ||
| /// <typeparam name="T">The type of object to be processed.</typeparam> | ||
| #if EXPOSE_EXPERIMENTAL_FEATURES | ||
| public class CompositeProcessor<T> : ExtendedBaseProcessor<T> |
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.
Checked the documentation and it should be OK to change the base class without making a breaking change as all the members in the new class aren't abstract.
| for (var cur = this.Head; cur != null; cur = cur.Next) | ||
| { | ||
| if (typeof(ExtendedBaseProcessor<T>).IsAssignableFrom(cur.Value.GetType())) | ||
| { | ||
| ((ExtendedBaseProcessor<T>)cur.Value).OnEnding(data); | ||
| } | ||
| } |
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.
Would this work to avoid reflection and casting?
| for (var cur = this.Head; cur != null; cur = cur.Next) | |
| { | |
| if (typeof(ExtendedBaseProcessor<T>).IsAssignableFrom(cur.Value.GetType())) | |
| { | |
| ((ExtendedBaseProcessor<T>)cur.Value).OnEnding(data); | |
| } | |
| } | |
| for (var current = this.Head; current != null; cur = current.Next) | |
| { | |
| if (current.Value is ExtendedBaseProcessor<T> processor) | |
| { | |
| processor.OnEnding(data); | |
| } | |
| } |
| /// The started telemetry object. | ||
| /// </param> | ||
| /// <remarks> | ||
| /// This function is called synchronously on the thread which ended |
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 function is called synchronously on the thread which ended | |
| /// This method is called synchronously on the thread which ended |
| if (typeof(ExtendedBaseProcessor<Activity>).IsAssignableFrom(this.processor?.GetType())) | ||
| { | ||
| (this.processor as ExtendedBaseProcessor<Activity>)?.OnEnding(activity); | ||
| } |
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.
| if (typeof(ExtendedBaseProcessor<Activity>).IsAssignableFrom(this.processor?.GetType())) | |
| { | |
| (this.processor as ExtendedBaseProcessor<Activity>)?.OnEnding(activity); | |
| } | |
| if (this.processor is ExtendedBaseProcessor<Activity> extended) | |
| { | |
| extended.OnEnding(activity); | |
| } |
| if (typeof(ExtendedBaseProcessor<Activity>).IsAssignableFrom(this.processor?.GetType())) | ||
| { | ||
| (this.processor as ExtendedBaseProcessor<Activity>)?.OnEnding(activity); | ||
| } |
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.
| if (typeof(ExtendedBaseProcessor<Activity>).IsAssignableFrom(this.processor?.GetType())) | |
| { | |
| (this.processor as ExtendedBaseProcessor<Activity>)?.OnEnding(activity); | |
| } | |
| if (this.processor is ExtendedBaseProcessor<Activity> extended) | |
| { | |
| extended.OnEnding(activity); | |
| } |
| * Add support for .NET 10.0. | ||
| ([#6307](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6307)) | ||
|
|
||
| * feat: add on ending span processor functionality |
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.
Please improve this document to talk about when users should implement OnEnd vs the new option.
I am not sure of the answer given .NET's use of Activity to represent span. This must be clear to avoid user confusion once this is released.
cijothomas
left a comment
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.
Requesting changes to make sure this is actually needed. While the Spec newly added OnEnding to allow Span modification, OTel .NET uses Activity to represent span, which has slightly different semantics than the spec. And Activity is already writeable in OnEnd.
Do we need this in OTel .NET at all? If yes, please list the reason(s) before adding this.
Fixes: N/A
Design discussion issue: open-telemetry/opentelemetry-specification#4024
Changes
Changes based loosely on the same changes made to opentelemetry-java: open-telemetry/opentelemetry-java#6367
This change creates a new ExtendedBaseProcessor that allows users to implement onEnding functionality to their processor, as per the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#onending
Specific changes:
CompositeProcessorto be extended from the newExtendedBaseProcessorto support processors that have OnEndingExtendedBaseProcessoras the place where this functionality is added, so as to not break any current experience of extending the normalBaseProcessorMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes