Skip to content
This repository has been archived by the owner on Oct 19, 2023. It is now read-only.

OT Bridge - Initial implementation #6

Closed
wants to merge 103 commits into from
Closed

Conversation

johananl
Copy link
Member

@johananl johananl commented Oct 4, 2019

This is an initial implementation of the OpenTracing bridge following received feedback on open-telemetry#120. The bridge is an implementation of the OpenTracing Tracer class and includes several other OpenTracing subclasses required for implementing the tracer.

The following pieces are still not implemented:

  • Inject/extract methods.
  • Baggage items.

To run the unit tests, 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 tox
tox

To test actual tracing, 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
cd opentracing-shim
python setup.py install
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 opentracingshim 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(1)
    with tracer.start_active_span('child') as child:
        time.sleep(2)

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

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

Copy link
Member

@krnowak krnowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments. But I'm not very well informed in the python stuff.

opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General implementation looks very good, just minor comments.

opentracing-shim/README.rst Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
opentracing-shim/setup.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
start_time=None,
ignore_active_span=False,
) -> SpanWrapper:
parent = child_of

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line really needed?

Copy link
Member Author

@johananl johananl Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It saves me from having to specify the else clause. I like this as it's more compact and less indented.
Please feel free to suggest an alternative (we need to allow parent to be None).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right. I didn't consider one of the ifs.

opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
return int(time.time() * 1e9)


# A default event name to be used when logging events when a better event name
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# A default event name to be used when logging events when a better event name
# A default event name to be used for logging events when a better event name

# Verify span hasn't ended.
self.assertIsNone(scope.span.unwrap().end_time)

# Verify span hasn't ended.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Verify span hasn't ended.
# Verify span hasn't ended after scope had been closed.

@johananl johananl force-pushed the johananl/ot-shim branch 2 times, most recently from e60e605 to e3ce0fd Compare October 7, 2019 14:54
Add missing stubs for fully implementing the Span and Scope classes
from OpenTracing.
- Explicitly use opentracing.* to make OpenTracing imports easily
distinguishable.
- Leave OpenTelemetry class names as-is.
Since start_span() is a context manager and since we want the
OpenTelemetry Span to stop when the SpanWrapper stops, we need to
turn start_active_span() into a context manager and yield the
ScopeWrapper rather than return it.
Due to the differences between decimal and binary float
representations, some time values yield "surprising" results when
performing float arithmetic operations to convert seconds to
nanoseconds and vice versa.
"ot" isn't clear enough and could lead to confusion.
If the user passes a custom ScopeManager, we want the TracerWrapper
to call `active_span` on the custom ScopeManager.
Log a warning instead in order not to break application code.
Let the OTel implementation create event timestamps instead of
generating a timestamp in the shim, i.e. outside of OpenTelemetry.
- Remove an unnecessary context manager layers.
- Allow creating `ContextWrapper` objects either from a `SpanWrapper`
  or from a `Span` context manager.
Until we figure out how to handle unsupported frameworks (e.g.
gevent), we remove the `scope_manager` argument of `create_tracer`
to avoid breaking the OpenTelemetry context propagation.
These are obvious and therefore unnecessary.
The API docs are clear about the `references` argument being a list.
We don't try to compensate for wrong argument types elsewhere, so no
reason to do that here.
Not all classes in the shim are actually wrappers, which makes the
XWrapper naming convention confusing. XShim is more neutral.
In the OpenTracing API, when an exception occurs while a span is
active, information about the exception is added to the span as logs
and a tag is added as well. We re-add this functionality to the shim
since we've overridden the `__exit__()` method in which it lives.
Validating the parent type complicates the logic of `start_span()`.
Since the OpenTracing API clearly specifies the valid types for a
parent span, it's the user's responsibility to specify a valid
parent.
Verify that calling `close()` on a `ScopeShim` deactivates the span
in the OpenTelemetry tracer.
We shouldn't return OpenTelemetry objects to the OpenTracing API.
Make it clearer that the provided tracer object should be an
OpenTelemetry tracer, not an OpenTracing tracer.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants