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

Implement OpenTracing bridge #120

Closed
wants to merge 1 commit into from

Conversation

alban
Copy link
Contributor

@alban alban commented Sep 4, 2019

This tracer implements the OpenTracing API using OpenTelemetry internally.

This includes unit tests too.


TODO:

  • documentation
  • start time & end time
  • OTel's links / OT's references
  • Baggage: what to do here?
  • Logs: what to do here?
  • Context propagation to other processes: which http headers to use?

This tracer implements the OpenTracing API using OpenTelemetry
internally.

This includes unit tests too.
@carlosalberto
Copy link
Contributor

Hey,

Thanks for the PR, but this is not the approach we are looking for - first of all, I don't see why we need to have a dependency to basictracer (doing so also would mean keeping actually two tracers: an OpenTracing and an OpenTelemetry one).

Second, we only want to expose publicly a way to retrieve an OpenTracing out of an OpenTelemetry Tracer, nothing else:

import opentracingshim

tracer = opentracingshim.create_tracer(io.opentelemetry.tracer())
with tracer.start_active_span("one") as scope:
  scope.span.set_tag("my", "tag")

Which means we cannot any actual implementation of ScopeManager and other members exposed by the OpenTracing API, but simply provide a set of wrapper classes around the OTel components.

See how the WIP looks for Java for reference (mostly missing OT Baggage support): https://github.com/open-telemetry/opentelemetry-java/tree/master/opentracing_shim/src/main/java/io/opentelemetry/opentracingshim (you don't have to port this, as there are enough differences).

@Oberon00 Oberon00 added this to the Alpha Release milestone Sep 13, 2019
@johananl
Copy link
Contributor

Updating here for visibility - I am continuing @alban's work on this following the feedback from @carlosalberto. I shall open a new PR with an initial implementation soon (I can also update this one if preferred).

@johananl
Copy link
Contributor

johananl commented Oct 9, 2019

Superseded by #211. This PR can be closed.

@reyang reyang closed this Oct 9, 2019
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.

None yet

5 participants