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

Separate no-op from interfaces #311

Merged
merged 13 commits into from
Jan 15, 2020
12 changes: 6 additions & 6 deletions ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ def test_explicit_start_time(self):
now = time.time()
with self.shim.start_active_span("TestSpan", start_time=now) as scope:
result = util.time_seconds_from_ns(scope.span.unwrap().start_time)
# Tolerate inaccuracies of less than a microsecond.
# TODO: Put a link to an explanation in the docs.
# Tolerate inaccuracies of less than a microsecond. See Note:
# https://open-telemetry.github.io/opentelemetry-python/opentelemetry.ext.opentracing_shim.html
# TODO: This seems to work consistently, but we should find out the
# biggest possible loss of precision.
self.assertAlmostEqual(result, now, places=6)
Expand All @@ -146,8 +146,8 @@ def test_explicit_end_time(self):
span.finish(now)

end_time = util.time_seconds_from_ns(span.unwrap().end_time)
# Tolerate inaccuracies of less than a microsecond.
# TODO: Put a link to an explanation in the docs.
# Tolerate inaccuracies of less than a microsecond. See Note:
# https://open-telemetry.github.io/opentelemetry-python/opentelemetry.ext.opentracing_shim.html
# TODO: This seems to work consistently, but we should find out the
# biggest possible loss of precision.
self.assertAlmostEqual(end_time, now, places=6)
Expand Down Expand Up @@ -412,8 +412,8 @@ def test_log_kv(self):
span.unwrap().events[1].timestamp
)
self.assertEqual(span.unwrap().events[1].attributes["foo"], "bar")
# Tolerate inaccuracies of less than a microsecond.
# TODO: Put a link to an explanation in the docs.
# Tolerate inaccuracies of less than a microsecond. See Note:
# https://open-telemetry.github.io/opentelemetry-python/opentelemetry.ext.opentracing_shim.html
# TODO: This seems to work consistently, but we should find out the
# biggest possible loss of precision.
self.assertAlmostEqual(result, now, places=6)
Expand Down
31 changes: 29 additions & 2 deletions opentelemetry-api/src/opentelemetry/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,15 @@ def record(self, label_set: LabelSet, value: ValueT) -> None:


# pylint: disable=unused-argument
class Meter:
class Meter(abc.ABC):
"""An interface to allow the recording of metrics.

`Metric` s are used for recording pre-defined aggregation (gauge and
counter), or raw values (measure) in which the aggregation and labels
for the exported metric are deferred.
"""

@abc.abstractmethod
def record_batch(
self,
label_set: LabelSet,
Expand All @@ -211,6 +212,7 @@ def record_batch(
corresponding value to record for that metric.
"""

@abc.abstractmethod
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on abstract methods having implementations? I see how it could be useful to have a safe default implementation that the user has to explicitly call with super, but I see that DefaultMeter has its own implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I'll remove the default implementation from the interfaces. I don't have a strong preference either ways, but it's less changes to remove the few that were left behind than to add an implementation for all abstract methods.

def create_metric(
self,
name: str,
Expand Down Expand Up @@ -251,6 +253,31 @@ def get_label_set(self, labels: Dict[str, str]) -> "LabelSet":
return DefaultLabelSet()


class DefaultMeter(Meter):
"""The default Meter used when no Meter implementation is available."""

def record_batch(
self,
label_set: LabelSet,
record_tuples: Sequence[Tuple["Metric", ValueT]],
) -> None:
pass

def create_metric(
self,
name: str,
description: str,
unit: str,
value_type: Type[ValueT],
metric_type: Type[MetricT],
label_keys: Sequence[str] = None,
enabled: bool = True,
monotonic: bool = False,
) -> "Metric":
# pylint: disable=no-self-use
return DefaultMetric()


Copy link
Contributor

Choose a reason for hiding this comment

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

I think get_label_set will be needed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, i'd forgotten to make get_label_set abstract.

# Once https://github.com/python/mypy/issues/7092 is resolved,
# the following type definition should be replaced with
# from opentelemetry.util.loader import ImplementationFactory
Expand All @@ -269,7 +296,7 @@ def meter() -> Meter:

if _METER is None:
# pylint:disable=protected-access
_METER = loader._load_impl(Meter, _METER_FACTORY)
_METER = loader._load_impl(DefaultMeter, _METER_FACTORY)
del _METER_FACTORY

return _METER
Expand Down
96 changes: 85 additions & 11 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
.. versionadded:: 0.1.0
"""

import abc
import enum
import types as python_types
import typing
Expand Down Expand Up @@ -142,9 +143,10 @@ class SpanKind(enum.Enum):
CONSUMER = 4


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

@abc.abstractmethod
def end(self, end_time: int = None) -> None:
"""Sets the current time as the span's end time.

Expand All @@ -154,6 +156,7 @@ def end(self, end_time: int = None) -> None:
implementations are free to ignore or raise on further calls.
"""

@abc.abstractmethod
def get_context(self) -> "SpanContext":
"""Gets the span's SpanContext.

Expand All @@ -164,12 +167,14 @@ def get_context(self) -> "SpanContext":
A :class:`.SpanContext` with a copy of this span's immutable state.
"""

@abc.abstractmethod
def set_attribute(self, key: str, value: types.AttributeValue) -> None:
"""Sets an Attribute.

Sets a single Attribute with the key and value passed as arguments.
"""

@abc.abstractmethod
def add_event(
self,
name: str,
Expand All @@ -183,12 +188,14 @@ def add_event(
timestamp if the `timestamp` argument is omitted.
"""

@abc.abstractmethod
def add_lazy_event(self, event: Event) -> None:
"""Adds an `Event`.

Adds an `Event` that has previously been created.
"""

@abc.abstractmethod
def update_name(self, name: str) -> None:
"""Updates the `Span` name.

Expand All @@ -198,13 +205,15 @@ def update_name(self, name: str) -> None:
on the implementation.
"""

@abc.abstractmethod
def is_recording_events(self) -> bool:
"""Returns whether this span will be recorded.

Returns true if this Span is active and recording information like
events with the add_event operation and attributes using set_attribute.
"""

@abc.abstractmethod
def set_status(self, status: Status) -> None:
"""Sets the Status of the Span. If used, this will override the default
Span status, which is OK.
Expand Down Expand Up @@ -348,6 +357,29 @@ def get_context(self) -> "SpanContext":
def is_recording_events(self) -> bool:
return False

def end(self, end_time: int = None) -> None:
pass

def set_attribute(self, key: str, value: types.AttributeValue) -> None:
pass

def add_event(
self,
name: str,
attributes: types.Attributes = None,
timestamp: int = None,
) -> None:
pass

def add_lazy_event(self, event: Event) -> None:
pass

def update_name(self, name: str) -> None:
pass

def set_status(self, status: Status) -> None:
pass


INVALID_SPAN_ID = 0x0000000000000000
INVALID_TRACE_ID = 0x00000000000000000000000000000000
Expand All @@ -360,7 +392,7 @@ def is_recording_events(self) -> bool:
INVALID_SPAN = DefaultSpan(INVALID_SPAN_CONTEXT)


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

This class provides methods for manipulating the context, creating spans,
Expand All @@ -369,8 +401,9 @@ class Tracer:

# Constant used to represent the current span being used as a parent.
# This is the default behavior when creating spans.
CURRENT_SPAN = Span()
CURRENT_SPAN = DefaultSpan(INVALID_SPAN_CONTEXT)
c24t marked this conversation as resolved.
Show resolved Hide resolved

@abc.abstractmethod
def get_current_span(self) -> "Span":
"""Gets the currently active span from the context.

Expand All @@ -381,9 +414,8 @@ def get_current_span(self) -> "Span":
The currently active :class:`.Span`, or a placeholder span with an
invalid :class:`.SpanContext`.
"""
# pylint: disable=no-self-use
return INVALID_SPAN

@abc.abstractmethod
def start_span(
self,
name: str,
Expand Down Expand Up @@ -427,10 +459,9 @@ def start_span(
Returns:
The newly-created span.
"""
# pylint: disable=unused-argument,no-self-use
return INVALID_SPAN

@contextmanager # type: ignore
@abc.abstractmethod
def start_as_current_span(
self,
name: str,
Expand Down Expand Up @@ -480,10 +511,8 @@ def start_as_current_span(
The newly-created span.
"""

# pylint: disable=unused-argument,no-self-use
yield INVALID_SPAN

@contextmanager # type: ignore
@abc.abstractmethod
def use_span(
self, span: "Span", end_on_exit: bool = False
) -> typing.Iterator[None]:
Expand All @@ -501,6 +530,46 @@ def use_span(
end_on_exit: Whether to end the span automatically when leaving the
context manager.
"""


class DefaultTracer(Tracer):
"""The default Tracer, used when no Tracer implementation is available.

All operations are no-op.
"""

def get_current_span(self) -> "Span":
# pylint: disable=no-self-use
return INVALID_SPAN

def start_span(
self,
name: str,
parent: ParentSpan = Tracer.CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
attributes: typing.Optional[types.Attributes] = None,
links: typing.Sequence[Link] = (),
start_time: typing.Optional[int] = None,
) -> "Span":
# pylint: disable=unused-argument,no-self-use
return INVALID_SPAN

@contextmanager # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

What's the right solution for context manager types? I see we've been adding # type: ignore to these since (at least) #92, but couldn't find a discussion about this.

Copy link
Member

Choose a reason for hiding this comment

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

This is tracked in #219. The best solution I know is still #198 (comment).

def start_as_current_span(
self,
name: str,
parent: ParentSpan = Tracer.CURRENT_SPAN,
kind: SpanKind = SpanKind.INTERNAL,
attributes: typing.Optional[types.Attributes] = None,
links: typing.Sequence[Link] = (),
) -> typing.Iterator["Span"]:
# pylint: disable=unused-argument,no-self-use
yield INVALID_SPAN

@contextmanager # type: ignore
def use_span(
self, span: "Span", end_on_exit: bool = False
) -> typing.Iterator[None]:
# pylint: disable=unused-argument,no-self-use
yield

Expand All @@ -525,7 +594,12 @@ def tracer() -> Tracer:

if _TRACER is None:
# pylint:disable=protected-access
_TRACER = loader._load_impl(Tracer, _TRACER_FACTORY)
try:
_TRACER = loader._load_impl(Tracer, _TRACER_FACTORY) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_TRACER = loader._load_impl(Tracer, _TRACER_FACTORY) # type: ignore
_TRACER = loader._load_impl(DefaultTracer, _TRACER_FACTORY) # type: ignore

Would this do the same thing as the try/catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a DefaultTracer would mean any implementation would be extending DefaultTracer rather than the Tracer interface, which means we would lose the value of using abstract methods to signal what methods are expected to be implemented.

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 this try: catch should really be moved to the loader. Probably some reflection will be required (or a class attribute DEFAULT_IMPLEMENTATION_CLASS linking the two (alternatively INTERFACE_CLASS in the other direction)

Copy link
Member

@Oberon00 Oberon00 Dec 6, 2019

Choose a reason for hiding this comment

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

But if I were a vendor, I would definitely inherit from DefaultTracer instead of Tracer because I don't want my code to crash the customer's app if they start calling a new method that my Tracer doesn't (yet) implement. Which was also one argument by me (and I think @carlosalberto agreed) against this ABC/Default split in #66 (#66 (comment) to be exact).

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 that's a really good argument that almost convinces me against ABCs. I'm +1 to doing that work in this PR, or going back to switching to no-ops once we have all of this standardized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, definitely same feeling about vendors ending up inheriting from DefaultTracer.

except TypeError:
# if we raised an excaption trying to instantiate an
# abstract class, default to no-op tracer impl
_TRACER = DefaultTracer()
del _TRACER_FACTORY

return _TRACER
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-api/tests/metrics/test_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
# pylint: disable=no-self-use
class TestMeter(unittest.TestCase):
def setUp(self):
self.meter = metrics.Meter()
self.meter = metrics.DefaultMeter()

def test_record_batch(self):
counter = metrics.Counter()
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-api/tests/test_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
DUMMY_TRACER = None


class DummyTracer(trace.Tracer):
class DummyTracer(trace.DefaultTracer):
pass


Expand All @@ -49,7 +49,7 @@ def setUp(self):

def test_get_default(self):
tracer = trace.tracer()
self.assertIs(type(tracer), trace.Tracer)
self.assertIs(type(tracer), trace.DefaultTracer)
Copy link
Member

Choose a reason for hiding this comment

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

@Oberon00 might want to check that this is testing the right thing still. It seems like DummyTracer should still extend Tracer here, but that seems to rely on instantiating Tracers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively here I can implement the abstract methods in DummyTracer and that should work as well

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 implementing the abstract methods would be worth it here.


def test_preferred_impl(self):
trace.set_preferred_tracer_implementation(
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-api/tests/trace/test_tracer.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

class TestTracer(unittest.TestCase):
def setUp(self):
self.tracer = trace.Tracer()
self.tracer = trace.DefaultTracer()

def test_get_current_span(self):
span = self.tracer.get_current_span()
Expand All @@ -34,6 +34,6 @@ def test_start_as_current_span(self):
self.assertIsInstance(span, trace.Span)

def test_use_span(self):
span = trace.Span()
span = trace.DefaultSpan(trace.INVALID_SPAN_CONTEXT)
with self.tracer.use_span(span):
pass