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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Oct 1, 2024
Merged

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

merged 11 commits into from
Oct 1, 2024

Conversation

pantuza
Copy link
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
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
Member

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
Contributor Author

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
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.

sdk/lib/opentelemetry/sdk/trace/span_processor.rb Outdated Show resolved Hide resolved
sdk/lib/opentelemetry/sdk/trace/span_processor.rb Outdated Show resolved Hide resolved
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
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
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
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
Merge branch 'main' of github.com:open-telemetry/opentelemetry-ruby i…
a2802af 
…nto span-processor-on-ending-interface
@pantuza
Merge branch 'span-processor-on-ending-interface' of github.com:pantu…
fe7c455 
…za/opentelemetry-ruby into span-processor-on-ending-interface
@pantuza pantuza mentioned this pull request Sep 6, 2024
Closed
mwear
mwear approved these changes Sep 13, 2024
View reviewed changes
Copy link
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
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
Contributor

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
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
62 checks passed
@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

@dmathieu dmathieu dmathieu left review comments

@kaylareopelle kaylareopelle kaylareopelle approved these changes

@mwear mwear mwear 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

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