Skip to content

Implement IdsGenerator interface for TracerProvider and include default RandomIdsGenerator #1153

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.

Sign up for GitHub

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 2 commits into from
Oct 1, 2020
Merged

Implement IdsGenerator interface for TracerProvider and include default RandomIdsGenerator #1153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a73f77
Add interface to make trace & span id customizable
NathanielRN Sep 23, 2020
56e9ac5
Update tests to use new ids generator
NathanielRN Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions docs/api/trace.ids_generator.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
opentelemetry.trace.ids_generator
=================================

.. automodule:: opentelemetry.trace.ids_generator
:members:
:undoc-members:
:show-inheritance:
1 change: 1 addition & 0 deletions docs/api/trace.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Submodules

trace.status
trace.span
trace.ids_generator
Copy link
Contributor Author

@NathanielRN NathanielRN Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Adding it to the docs here for visibility. Also because both span.py is added and they are in the same directory level and similar in that they both define an interface.


Module contents
---------------
Expand Down
6 changes: 3 additions & 3 deletions docs/examples/opentelemetry-example-app/tests/test_flask_example.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import opentelemetry_example_app.flask_example as flask_example
from opentelemetry import trace
from opentelemetry.sdk import trace as trace_sdk


class TestFlaskExample(unittest.TestCase):
Expand All @@ -46,7 +45,8 @@ def tearDown(self):
self.send_patcher.stop()

def test_full_path(self):
trace_id = trace_sdk.generate_trace_id()
ids_generator = trace.RandomIdsGenerator()
trace_id = ids_generator.generate_trace_id()
# We need to use the Werkzeug test app because
# The headers are injected at the wsgi layer.
# The flask test app will not include these, and
Expand All @@ -58,7 +58,7 @@ def test_full_path(self):
headers={
"traceparent": "00-{:032x}-{:016x}-{:02x}".format(
trace_id,
trace_sdk.generate_span_id(),
ids_generator.generate_span_id(),
trace.TraceFlags.SAMPLED,
)
},
Expand Down
9 changes: 5 additions & 4 deletions exporter/opentelemetry-exporter-datadog/tests/test_datadog_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,12 @@ def get_as_list(dict_object, key):
class TestDatadogFormat(unittest.TestCase):
@classmethod
def setUpClass(cls):
ids_generator = trace_api.RandomIdsGenerator()
cls.serialized_trace_id = propagator.format_trace_id(
trace.generate_trace_id()
ids_generator.generate_trace_id()
)
cls.serialized_parent_id = propagator.format_span_id(
trace.generate_span_id()
ids_generator.generate_span_id()
)
cls.serialized_origin = "origin-service"

Expand Down Expand Up @@ -107,7 +108,7 @@ def test_context_propagation(self):
"child",
trace_api.SpanContext(
parent_context.trace_id,
trace.generate_span_id(),
trace_api.RandomIdsGenerator().generate_span_id(),
is_remote=False,
trace_flags=parent_context.trace_flags,
trace_state=parent_context.trace_state,
Expand Down Expand Up @@ -152,7 +153,7 @@ def test_sampling_priority_auto_reject(self):
"child",
trace_api.SpanContext(
parent_context.trace_id,
trace.generate_span_id(),
trace_api.RandomIdsGenerator().generate_span_id(),
is_remote=False,
trace_flags=parent_context.trace_flags,
trace_state=parent_context.trace_state,
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
([#1123](https://github.com/open-telemetry/opentelemetry-python/pull/1123))
- Store `int`s as `int`s in the global Configuration object
([#1118](https://github.com/open-telemetry/opentelemetry-python/pull/1118))
- Allow for Custom Trace and Span IDs Generation - `IdsGenerator` for TracerProvider
([#1153](https://github.com/open-telemetry/opentelemetry-python/pull/1153))

## Version 0.13b0

Expand Down
3 changes: 3 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
from contextlib import contextmanager
from logging import getLogger

from opentelemetry.trace.ids_generator import IdsGenerator, RandomIdsGenerator
from opentelemetry.trace.propagation import (
get_current_span,
set_span_in_context,
Expand Down Expand Up @@ -436,6 +437,7 @@ def get_tracer_provider() -> TracerProvider:
__all__ = [
"DEFAULT_TRACE_OPTIONS",
"DEFAULT_TRACE_STATE",
"IdsGenerator",
"INVALID_SPAN",
"INVALID_SPAN_CONTEXT",
"INVALID_SPAN_ID",
Expand All @@ -446,6 +448,7 @@ def get_tracer_provider() -> TracerProvider:
"Link",
"LinkBase",
"ParentSpan",
"RandomIdsGenerator",
"Span",
"SpanContext",
"SpanKind",
Expand Down
52 changes: 52 additions & 0 deletions opentelemetry-api/src/opentelemetry/trace/ids_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Copyright The 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.

import abc
import random


class IdsGenerator(abc.ABC):
@abc.abstractmethod
def generate_span_id(self) -> int:
"""Get a new span ID.

Returns:
A 64-bit int for use as a span ID
"""

@abc.abstractmethod
def generate_trace_id(self) -> int:
"""Get a new trace ID.

Implementations should at least make the 64 least significant bits
uniformly random. Samplers like the `TraceIdRatioBased` sampler rely on
this randomness to make sampling decisions.

See `the specification on TraceIdRatioBased <https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#traceidratiobased>`_.

Returns:
A 128-bit int for use as a trace ID
"""


class RandomIdsGenerator(IdsGenerator):
"""The default IDs generator for TracerProvider which randomly generates all
bits when generating IDs.
"""

def generate_span_id(self) -> int:
return random.getrandbits(64)

def generate_trace_id(self) -> int:
return random.getrandbits(128)
2 changes: 2 additions & 0 deletions opentelemetry-sdk/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
([#1128](https://github.com/open-telemetry/opentelemetry-python/pull/1128))
- Add support for `OTEL_BSP_MAX_QUEUE_SIZE`, `OTEL_BSP_SCHEDULE_DELAY_MILLIS`, `OTEL_BSP_MAX_EXPORT_BATCH_SIZE` and `OTEL_BSP_EXPORT_TIMEOUT_MILLIS` environment variables
([#1105](https://github.com/open-telemetry/opentelemetry-python/pull/1120))
- Allow for Custom Trace and Span IDs Generation - `IdsGenerator` for TracerProvider
([#1153](https://github.com/open-telemetry/opentelemetry-python/pull/1153))

## Version 0.13b0

Expand Down
27 changes: 7 additions & 20 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,24 +663,6 @@ def record_exception(self, exception: Exception) -> None:
)


def generate_span_id() -> int:
"""Get a new random span ID.

Returns:
A random 64-bit int for use as a span ID
"""
return random.getrandbits(64)


def generate_trace_id() -> int:
"""Get a new random trace ID.

Returns:
A random 128-bit int for use as a trace ID
"""
return random.getrandbits(128)


class Tracer(trace_api.Tracer):
"""See `opentelemetry.trace.Tracer`.

Expand Down Expand Up @@ -733,7 +715,7 @@ def start_span( # pylint: disable=too-many-locals

if parent_context is None or not parent_context.is_valid:
parent = parent_context = None
trace_id = generate_trace_id()
trace_id = self.source.ids_generator.generate_trace_id()
Copy link
Contributor

@owais owais Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Since this isn't part of the tracer provider API as far as I can tell from this PR, we can't guarantee that source will have an ids_generator attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, but this is just being consistent with the assumption later in this file for the sampler, resource, and _active_span_processor variables.

Even so, I think we can guarantee the source will have an ids_generator attribute because source is typed to be of the same type as TracerProvider which will always have an ids_generator because of the change in this PR.

I think that the only danger that could come about would be if you were to implement your own Tracer class that expects an ids_generator, in which case you would also be in danger of assuming there were a sampler or resource as well because they're also not in the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even so, I think we can guarantee the source will have an ids_generator attribute because source is typed to be of the same type as TracerProvider which will always have an ids_generator because of the change in this PR.

I don't think this guarantees anything really. It's only a type hint and makes mypy or other checkers complain if they are run. There is no runtime or build/package time guarantees.

I think that the only danger that could come about would be if you were to implement your own Tracer class that expects an ids_generator, in which case you would also be in danger of assuming there were a sampler or resource as well because they're also not in the API.

Another instance would be if someone implements a custom TracerProvider to add some functionality but wanted to return instance of the stock Tracer. I think both cases should be supported without surprises. Looks like SDK Tracer has taken an implicit dependency on the SDK TracerProvider which shouldn't be the case. Users and vendors should be able to implement just one or the other without having to satisfy undocumented APIs.

I think we should either update the TracerProvider API to specify all these properties or go all in on DI and modify Tracer so that it requires all these dependencies to be explicitly specified by a tracer provider implementation. Meaning IDGenerator would be an initialization argument to the Tracer class. TracerProvider can pass it down explicitly. That way people can implement custom Tracer or custom TracerProvider without surprises.

Since there are other instances of such implicit dependencies, may be we can do this in another PR but if all maintainers agree some solution now, perhaps we can start with IDGenerator now and follow up with the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like SDK Tracer has taken an implicit dependency on the SDK TracerProvider which shouldn't be the case.

Yeah we should probably remove source from both Meter and Tracer SDK implementations and then pass in some sort of "config" object upon creation. Tracer and TracerProvider shouldn't be coupled. But yeah, might be for another PR. @owais can you create an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you're saying, yes it makes more sense to have Tracer and TraceProvider not be requirements of each other.

I would also prefer to have that as a separate PR, and minimize the new things this PR is trying to do. The PR is pretty straightforward the way it is right now, and that kind of refactor could be lost in the IDs Generator specific log messages.

I wouldn't mind helping with doing that separate PR either if we can decide on what we want to do!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. Tracking here: #1181

trace_flags = None
trace_state = None
else:
Expand All @@ -757,7 +739,7 @@ def start_span( # pylint: disable=too-many-locals
)
context = trace_api.SpanContext(
trace_id,
generate_span_id(),
self.source.ids_generator.generate_span_id(),
is_remote=False,
trace_flags=trace_flags,
trace_state=trace_state,
Expand Down Expand Up @@ -826,10 +808,15 @@ def __init__(
active_span_processor: Union[
SynchronousMultiSpanProcessor, ConcurrentMultiSpanProcessor
] = None,
ids_generator: trace_api.IdsGenerator = None,
):
self._active_span_processor = (
active_span_processor or SynchronousMultiSpanProcessor()
)
if ids_generator is None:
self.ids_generator = trace_api.RandomIdsGenerator()
else:
self.ids_generator = ids_generator
self.resource = resource
self.sampler = sampler
self._atexit_handler = None
Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import opentelemetry.trace as trace
from opentelemetry.context import Context
from opentelemetry.sdk.trace import generate_span_id, generate_trace_id
from opentelemetry.trace.propagation.textmap import (
Getter,
Setter,
Expand Down Expand Up @@ -103,8 +102,9 @@ def extract(
self._trace_id_regex.fullmatch(trace_id) is None
or self._span_id_regex.fullmatch(span_id) is None
):
trace_id = generate_trace_id()
span_id = generate_span_id()
ids_generator = trace.get_tracer_provider().ids_generator
trace_id = ids_generator.generate_trace_id()
span_id = ids_generator.generate_span_id()
sampled = "0"

else:
Expand Down
25 changes: 17 additions & 8 deletions opentelemetry-sdk/tests/trace/propagation/test_b3_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def get_child_parent_new_carrier(old_carrier):
"child",
trace_api.SpanContext(
parent_context.trace_id,
trace.generate_span_id(),
trace_api.RandomIdsGenerator().generate_span_id(),
is_remote=False,
trace_flags=parent_context.trace_flags,
trace_state=parent_context.trace_state,
Expand All @@ -56,14 +56,15 @@ def get_child_parent_new_carrier(old_carrier):
class TestB3Format(unittest.TestCase):
@classmethod
def setUpClass(cls):
ids_generator = trace_api.RandomIdsGenerator()
cls.serialized_trace_id = b3_format.format_trace_id(
trace.generate_trace_id()
ids_generator.generate_trace_id()
)
cls.serialized_span_id = b3_format.format_span_id(
trace.generate_span_id()
ids_generator.generate_span_id()
)
cls.serialized_parent_id = b3_format.format_span_id(
trace.generate_span_id()
ids_generator.generate_span_id()
)

def test_extract_multi_header(self):
Expand Down Expand Up @@ -246,8 +247,12 @@ def test_missing_trace_id(self):
span_context = trace_api.get_current_span(ctx).get_context()
self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID)

@patch("opentelemetry.sdk.trace.propagation.b3_format.generate_trace_id")
@patch("opentelemetry.sdk.trace.propagation.b3_format.generate_span_id")
@patch(
"opentelemetry.sdk.trace.propagation.b3_format.trace.RandomIdsGenerator.generate_trace_id"
)
@patch(
"opentelemetry.sdk.trace.propagation.b3_format.trace.RandomIdsGenerator.generate_span_id"
)
def test_invalid_trace_id(
self, mock_generate_span_id, mock_generate_trace_id
):
Expand All @@ -268,8 +273,12 @@ def test_invalid_trace_id(
self.assertEqual(span_context.trace_id, 1)
self.assertEqual(span_context.span_id, 2)

@patch("opentelemetry.sdk.trace.propagation.b3_format.generate_trace_id")
@patch("opentelemetry.sdk.trace.propagation.b3_format.generate_span_id")
@patch(
"opentelemetry.sdk.trace.propagation.b3_format.trace.RandomIdsGenerator.generate_trace_id"
)
@patch(
"opentelemetry.sdk.trace.propagation.b3_format.trace.RandomIdsGenerator.generate_span_id"
)
def test_invalid_span_id(
self, mock_generate_span_id, mock_generate_trace_id
):
Expand Down
9 changes: 5 additions & 4 deletions opentelemetry-sdk/tests/trace/test_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,14 +615,15 @@ def test_invalid_event_attributes(self):
self.assertEqual(root.events[3].attributes, {"attr2": (1, 2)})

def test_links(self):
ids_generator = trace_api.RandomIdsGenerator()
other_context1 = trace_api.SpanContext(
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
trace_id=ids_generator.generate_trace_id(),
span_id=ids_generator.generate_span_id(),
is_remote=False,
)
other_context2 = trace_api.SpanContext(
trace_id=trace.generate_trace_id(),
span_id=trace.generate_span_id(),
trace_id=ids_generator.generate_trace_id(),
span_id=ids_generator.generate_span_id(),
is_remote=False,
)

Expand Down