-
Notifications
You must be signed in to change notification settings - Fork 249
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
Added set_status to span, updated pymongo integration #738
Added set_status to span, updated pymongo integration #738
Conversation
Should we update span to initiate with
|
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.
LGTM. Feel free to continue work on the default value for Status
class, and let me know when you're ready to merge it.
Would it be okay to rewrite the
This is a lot closer to the specification. If it isn't ok because it is a breaking change making |
@reyang updated |
It looks good to me! I don't know if the SDK consumers would use the status class directly. What they would do is to set status, and this is the place we might break them. Given we're still in beta, I guess it is probably okay to have breaking changes for a small set of customers? @c24t for more input. |
The change may be ok, but in general OpenCensus does not follow the OpenTelemetry spec. This isn't described in the OC spec. |
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.
The code and tests look great, but renaming Status.code
and Status.message
is a breaking change for no obvious benefit. LGTM otherwise.
@@ -1,6 +1,8 @@ | |||
# Changelog | |||
|
|||
## Unreleased | |||
- Changed attributes names to match [specs](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-semantic-conventions.md) |
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.
We shouldn't link to the OT spec here.
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 have slightly different opinion on this.
I agree that OpenCensus Python should follow OpenCensus spec. Following OpenTelemetry spec is not the goal here, but it is nice to have if:
- It doesn't go against OpenCensus spec.
- It doesn't introduce major breaking changes (given we're moving to OpenTelemetry soon) without a good rationale.
- It helps us to form better understanding on our path to OpenTelemetry, or simply make the job easier when we try to steal some code and morph them into OpenTelemetry.
With these, I think having a link to OT spec is similar like saying "we are supporting W3C CorrelationContext version 2" - not something required in OC spec, but a nice to have feature.
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 agree with all that, I'm just concerned about putting a link to the OT spec in something as public as the release notes. There's already a lot of confusion around OC/OT, linking the OT spec here might invite more.
opencensus/trace/status.py
Outdated
self.message = message | ||
def __init__(self, code, message=None, details=None): | ||
self._code = code | ||
self._message = message |
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.
The changes to add set_status
look good, but this looks like a breaking change for the sake of matching the names in a different spec.
@c24t thanks for the review! Just some clarifications:
My idea was to don't do anything that breaks the OpenCensus Specs and change some internals to make an easier transition in the future (OpenTelemetry).
Is it okay to maintain the |
@victoraugustolls I'm fine with the change, and I've signed off. BTW, @victoraugustolls I've sent some messages to you on gitter, would you take a look? Thanks! |
@victoraugustolls we already have 0.7.0 release done. We can consider a minor release (e.g. 0.7.1) if there is no breaking change. |
That sound great to me. Even though it's a minor change and doesn't technically run afoul of OC spec I think it's better to keep the existing names. |
Done, also I updated |
Looks great, thanks @victoraugustolls! |
Added
set_status
to spans, according to specificationAdded
set_status_to_current_span
toTracer
Swapped pymongo integration attributes to specs described here
Added tests do maintain 100% coverage