Skip to content

Add initial tracer API #11

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

Merged
merged 13 commits into from
Jun 20, 2019
Empty file added opentelemetry/__init__.py
Empty file.
Empty file added opentelemetry/api/__init__.py
Empty file.
190 changes: 190 additions & 0 deletions opentelemetry/api/trace/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
# Copyright 2019, OpenTelemetry Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""
The OpenTelemetry tracing API describes the classes used to generate
distributed traces.
Copy link
Member Author

Choose a reason for hiding this comment

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

These docs are WIP, I just wanted a good example module docstring here to test the generated html docs.


The :class:`.Tracer` class controls access to the execution context, and
manages span creation. Each operation in a trace is represented by a
:class:`.Span`, which records the start, end time, and metadata associated with
the operation.

This module provides abstract (i.e. unimplemented) classes required for
tracing, and a concrete no-op ``BlankSpan`` that allows applications to use the
API package alone without a supporting implementation.

The tracer supports implicit and explicit context propagation. By default spans
are created as children of the currently active span, and the newly-created
span becomes the new active span::

from opentelemetry.sdk.trace import tracer

# Create a new root span, set it as the current span in context
with tracer.span("parent"):
# Attach a new child and update the current span
with tracer.span("child"):
do_work():
# Close child span, set parent as current
# Close parent span, set default span as current

Under explicit context propagation there is no concept of an active span, and
the caller is responsible for managing the span's lifetime::

from opentelemetry.sdk.trace import tracer
from your.integration import deserialize_span

parent = deserialize_span(serialized_span)
# Explicit parent span assignment
with tracer.span("child", parent=parent) as child:
Copy link
Member

Choose a reason for hiding this comment

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

How would the implicit one and explicit one work together in a mixed mode?
For example, I have a function A which is using implicit in-proc propagation, another function B which is using explicit propagation, if I need to call A from B, what should I do?

Copy link
Member

Choose a reason for hiding this comment

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

I think functions that want to support both styles (e.g. in libraries) will need to add an optional parameter for the parent Span that defaults to the current one.

Copy link
Member

Choose a reason for hiding this comment

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

Going down this path, I would imagine either all the libraries end up having parent span as the optional parameter for all the exposed functions, or most libraries will only support implicit context and makes it super difficult for the application developer to use explicit context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Supporting both implicit and explicit context like this is going to make for an ugly implementation. The spec should probably describe what happens for each, and I think there's good reason to separate the span constructors for each. We may need to solve this in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

So honestly adding support for explicit parent at the core API shouldn't look that bad - for libraries (i.e. Django instrumentation), I think it's fine to make it super difficult for the user to override it, or even do not expose it at all IHMO.

do_work(span=child)

Applications should generally use a single global tracer, and use either
implicit or explicit context propagation consistently throughout.
Copy link
Member

Choose a reason for hiding this comment

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

How about libraries? The application could use explicit propagation consistently, while calling into a library which expects implicit context propagation.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we definitely should think about and document best practices for what libs should do. For that question, see https://github.com/open-telemetry/opentelemetry-python/pull/11/files#r290703075. Another question is what tracer they should use (e.g.: Use global tracer? Pass tracer at every call? Provide way to set library-wide tracer? Provide way to set a library-wide callback function that it calls to get the tracer?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Best-practices aside, users should have the final thing to say - if they, for some reason, need to 'mix' these modes (as you guys call it), that should be supported IHMO.

(As for the other questions, I'd say using a global Tracer would be a good thing to have)

Copy link
Member Author

Choose a reason for hiding this comment

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

The application could use explicit propagation consistently, while calling into a library which expects implicit context propagation.

This is a problem since it would mean that passing an explicit context should change the implicit one. This would be unexpected in the application, but required if it's calling into a library that uses implicit context.

Provide way to set a library-wide callback function that it calls to get the tracer?

I thought implementations would provide a global tracer, do you want the callback so integrations can change this?


.. versionadded:: 0.1.0
"""

from __future__ import annotations

from typing import Iterator
from contextlib import contextmanager


class Tracer(object):
"""Handles span creation and in-process context propagation.

This class provides methods for manipulating the context, creating spans,
and controlling spans' lifecycles.
"""

def get_current_span(self) -> Span:
"""Get the currently active span from the context.

Returns:
The currently active :class:`.Span`.
"""
raise NotImplementedError

@contextmanager
def span(self, name: str, parent: Span=None) -> Iterator[None]:
"""Context manager for span creation.

Create a new child of the current span, or create a root span if no
current span exists. Start the span and set it as the current span in
this tracer's context.

On exiting the context manager stop the span and set its parent as the
current span.

Example::

with tracer.span("one") as parent:
parent.add_event("parent's event")
with tracer.span("two") as child:
child.add_event("child's event")
tracer.get_current_span() # returns child
tracer.get_current_span() # returns parent
tracer.get_current_span() # returns the previously active span

Args:
name: The name of the span to be created.
parent: This span's parent.

Raises:
ValueError: if ``name`` is null.
"""
raise NotImplementedError

def create_span(self, name: str, parent: Span=None) -> Span:
Copy link
Member

Choose a reason for hiding this comment

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

How to create a root span which has no parent?

Copy link
Member

Choose a reason for hiding this comment

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

True. In fact, I think its strange that create_span(..., parent=None) should have a non-None parent. One suggestion: create_span should just never affect or use (as parent) the active span. But instead, enhance the context manager returned by span to return the created span at __enter__.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can think of a few options here: use a sentinel default value instead of null and take parent=None to mean that we should create a root span, add a separate method create_root_span, or change the way we create spans and match java's builder pattern more closely.

create_span should just never affect or use (as parent) the active span

That's pretty surprising, I'd expect creating a child of the current active span to be the most common use case.

But instead, enhance the context manager returned by span to return the created span at __enter__

That's what I'd expect the context manager to do now.

"""Create a new span as a child of the currently active span.

If called with ``parent`` this method won't affect the tracer's
context.

See ``span`` for a context manager that controls the span's lifetime.

Args:
name: The name of the span to be created.
parent: This span's parent.

Raises:
ValueError: if ``name`` is null.
Copy link
Member

Choose a reason for hiding this comment

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

Do we only check if name is null or we expect more validations?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally advocate for using a generic name here, instead of failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, it sounds like implementations should provide a safe default here. In general, do you expect implementations to be more or less permissive than the API? I.e. should implementations be required to throw errors we list in the API? Should they be allowed to throw errors we don't list?

Copy link
Member

Choose a reason for hiding this comment

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

In general, do you expect implementations to be more or less permissive than the API? I.e. should implementations be required to throw errors we list in the API?

Yes, I bet almost all implementations will have more restriction than the API would allow.

Should they be allowed to throw errors we don't list?

Probably not. One design principle we may want to follow is "the monitoring SDK will try to keep silent instead of changing the application behavior". In this spirit I guess we will have exporters doing some downcast/trimming based on their specific need, and not throw exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case: do we want to allow implementations to throw here on null/blank name by including it in the API or are we ok with forbidding them from throwing by removing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I bet almost all implementations will have more restriction than the API would allow

This suggests that implementations will throw errors we don't list in the API, or am I missing something?

FWIW the java API says to raise a NPE here. It might be worth taking this conversation there (or to the spec repo).

Using a safe default span name sounds good to me as long as we get the same behavior in all clients, or decide that this is "unspecified behavior" and that different clients can do this differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'm in favor of option 1.

I'm also in favor one. That being said, this can also be mentioned/discussed in the Specification ;)

Copy link
Member

Choose a reason for hiding this comment

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

I'd also prefer option 1. Option 3 (generating a random span name) is no good IMHO, because span names should be low-cardinality.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added open-telemetry/opentelemetry-specification#141 to track this in specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

It sounds like we should list any error implementations could throw in the api method docstring. Safe default names sound good to me, but let's clarify this in the spec first. I added #18 to track this.

"""
raise NotImplementedError


class Span(object):
"""A span represents a single operation within a trace.
"""

def start(self) -> None:
"""Set the current time as the span's start time.

Each span represents a single operation. The span's start time is the
wall time at which the operation started.

Only the first call to ``start`` should modify the span, and
implementations are free to ignore or raise on further calls.
"""
raise NotImplementedError

def end(self) -> None:
"""Set the current time as the span's end time.

The span's end time is the wall time at which the operation finished.

Only the first call to ``end`` should modify the span, and
implementations are free to ignore or raise on further calls.
"""
raise NotImplementedError

def get_context(self) -> SpanContext:
"""Get an immutable snapshot of this span.

Returns:
A :class:`.SpanContext` with a copy of this span's immutable state.
"""
raise NotImplementedError


class SpanContext(object):
"""The state of a Span to propagate between processes.

This class includes the immutable attributes of a :class:`.Span` that must
be propagated to a span's children and across process boundaries.

Args:
trace_id: The ID of the trace that this span belongs to.
span_id: This span's ID.
options: Global trace options to propagate.
state: Global tracing-system-specific info to propagate.
"""

def __init__(self,
trace_id: str,
span_id: str,
options: TraceOptions,
state: TraceState) -> SpanContext:
pass


# TODO
class TraceOptions(int):
pass


# TODO
class TraceState(dict):
pass