-
Notifications
You must be signed in to change notification settings - Fork 631
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
Removing create_span from the api #290
Conversation
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Codecov Report
@@ Coverage Diff @@
## master #290 +/- ##
==========================================
- Coverage 85.66% 85.55% -0.12%
==========================================
Files 33 33
Lines 1612 1599 -13
Branches 180 178 -2
==========================================
- Hits 1381 1368 -13
Misses 185 185
Partials 46 46
Continue to review full report at Codecov.
|
From #206 (comment): This PR must not remove the capability of specifying explicit start times for the span. This is both in the spec and was explicitly requested, see #134. I guess the parameter should be added to start_span at least. |
Applications that need to create spans detached from the tracer's | ||
context should use this method. |
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.
I would remove this paragraph, and maybe also rename the method to _create_span
. Or we could commit to having this method be a "semi-public" interface of the SDK, to be overriden by any vendor-SDKs that need their own Span type but nevertheless want to reuse functionality from vendor SDKs.
In any case, applications (or any instrumentation use case really) must not use this method.
We should also remove |
This preserves the ability to set the `start_time` of a span that is currently available through calling `.start` Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
return trace_api.DefaultSpan(context=context) | ||
span.start(start_time=start_time) | ||
else: | ||
span = trace_api.DefaultSpan(context=context) |
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 love some feedback on whether or not this is the right thing to do here, since a DefaultSpan no longer has a start()
method, I'm just returning it after creation
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.
I think this is fine, a DefaultSpan doesn't even know whether it was started anyway.
@open-telemetry/python-approvers: We need more reviews for this one! |
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.
Changes LGTM, FYI I pushed some conflict fixes after merging #206.
In 371fa89 I changed some tests that relied on the fact that Also note that because |
@c24t about the test issues: I think we should switch all ext tests to use the SDK and the in-memory exporter instead of I created #303 to track this, I think we should do this in a separate PR, and I still think this one is fine. |
Simplifying the API by removing
create_span
. Updated the WSGI integration and docs.Resolves #152
Signed-off-by: Alex Boten aboten@lightstep.com