-
Notifications
You must be signed in to change notification settings - Fork 887
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
Opencensus compatibility specification #1332
Opencensus compatibility specification #1332
Conversation
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
7dbd03b
to
debc173
Compare
@anuraaga Looks like i had a dropped commit when rebasing that lost all the committed suggestions from upstream, trying to recollect those now. |
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.
Just a few smaller points but this seems to be good, thanks!
This adapter MUST use an OpenTelemetry `TextMapPropagator` to implement the | ||
OpenCensus `TextFormat`. | ||
|
||
This adapter SHOULD use configured OpenTelemetry `TextMapPropagator` on the |
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.
Just checking, is this something Java can't do but other languages may? For example, we have explicit extension points for B3 and W3C
So it seems like it wouldn't ever make sense to use the OTel configured propagator instead of directly exposing the correct implementations for this sort of interface.
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 can clarify this. I actually wanted to specify it so that OpenCensus shim will use the globallly configured propogator (see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#get-global-propagator). Right now the implementation DOES NOT do this, but I'd like to fix it. Happy to hear thoughts/.copncerns.
My main reasoning for this shift from the Java impl is because I'd like to give OTel configuration the ability to control the impl of the propogator. One of the "principles" here is when adopting the shim, OTEL configuration should override OC, always.
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.
Probably you may want to state this principal somewhere.
This adapter SHOULD use configured OpenTelemetry `TextMapPropagator` on the | ||
OpenTelemetry `TraceProvider` for text format propagation. | ||
|
||
This adapter MUST provide a default `W3CTraceContextPropagator` in lieu of a |
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.
Not sure what this sentence means. Is it the same as my above point, where we actually don't want to use the configured provider? More concretely, this line of code in Java?
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.
NO, this is related to selecting the W3C propagatoor off the global (see: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#get-global-propagator) and if there is none available, providing a default.
I'll take a stab at rewording this, need to think about a better phrasing.
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.
Take look at the rewording, hopefully it's more clear.
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>
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.
Some minor comments, LGTM
Some OpenCensus APIs support "debug" and "defer" tracing flags in additon to | ||
"sampled". In this case, the OpenCensus bridge will do its best to support | ||
and translate unspecified flags into the closest OpenTelemetry equivalent. |
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.
facepalm :)))
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.
This adapter MUST use an OpenTelemetry `TextMapPropagator` to implement the | ||
OpenCensus `TextFormat`. | ||
|
||
This adapter SHOULD use configured OpenTelemetry `TextMapPropagator` on the |
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.
Probably you may want to state this principal somewhere.
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.
Please add a status
@bogdandrutu for "status" do you mean the stable, experimental etc? I'll go look up the rules and annotate sections accordingly. |
@jsuereth yes what we call the |
@jsuereth ping :) |
1 similar comment
@jsuereth ping :) |
@bogdandrutu Will get to this later today/tomorrow. Sorry for delays, had a lot of urgent things / Scala.Love conference prep to do last week. |
@bogdandrutu Ok, so I updated stability guarantees. Here's the plan:
Sound reasonable? |
#### Known Incompatibilities | ||
|
||
Below are listed known incompatibilities between OpenTelemetry and OpenCensus | ||
specifications. Applications leveraging unspecified behavior from OpenCensus |
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.
specifications. Applications leveraging unspecified behavior from OpenCensus | |
specifications. Applications leveraging unspecified behavior from OpenCensus |
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.
There are other places in the doc that also have 3 SPCs.
details of the OpenTelemetry SDK. | ||
|
||
More specifically, the intention is to allow OpenCensus instrumentation to be | ||
recorded using OpenTelemetry. This Shim Layer MUST NOT publicly expose any |
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.
Might need to clarify where the shim should go - in OpenTelemetry project or OpenCensus project.
OpenTelemetry already supports two tracing APIs: OpenTelemetry and OpenTracing. We invented a new tracing API, but continue to support the old 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.
I can do. In practice, we've actually released new OpenCensus versions that expose hooks for the shim, and the shim lives on the OTEL side (as that's where you configure your pipeline).
As an FYI for you, I expect we might have to do something similar in Python/JavaScript if we run into similar issues as what we found in Go/Java.
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 as "Experimental".
@jsuereth please fix the build 👯 |
@bogdandrutu fixed :) Sorry, for some reason can't run that check locally. |
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
Fixes #766
Changes
Provide a specification of OpenCensus migration to open telemetry.
A few notes for discussion: