OpenTracing Bridge - Initial implementation

[johananl]

General

This is an initial implementation for the OpenTracing bridge for the Python OpenTelemetry library. There are still some things I would like to handle before considering a merge. On the other hand this is a big PR and I would like to start getting feedback as soon as possible, so I'm opening a draft PR while I work on closing the remaining work items (see TODO section below).

This PR supersedes #120. I have found it easier to open a new PR rather than altering the old one due to a significantly different implementation. Please tell me if I should edit the original PR instead.

Implementation

The PR adds an OpenTracing "shim". The shim is a collection of wrapper classes which should allow an OpenTracing user to instantiate an OpenTelemetry tracer and use it as if it were an OpenTracing tracer, without altering instrumentation code.

TODO

The following should probably be done before merging:

• Squash commits. The current commits aren't properly organized so we should rebase and/or squash them before merging. I'm not sure how granular the commits should be - one big commit for the entire feature? A couple of smaller commits which are divided logically?
• Add documentation. I want to ensure all the new classes and functions have Sphinx-friendly docstrings. Additionally, we should generate the required Sphinx files to have proper docs for the bridge.
• Add typing. Following my conversation with @c24t, it could be nice to have typing info at least for the bridge's public API. This may require writing a typing stub for OpenTracing since the library doesn't currently include typing information.

The following features haven't been implemented yet:

• Inject/extract methods. I haven't got to implementing them yet but there is no blocker for doing so AFAICT.
• Baggage items. The spec doesn't specify baggage support yet, which is probably a blocker.

Testing

To run the unit tests, execute tox. Following are full instructions for running the tests from scratch, in case somebody finds it useful:

git clone git@github.com:kinvolk/opentelemetry-python.git
cd opentelemetry-python
git checkout johananl/ot-shim
virtualenv /tmp/venv
source /tmp/venv/bin/activate
pip install tox
tox

To use the bridge for tracing a dummy app, use the following:

git clone git@github.com:kinvolk/opentelemetry-python.git
cd opentelemetry-python
git checkout johananl/ot-shim
virtualenv /tmp/venv
source /tmp/venv/bin/activate
pip install ext/opentelemetry-ext-opentracing-shim
# An OpenTelemetry tracer is necessary. I've used the SDK here but a different tracer should also work.
pip install opentelemetry-sdk

Then, instrument an app:

import time
from opentelemetry.sdk.trace import Tracer, export
from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter
from opentelemetry.ext.opentracing_shim import create_tracer

otel_tracer = Tracer()
memory_exporter = InMemorySpanExporter()
span_processor = export.SimpleExportSpanProcessor(memory_exporter)
otel_tracer.add_span_processor(span_processor)

tracer = create_tracer(otel_tracer)

print('------------Started------------')

with tracer.start_active_span('parent') as parent:
    time.sleep(0.1)
    with tracer.start_active_span('child') as child:
        time.sleep(0.3)

for s in memory_exporter.get_finished_spans():
    print(s)
    print(s.get_context())

print('-------------Done--------------')

[johananl]

Looks like reviewers don't get tagged in draft PRs. Changing to a regular PR.

[reyang]

Do we want to make this a top level folder? I suggest that we move it to the ext folder.

[c24t]

Nice work! Thanks @johananl.

The current commits aren't properly organized so we should rebase and/or squash them before merging.

If you merge via github we have this repo configured to automatically squash the branch into a single commit, you just have to remember to fix the ugly generated commit message.

I think it's fine to split inject/extract and baggage into separate PRs.

[Oberon00]

In addition:

• Shouldn't that be a namespace package under opentelemetry instead of a new top-level package? EDIT: I think I duplicated OpenTracing Bridge - Initial implementation #211 (comment) here 😄

[c24t]

This looks great. After reviewing I see how type annotations could be helpful here -- the code gets confusing quickly when we've got two different packages with the same class names.

Descriptions for args and return values in docstrings for non-API methods would be helpful. So would a module docstring or README that describes how to use the shim.

It'll be especially helpful to see tests or examples that show code instrumented with only the OpenTracing API, but create OpenTelemetry objects under the hood.

None of my comments are blocking, but I still see a few from @Oberon00. I'll review again and stamp it once those are resolved.

[reyang]

Maybe a dumb question - what's the difference between bridge and shim here?

This PR seems to be focusing on the OpenTracing shim, which means one can use the shim to convert an OpenTelemetry tracer into OpenTracing tracer.

A bridge sounds like bi-directional thing, which also means one can turn an OpenTracing tracer into an OpenTelemetry one.

[johananl]

Maybe a dumb question - what's the difference between bridge and shim here?

@reyang that's a great question actually. I've been using the two terms interchangeably. In the code I've stuck to "shim" for consistency, however in issues etc. I've seen people use both.

I don't have a strong opinion on which one to use, as long as we're consistent with it. We should also remember we have a similar term in other languages.

[johananl]

Rebased over current master. @carlosalberto, could you please merge if things look OK to you? Editing the squashed commit message is going to be a mess... I guess we can call it "Add initial OpenTracing bridge implementation" or something similar.

[c24t]

Editing the squashed commit message is going to be a mess...

@carlosalberto please delete the generated commit message or rewrite it as a nice summary when you merge! No more ugly commits on master. :D

[carlosalberto]

@c24t done ;)