Skip to content

Adds on_ending callback to allow processors to mutate spans before End operation#1713

Merged
mwear merged 11 commits intoopen-telemetry:mainfrom
pantuza:span-processor-on-ending-interface
Oct 1, 2024
Merged

Adds on_ending callback to allow processors to mutate spans before End operation#1713
mwear merged 11 commits intoopen-telemetry:mainfrom
pantuza:span-processor-on-ending-interface

Conversation

@pantuza
Copy link
Copy Markdown
Contributor

@pantuza pantuza commented Sep 1, 2024 •
edited
Loading

This Pull Request aims to solve the issue #1695 by adding the on_ending callback to SpanProcessor class.
It also make sure to call that method during the span End operation (finish() method), right after the end
timestamp has been set but while the span is still mutable.

Based on the spec, this implementation complies with:

  • The on_ending callback is optional within ANY SpanProcessor. Only called when exists.
  • called during the span End() operation
  • Span object is still mutable
  • MUST be called synchronously within the Span End operation
  • it should not block or throw an exception
  • MUST guarantee that the span can no longer be modified by any other thread before invoking on_ending
  • modifications are only allowed synchronously from within the invoked on_ending
  • SpanProcessor on_ending callbacks are executed before any SpanProcessor's on_end callback is invoked

@pantuza
feature: adds on_ending method as an optional callback for span proce…
90b2c85 
…ssors

Signed-off-by: Gustavo Pantuza <gustavopantuza@gmail.com>
@pantuza
feature: calls every span processor that has on_ending implemented ri…
93e6d9f 
…ght after setting the end timestamp

Signed-off-by: Gustavo Pantuza <gustavopantuza@gmail.com>
@pantuza
test: adds tests for the new on_ending method from span processors
0f1cf16 
Signed-off-by: Gustavo Pantuza <gustavopantuza@gmail.com>
@pantuza
Merge branch 'main' of github.com:open-telemetry/opentelemetry-ruby i…
e1c7cc3 
…nto span-processor-on-ending-interface
dmathieu
dmathieu reviewed Sep 2, 2024
View reviewed changes
Comment thread sdk/lib/opentelemetry/sdk/trace/span_processor.rb Outdated
#
# @param [Span] span the {Span} that just is ending (still mutable).
# @return [void]
def on_ending(span); end
Copy link
Copy Markdown
Member

@dmathieu dmathieu Sep 2, 2024

Choose a reason for hiding this comment

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

This is an experimental feature and may have breaking changes. How about specifying that here ?

Copy link
Copy Markdown
Contributor Author

@pantuza pantuza Sep 2, 2024

Choose a reason for hiding this comment

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

Thank you for the suggestion. I will update it.

@pantuza
docs: Informs the on_ending callback is an experimental features from…
2c28b76 
… official spec

Signed-off-by: Gustavo Pantuza <gustavopantuza@gmail.com>
@pantuza pantuza marked this pull request as ready for review September 3, 2024 19:21
@pantuza pantuza changed the title [Draft] Adds on_ending callback to allow processors to mutate spans before End operation Adds on_ending callback to allow processors to mutate spans before End operation Sep 3, 2024
kaylareopelle
kaylareopelle requested changes Sep 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, @pantuza! I want to think about this a little more, but I have some feedback on the documentation for now.

Comment thread sdk/lib/opentelemetry/sdk/trace/span_processor.rb Outdated
Comment thread sdk/lib/opentelemetry/sdk/trace/span_processor.rb Outdated
pantuza and others added 2 commits September 4, 2024 11:05
@pantuza @kaylareopelle
Update sdk/lib/opentelemetry/sdk/trace/span_processor.rb
9d9c944 
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
@pantuza @kaylareopelle
Update sdk/lib/opentelemetry/sdk/trace/span_processor.rb
173e57a 
Co-authored-by: Kayla Reopelle <87386821+kaylareopelle@users.noreply.github.com>
@pantuza
Copy link
Copy Markdown
Contributor Author

pantuza commented Sep 4, 2024

Thanks for your PR, @pantuza! I want to think about this a little more, but I have some feedback on the documentation for now.

Thank you for your feedback and documentation suggestions. I really appreciate it.
Take the necessary time to think about the PR and please, let me know in case you think there is anything left to be done in this PR.

To give you a little background context, have started this work because of the following discussion in this Pull Request #1713

@robbkidd
Copy link
Copy Markdown
Member

robbkidd commented Sep 4, 2024

… have started this work because of the following discussion in this Pull Request #1713 [👀]

1713 is this pull request. Is there a different discussion you meant to link to?

@pantuza
Copy link
Copy Markdown
Contributor Author

pantuza commented Sep 4, 2024

… have started this work because of the following discussion in this Pull Request #1713 [👀]

1713 is this pull request. Is there a different discussion you meant to link to?

You are right! Thank you for pointing it out. The correct discussion is in #1690

@pantuza pantuza mentioned this pull request Sep 6, 2024
Closed
mwear
mwear approved these changes Sep 13, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me. For reference, Java already has this: open-telemetry/opentelemetry-java#6367. We chose to rename the end method to finish since end is a Ruby keyword. One thought I had was whether or not we should considering calling this on_finishing, but I am ok with this as is. Curious if anyone else has any thoughts.

kaylareopelle
kaylareopelle approved these changes Sep 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Thank you!

@kaylareopelle
Copy link
Copy Markdown
Contributor

kaylareopelle commented Sep 13, 2024

One thought I had was whether or not we should considering calling this on_finishing, but I am ok with this as is. Curious if anyone else has any thoughts.

I do like the idea of using on_finishing here to keep the same term around, but don't feel strongly one way or the other. If we use on_finishing instead, we may want to add a link to the SDK documentation or "On Ending" to clear up any confusion that might exist connecting the on_finishing method to its specification.

@pantuza
Copy link
Copy Markdown
Contributor Author

pantuza commented Sep 17, 2024

I also like the idea of using on_finishing to keep names concise with the project decisions.
If you don't mind I will update the code that way and make sure to document it properly.

@pantuza
Merge branch 'main' of github.com:open-telemetry/opentelemetry-ruby i…
c9bb0f5 
…nto span-processor-on-ending-interface
@pantuza
refactor: switch method name from on_ending to on_finish to comply wi…
fc085c3 
…th project name strategy

Signed-off-by: Gustavo Pantuza <gustavopantuza@gmail.com>
@mwear mwear merged commit cdce3cc into open-telemetry:main Oct 1, 2024
@github-actions github-actions Bot mentioned this pull request Oct 1, 2024
Closed
@dmathieu dmathieu mentioned this pull request Oct 10, 2024
Closed
@arielvalentin arielvalentin mentioned this pull request Nov 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mwear mwear mwear approved these changes

@kaylareopelle kaylareopelle kaylareopelle approved these changes

@fbogsany fbogsany Awaiting requested review from fbogsany fbogsany is a code owner

@robertlaurin robertlaurin Awaiting requested review from robertlaurin robertlaurin is a code owner

@dazuma dazuma Awaiting requested review from dazuma dazuma is a code owner

@ericmustin ericmustin Awaiting requested review from ericmustin ericmustin is a code owner

@arielvalentin arielvalentin Awaiting requested review from arielvalentin arielvalentin is a code owner

@ahayworth ahayworth Awaiting requested review from ahayworth ahayworth is a code owner

@plantfansam plantfansam Awaiting requested review from plantfansam plantfansam is a code owner

@robbkidd robbkidd Awaiting requested review from robbkidd robbkidd is a code owner

+1 more reviewer

@dmathieu dmathieu dmathieu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pantuza @robbkidd @kaylareopelle @dmathieu @mwear