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

Mark the OpenTracing compatibility section as stable #2147

Closed
8 of 9 tasks
carlosalberto opened this issue Nov 22, 2021 · 7 comments · Fixed by #2327
Closed
8 of 9 tasks

Mark the OpenTracing compatibility section as stable #2147

carlosalberto opened this issue Nov 22, 2021 · 7 comments · Fixed by #2327
Assignees

Comments

@carlosalberto
Copy link
Contributor

carlosalberto commented Nov 22, 2021

This issue will serve to track the remaining work to mark the OpenTracing compatibility as stable:

The plan has been updated to mark the Spec as stable in very early Q1, and have the first compliant implementations ready by Q1 a well (Python and Java for a start).

@bhs @yurishkuro @tedsuo Please review in case anything else needs to be added.

@carlosalberto
Copy link
Contributor Author

@yurishkuro Please review, as I've update this with a few issues that appeared lately.

@carlosalberto
Copy link
Contributor Author

We have completed all the important tasks, and I'd only wait for #2298 and #2297 as final improvements before we mark this section stable (we could work on the porting guides/documentation after the release).

Opinions @bhs @yurishkuro @tedsuo ?

@bhs
Copy link

bhs commented Feb 8, 2022

Mega-apologies for the delay on this. I'm fine marking this as stable, particularly given that OT is now officially archived.

@yurishkuro
Copy link
Member

sgtm

@jack-berg
Copy link
Member

I've been thinking a bit about this PR to implement Tracer.close() in the java OpenTracing shim.

The OpenTracing shim spec says:

This functionality will be provided as a bridge layer implementing the OpenTracing API using the OpenTelemetry API. This layer MUST NOT rely on implementation specific details of any SDK.

Why does the shim operate on the API instead of SDK? Couple reasons it might make sense to instead depend on the SDK:

  • There's precedent. The OpenCensus metrics shim depends on the metrics SDK, and there's really no way to implement it using just the metrics API, and I would be opposed to extending the metrics API to support it.
  • Based on the opentracing migration guide, it appears that application owners will be the ones using the OpenTracing shim, not instrumentation authors. Application owners don't need to be shielded from the SDK dependency.
  • Having the shim depend on the SDK would make implementing the close operation trivial.

@carlosalberto
Copy link
Contributor Author

Hey @jack-berg

This layer MUST NOT rely on implementation specific details of any SDK.

I think this is improper wording for the Specification - i.e. we wanted to prevent users from using telemetry-specific functionality in the SDKs, i.e. allow the Shim to specifically configure sampling in the reference Java implementation, or configure Resource, for example. In that regard, checking a standard Closeable interface wouldn't be considered an offender (will send an amendment to the Spec this week).

So overall there are way more reasons for the OpenCensus to use the SDK, as OpenCensus itself extensively exposes sampling, resources and other very similar implementation details compared to OTel.

So there are a pair of reasons why I would also prefer to keep it the current way:

  1. The OT compatibility section has been declared stable.
  2. The Tracer.Close() operation is something we added rather late in the OT lifecycle, and hence only two languages implemented it (Java and C++). I don't feel this justifies changing the dependency to the SDK, and we can do a best-effort thing in Java and C++ sides.

cc @yurishkuro

@yurishkuro
Copy link
Member

@carlosalberto +1 not depending on SDK. But you'd probably still want some language in the spec about Close for the OT languages that supported it. I think for Java it's trivial by checking against an interface, would the same approach be used for C++?

Another alternative is to simply say Close() is not supported by the bridge. I don't recall the reasons why it was allowed in OT, I always felt like it belongs to implementations since someone needs to instantiate them explicitly. And for the auto-instantiation via service registry, the Close could've been a part of that mechanism, not of the Tracer. I am rather interested who would complain about in the first place - do you have any signal of how many people are relying on the shims? My experience with the Go shim was ... mixed.

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 a pull request may close this issue.

5 participants