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.

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

refactor: context only parenting #398

Merged
merged 10 commits into from
Sep 29, 2020

Conversation

mwear
Copy link
Member

@mwear mwear commented Sep 18, 2020

This PR is a draft showing what changes would be required to restrict explicit span parenting to context only (rather than allowing a span, or context). This is an effort towards validating that: open-telemetry/opentelemetry-specification#510 is viable in Ruby.

There were two options to provide an explicit parent to start_span and friends. One was named with_parent and expected an argument to be a span. The other was named with_parent_context where the argument was a context. In this PR we removed with_parent_context and updated with_parent to accept a context rather than a span.

I also added a Tracer#context_with_span convenience method which is the equivalent of ctx = tracer.with_span(span) { |_, c| c }, which was something I found myself doing in tests when I removed explicit span based parenting.

The original with_parent option was only used in tests. Instrumentation used with_parent_context, further validating context based parenting want we want.

Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

I realize this is just a prototype, but it looks like a good improvement to me 👍

@fbogsany
Copy link
Contributor

open-telemetry/opentelemetry-specification#875 also requires on_start for span processors to accept a parent_context parameter. This requires a little refactoring because Span#initialize only has a SpanContext and not the parent Context.

@mwear mwear marked this pull request as ready for review September 23, 2020 21:31
@mwear
Copy link
Member Author

mwear commented Sep 23, 2020

This is ready for review and can merge when folks are comfortable with it. The corresponding spec changes merged earlier today: open-telemetry/opentelemetry-specification#875.

Copy link
Contributor

@robertlaurin robertlaurin left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

@fbogsany fbogsany merged commit b47b51c into open-telemetry:master Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants