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

Make SpanProcessor.on_start accept parent Context #1251

Merged

Conversation

mariojonke
Copy link
Contributor

@mariojonke mariojonke commented Oct 16, 2020

Description

Adds optional parent Context parameter to SpanProcessor.on_start as per spec.

Fixes #1250

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Additional unit tests checking that the parent Context is passed through from tracer.start_span to SpanProcessor.on_start

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

* context from Tracer.start_span is passed through to the SpanProcessor
* fix linting issue in falcon test app when linting with eachdist script
* fix global error handler test as it read installed extensions
* reset global Configuration object state after tests were run
@mariojonke mariojonke requested review from a team, codeboten and lzchen and removed request for a team October 16, 2020 07:20
@lzchen
Copy link
Contributor

lzchen commented Oct 16, 2020

I understand that this is in the specs but what exactly is it being used for? I don't see the behavior being defined anywhere (or where this parameter is being used) specifically for SpanProcessor. @Oberon00 any thoughts on this?

@Oberon00
Copy link
Member

Oberon00 commented Oct 16, 2020

@lzchen

I don't see the behavior being defined anywhere

Which behavior do you mean? It is clearly specified what must be in there: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#OnStart

what exactly is it being used for

This is not used in the built-in SDK, but one usage example from Dynatrace is this: We have a custom propagator that reads an additional HTTP header. We set this http header in the returned context. In a custom SpanProcessor we can copy the header from the context to a span attribute, in order to later have it exported.

@Oberon00
Copy link
Member

@mariojonke

New feature (non-breaking change which adds functionality)

This changes the SpanProcessor interface, so it is breaking I think.

@lzchen
Copy link
Contributor

lzchen commented Oct 16, 2020

@Oberon00

Which behavior do you mean?

Sorry what I meant was:
If the default SDK is expected to use this parameter somewhere, the behavior isn't defined anywhere.

This is not used in the built-in SDK

If it is not used in the built-in SDK, why define Onstart with this parameter in the default SDK? Shouldn't your custom SpanProcessor have this method signature instead, and whatever implementation of Span should have self.span_processor.on_end(self, context)? Unless if I'm mistaken about how to actually use this.

@Oberon00
Copy link
Member

Oberon00 commented Oct 16, 2020

If it is not used in the built-in SDK, why define Onstart with this parameter in the default SDK?

Because one purpose of the built-in SDK is to allow users to add custom span processors which might need this parameter 😃

Shouldn't your custom SpanProcessor have this method signature instead, and whatever implementation of Span should have self.span_processor.on_end(self, context)

(you mean on_start here, right?)
"Whatever implementation of Span" is the default SDKs implementation. One doesn't need to implement a custom span to use a custom SpanProcessor.

@Oberon00
Copy link
Member

Note that Dynatrace is not the only party interested in this, as evident e.g. from open-telemetry/opentelemetry-java#1811

@lzchen
Copy link
Contributor

lzchen commented Oct 16, 2020

@Oberon00

"Whatever implementation of Span" is the default SDKs implementation.

Ahh I see that was the confusion. Thanks for the explanation! :)

@@ -25,7 +25,8 @@


class TestErrorHandler(TestCase):
def test_default_error_handler(self):
@patch("opentelemetry.sdk.error_handler.iter_entry_points")
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when running the test locally with the eachdist script this test fails. The problem is that there are some entry points already setup when installing a dev environment with eachdist develop. This test however checkes the default error handler when no error handler entry points are configured.

@mariojonke
Copy link
Contributor Author

mariojonke commented Oct 16, 2020

@Oberon00

@mariojonke

New feature (non-breaking change which adds functionality)

This changes the SpanProcessor interface, so it is breaking I think.

calling on_start without parent_context should still work as before since it is an optional parameter (unless i overlooked something)
on a second thought it is a breaking change after all since the tracer now explicitly passes the parent_context explicitly to the SpanProcessor

Copy link
Contributor

@codeboten codeboten 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 the change, lgtm

@codeboten codeboten merged commit 2d873e5 into open-telemetry:master Oct 16, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@mariojonke mariojonke deleted the span_processor_on_start_parent branch November 26, 2020 13:14
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.

Update SpanProcessor.on_start to accept parent Context
4 participants