Named tracers #301

Oberon00 · 2019-11-22T13:22:44Z

Implements the "Tracers" part of #203.

EDIT: I chose the name TracerSource instead of TracerFactory because it seems to be the consensus that TracerFactory is not an ideal name since the spec allows the same instance to be returned by different calls to getTracer, which means that TracerFactory is not really a Factory. See also open-telemetry/opentelemetry-specification#354 (comment)

codecov-io · 2019-11-22T15:21:26Z

Codecov Report

Merging #301 into master will increase coverage by 0.12%.
The diff coverage is 93.42%.

@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
+ Coverage    84.6%   84.73%   +0.12%     
==========================================
  Files          33       35       +2     
  Lines        1676     1716      +40     
  Branches      199      200       +1     
==========================================
+ Hits         1418     1454      +36     
- Misses        201      204       +3     
- Partials       57       58       +1

Impacted Files	Coverage Δ
opentelemetry-api/src/opentelemetry/util/loader.py	`81.25% <ø> (ø)`	⬆️
...ts/src/opentelemetry/ext/http_requests/__init__.py	`89.74% <100%> (+0.55%)`	⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`68.18% <100%> (ø)`	⬆️
...app/src/opentelemetry_example_app/flask_example.py	`100% <100%> (ø)`	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.9% <100%> (+0.03%)`	⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`89.87% <90.9%> (-0.52%)`	⬇️
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`84.12% <92.85%> (+0.38%)`	⬆️
.../src/opentelemetry/ext/opentracing_shim/version.py	`100% <0%> (ø)`
...sts/src/opentelemetry/ext/http_requests/version.py	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d74c39c...b48e708. Read the comment docs.

examples/opentelemetry-example-app/src/opentelemetry_example_app/flask_example.py ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py

Conflicts: ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py ext/opentelemetry-ext-testutil/src/opentelemetry/ext/testutil/wsgitestutil.py ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py opentelemetry-sdk/tests/trace/test_trace.py

codeboten

Only blocking on the suggested changes. One thought looking through the basic_tracer example is that it would be nice to still be able to get a usable Tracer without having to name a TracerSource. That can be addressed later though.

opentelemetry-api/src/opentelemetry/trace/__init__.py

codeboten · 2019-11-26T22:14:27Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -133,6 +133,7 @@ def __init__(
        links: Sequence[trace_api.Link] = (),
        kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
        span_processor: SpanProcessor = SpanProcessor(),
+        creator_info: "InstrumentationInfo" = None,


Any reason not to call this argument instrumentation_info?

Because it is the instrumentation_info that identifies the creator of this span, so I thought creator_info is a more specific name.

I agree that instrumentation_info is the more obvious name here since it's called that on the tracer.

I renamed it in 725bb16.

codeboten · 2019-11-26T22:41:56Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+    ) -> "trace_api.Tracer":
+        if not instrumenting_library_name:  # Reject empty strings too.
+            instrumenting_library_name = "ERROR:MISSING LIB NAME"
+            logger.error("get_tracer called with missing library name.")


Looking at the spec, it looks like the name should be optional? https://github.com/open-telemetry/opentelemetry-specification/pull/354/files#diff-ea4f4a46fe8755cf03f9fb3a6434cb0cR98

No, it's actually required right now: https://github.com/open-telemetry/opentelemetry-specification/blob/a6a22409a2d242a6af097d72fd1241dd64e97219/specification/api-tracing.md#obtaining-a-tracer

Ah. Thank you

I think we ought to push back on this, the most common case is likely to be the default tracer, which doesn't need a name.

Why do you think that? I think the most common case will be tracers created by instrumentations, which should definitely tell their name to OpenTelemetry. Even when you have no dedicated instrumentation library, you can use the name and version of your application here.

(sharing an extra opinion) I don't think it's a big deal. two extra arguments of boilerplate isn't the best, but it's not a usability blocker.

I personally imagine tracing will be transparent to most people as the integrations will take care of injecting traces into the right locations. If there are enough extensions, the need for custom traces will probably be fairly minimal.

Co-Authored-By: alrex <alrex.boten@gmail.com>

Conflicts: opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

c24t

This looks like a faithful implementation of the spec, and follows open-telemetry/opentelemetry-java#584. In that sense it LGTM.

But these changes are also make the API significantly more complicated, and make our already user-unfriendly boilerplate even more unfriendly.

Compare our API:

trace.set_preferred_tracer_source_implementation(lambda T: TracerSource())
tracer = trace.tracer_source().get_tracer(name, version)

to logging:

logger = logging.getLogger(name)

There are also some significant unanswered questions about the relationship between tracers and context that need to be addressed in the spec.

c24t · 2019-12-06T19:11:01Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

@@ -60,7 +61,9 @@ def _before_flask_request():
        otel_wsgi.get_header_from_environ, environ
    )

-    tracer = trace.tracer()
+    tracer = trace.tracer_source().get_tracer(
+        "opentelemetry-ext-flask", __version__


Out of curiosity: why use the package name instead of the dotted namespace? E.g. opentelemetry.ext.flask? The later is closer to the __file__ convention for logger names, and you can have multiple unique names per package.

That would be another possibility. There should probably a unique mapping full module name -> package anyway.

+1 on the suggestion to use the name. Most loggers are configured that way as well, and having parity of the logging namespace and tracer namespace could be very useful.

I changed it to the module name in e4db217. But note that once we have a registry pattern, we will be creating a tracer per module instead of per library with the most naive implementation at least.

Yeah, one risk of copying logging here is that users might reasonably assume these names are meant to be hierarchical too.

opentelemetry-api/src/opentelemetry/trace/__init__.py

c24t · 2019-12-06T20:47:57Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        # TODO: How should multiple TracerSources behave? Should they get their own contexts?
+        # This could be done by adding `str(id(self))` to the slot name.


Making TracerSource the shared container the tracer config (sampler, processor, etc.) and the context seems like it would create some problems.

What would you do if you want a particular tracer to share context with the default tracer, but configure it to use a different span processor? Silencing noisy tracers from instrumented libraries is one of the main motivations for https://github.com/open-telemetry/oteps/blob/master/text/0016-named-tracers.md, and the span processor is only mechanism the API provides to suppress span creation.

FWIW, it looks like the java client doesn't allow users to configure the context at all -- all TracerSdks use the global context. The only way to use a different context is to bring your own tracer impl.

I think there's a good argument for making the context configurable on a per-tracer or -factory basis, and the spec doesn't adequately explain the relationship between tracers and the context now.

Silencing noisy tracers from instrumented libraries is one of the main motivations for https://github.com/open-telemetry/oteps/blob/master/text/0016-named-tracers.md, and the span processor is only mechanism the API provides to suppress span creation.

I think I should actually add the instrumentation info to to the arguments for the sampler. What do you think? EDIT: On second thought, this is a breaking change for the sampler API and we might want to gather all the sampler arguments into some SpanArguments object/namedtuple to not break the API again in the future with such a change. I think that could be done in a separate PR to not drag this one out longer.

Making TracerSource the shared container the tracer config (sampler, processor, etc.) and the context seems like it would create some problems.

But that's what we decided on in the spec, after some discussion: open-telemetry/opentelemetry-specification#304 (and open-telemetry/opentelemetry-specification#339) tl;dr: While this may be a problem worth solving, "named tracers" was never meant as a solution for that problem.

I would also argue that sharing the same trace configuration is generally the preferred thing to do.

Potentially there's value in adding additional processors per tracer, thus chaining the processing. But the vendor tracing integrations I've seen sample and process globally anyway.

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

c24t · 2019-12-06T20:50:34Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        if not instrumenting_library_name:  # Reject empty strings too.
+            instrumenting_library_name = "ERROR:MISSING LIB NAME"
+            logger.error("get_tracer called with missing library name.")
+        return Tracer(


Is there a reason not to cache the tracers? This class seems like a natural registry.

What would be the benefit of caching them? Creating the named tracers instance should be very cheap.

I could imagine it's more objects the GC has to collect aggressively. I think we should drive these types of requirements through benchmark tests in the repo. Ultimately the concern is performance so a way to quantify the ramifications of design choices would be great.

Tracking ticket for that suggestion: #335

c24t · 2019-12-06T20:52:51Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -133,6 +133,7 @@ def __init__(
        links: Sequence[trace_api.Link] = (),
        kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL,
        span_processor: SpanProcessor = SpanProcessor(),
+        creator_info: "InstrumentationInfo" = None,


I agree that instrumentation_info is the more obvious name here since it's called that on the tracer.

c24t · 2019-12-06T21:14:29Z

I also see some stray set_preferred_tracer_implementations.

Oberon00 · 2019-12-09T09:35:29Z

It looks like I can't push to this branch? See dynatrace-oss-contrib#2.

Thank you, I merged it. Maybe the permissions on the repository I'm using (it's in the dynatrace org) are overriding the "allow edits from maintainers" setting.

In-reply-to: https://github.com/open-telemetry/opentelemetry-python/pull/301/files#r351007226

c24t

Thanks for addressing my comments @Oberon00. I think there are still some issues to resolve in the spec, but I don't want to hold up this PR.

This should be ready to merge except for the merge conflict (which I can't resolve on this branch).

toumorokoshi

Functionally everything looks great!

My one concern is the interface. "TracerSource" seems less intuitive to me than TraceFactory. Although I understand that there is a distinction there, "Source" is not a design pattern I'm aware of. For me, "Factory" gives me a more immediate understanding of the purpose of that class.

toumorokoshi · 2019-12-12T17:47:39Z

ext/opentelemetry-ext-flask/src/opentelemetry/ext/flask/__init__.py

@@ -60,7 +61,9 @@ def _before_flask_request():
        otel_wsgi.get_header_from_environ, environ
    )

-    tracer = trace.tracer()
+    tracer = trace.tracer_source().get_tracer(
+        "opentelemetry-ext-flask", __version__


+1 on the suggestion to use the name. Most loggers are configured that way as well, and having parity of the logging namespace and tracer namespace could be very useful.

toumorokoshi · 2019-12-12T17:49:23Z

README.md

@@ -52,15 +52,15 @@ pip install -e ./ext/opentelemetry-ext-{integration}
 ```python
 from opentelemetry import trace
 from opentelemetry.context import Context
-from opentelemetry.sdk.trace import Tracer
+from opentelemetry.sdk.trace import TracerSource


another idea on naming: "tracing". Since it's conceptually similar to the logging module, we could use that interface convention:

tracing.get_tracer()

Although I'm also a fan of adding these into the top level trace interface, as that's simpler:

trace.get_tracer(__name__)

toumorokoshi · 2019-12-12T17:54:43Z

ext/opentelemetry-ext-jaeger/examples/jaeger_exporter_example.py

-trace.set_preferred_tracer_implementation(lambda T: Tracer())
-tracer = trace.tracer()
+trace.set_preferred_tracer_source_implementation(lambda T: TracerSource())
+tracer = trace.tracer_source().get_tracer("myapp")


I worry if we put all the examples as get_tracer("myapp"), that might cause a lot of people to not understand the convention to use the package / module name in the tracer, and make integrations that are not part of this repo harder to turn on / off.

How about following Flask's example of passing in name? Another argument to just use module names, in my opinion.

toumorokoshi · 2019-12-12T18:03:48Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        # TODO: How should multiple TracerSources behave? Should they get their own contexts?
+        # This could be done by adding `str(id(self))` to the slot name.


I would also argue that sharing the same trace configuration is generally the preferred thing to do.

Potentially there's value in adding additional processors per tracer, thus chaining the processing. But the vendor tracing integrations I've seen sample and process globally anyway.

toumorokoshi · 2019-12-12T18:05:54Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+    ) -> "trace_api.Tracer":
+        if not instrumenting_library_name:  # Reject empty strings too.
+            instrumenting_library_name = "ERROR:MISSING LIB NAME"
+            logger.error("get_tracer called with missing library name.")


(sharing an extra opinion) I don't think it's a big deal. two extra arguments of boilerplate isn't the best, but it's not a usability blocker.

I personally imagine tracing will be transparent to most people as the integrations will take care of injecting traces into the right locations. If there are enough extensions, the need for custom traces will probably be fairly minimal.

toumorokoshi · 2019-12-12T18:11:27Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

+        if not instrumenting_library_name:  # Reject empty strings too.
+            instrumenting_library_name = "ERROR:MISSING LIB NAME"
+            logger.error("get_tracer called with missing library name.")
+        return Tracer(


I could imagine it's more objects the GC has to collect aggressively. I think we should drive these types of requirements through benchmark tests in the repo. Ultimately the concern is performance so a way to quantify the ramifications of design choices would be great.

Tracking ticket for that suggestion: #335

toumorokoshi · 2019-12-12T18:13:09Z

opentelemetry-sdk/tests/trace/export/test_in_memory_span_exporter.py

        self.assertEqual(len(span_list), 0)

    def test_shutdown(self):
-        tracer = trace.Tracer()
-
-        memory_exporter = InMemorySpanExporter()


nice! good catch on some DRY-able code :)

toumorokoshi · 2019-12-12T19:36:58Z

@Oberon00 it looks like the build is segfaulting on the tracecontext step, due to a segfault. IIRC there was a fix with pinning the multidict version, which I'm having trouble finding. Is that the case here?

c24t · 2019-12-12T19:49:35Z

The build fix is on master: #330, we just need to pick up master here.

Oberon00 · 2019-12-13T14:14:15Z

Just calling out that I changed this PR to use the module name instead of the library name, see #301 (comment).

c24t

Looks great, thanks @Oberon00!

🎉

…etry#301)

Oberon00 added 2 commits November 22, 2019 13:31

Named tracer.

d5c1da1

Adapt all the things to named tracers.

94c72f8

Oberon00 requested a review from c24t November 22, 2019 13:22

Oberon00 added 5 commits November 22, 2019 15:06

Fix examples, move add_span_processor to source.

f1a23c1

Fix examples for add_span_processor move.

d91e5dd

Fix examples for good.

2f95b61

Add unit tests.

8014e52

Fix W3C tests, lint, docs.

fbf4a1e

Oberon00 marked this pull request as ready for review November 22, 2019 15:21

Oberon00 requested review from a-feld, carlosalberto, lzchen, reyang and toumorokoshi as code owners November 22, 2019 15:21

Oberon00 added 5 commits November 22, 2019 16:32

Increase test coverage.

716ead7

Merge branch 'master' into named-tracers

ac5dc8b

examples/opentelemetry-example-app/src/opentelemetry_example_app/flask_example.py ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py ext/opentelemetry-ext-wsgi/tests/test_wsgi_middleware.py

Adapt flask extension for named tracers.

7e9dad7

Fix mismerge of da8b8d9.

009418a

codeboten suggested changes Nov 26, 2019

View reviewed changes

Oberon00 and others added 4 commits December 5, 2019 16:53

Fix tracer -> tracer source in comments/doc.

9163c9b

Co-Authored-By: alrex <alrex.boten@gmail.com>

Update opentelemetry-api/src/opentelemetry/trace/__init__.py

ab805d2

Co-Authored-By: alrex <alrex.boten@gmail.com>

Merge branch 'master' into named-tracers

c987246

Conflicts: opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

Adjust new tests to named tracers.

2aa258a

codeboten approved these changes Dec 5, 2019

View reviewed changes

This was referenced Dec 5, 2019

Update WSGI & Flask integrations to follow new semantic conventions #299

Merged

Rename Tracer/MeterFactory to Tracer/MeterRegistry open-telemetry/opentelemetry-java#664

Closed

c24t reviewed Dec 6, 2019

View reviewed changes

toumorokoshi added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Dec 9, 2019

c24t added 2 commits December 9, 2019 10:31

Rewrap comments

f05eaaf

Fix stray examples

a6a2670

Oberon00 added 4 commits December 9, 2019 11:17

Merge remote-tracking branch 'upstream/master' into named-tracers

e28713e

Adapt new tests.

0cbdccb

Fix docstrings.

9236606

Rename creator_info to instrumentation_info.

725bb16

In-reply-to: https://github.com/open-telemetry/opentelemetry-python/pull/301/files#r351007226

a-feld removed their request for review December 10, 2019 00:54

c24t approved these changes Dec 10, 2019

View reviewed changes

Merge branch 'master' into named-tracers-merge

4de385e

This was referenced Dec 10, 2019

Merge master into #301 dynatrace-oss-contrib/opentelemetry-python#3

Merged

Named tracers (rebase) #326

Closed

Oberon00 requested a review from a team December 11, 2019 08:29

toumorokoshi approved these changes Dec 12, 2019

View reviewed changes

Oberon00 added 3 commits December 12, 2019 21:39

Merge branch 'master' into named-tracers

7907133

Fix tests.

b7c688c

libname -> module name

e4db217

Oberon00 added 2 commits December 13, 2019 15:22

Lint.

239a18d

Fix docs build.

b48e708

c24t approved these changes Dec 14, 2019

View reviewed changes

c24t merged commit baab508 into open-telemetry:master Dec 14, 2019

Oberon00 removed the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Dec 17, 2019

lzchen mentioned this pull request Jan 7, 2020

Adding psycopg2 integration #298

Merged

c24t mentioned this pull request Jan 13, 2020

Updating examples to use tracer_source #360

Merged

dgzlopes mentioned this pull request Jan 29, 2020

Add stackdriver trace exporter #288

Closed

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

feat(exporter-jaeger): add forceFlush and timeout options (open-telem…

288bf68

…etry#301)

		# TODO: How should multiple TracerSources behave? Should they get their own contexts?
		# This could be done by adding `str(id(self))` to the slot name.

Named tracers #301

Named tracers #301

Conversation

Oberon00 commented Nov 22, 2019 • edited Loading

codecov-io commented Nov 22, 2019 • edited Loading

Codecov Report

codeboten left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Oberon00 Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t commented Dec 6, 2019

Oberon00 commented Dec 9, 2019

c24t left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi commented Dec 12, 2019

c24t commented Dec 12, 2019

Oberon00 commented Dec 13, 2019

c24t left a comment

Choose a reason for hiding this comment

Oberon00 commented Nov 22, 2019 •

edited

Loading

codecov-io commented Nov 22, 2019 •

edited

Loading

codeboten left a comment •

edited

Loading

Oberon00 Dec 9, 2019 •

edited

Loading

Oberon00 Dec 9, 2019 •

edited

Loading

Oberon00 Dec 9, 2019 •

edited

Loading