Add named tracers to the API #584

jkwatson · 2019-10-01T23:12:52Z

This implements #573

/cc @bogdandrutu

arminru

Thanks for taking it on right away! 🙂

api/src/main/java/io/opentelemetry/trace/spi/TracerFactory.java

jkwatson · 2019-10-02T17:19:46Z

Updated with feedback from @arminru

codecov-io · 2019-10-02T17:25:35Z

Codecov Report

Merging #584 into master will increase coverage by 0.16%.
The diff coverage is 89.13%.

@@             Coverage Diff              @@
##             master     #584      +/-   ##
============================================
+ Coverage      78.1%   78.27%   +0.16%     
- Complexity      691      702      +11     
============================================
  Files            86       89       +3     
  Lines          2453     2476      +23     
  Branches        232      233       +1     
============================================
+ Hits           1916     1938      +22     
- Misses          448      449       +1     
  Partials         89       89

Impacted Files	Coverage Δ	Complexity Δ
...va/io/opentelemetry/opentracingshim/TraceShim.java	`25% <0%> (-8.34%)`	`1 <0> (ø)`
...ntelemetry/sdk/trace/TracerSdkFactoryProvider.java	`100% <100%> (ø)`	`2 <2> (?)`
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java	`100% <100%> (ø)`	`3 <1> (ø)`	⬇️
...ntelemetry/trace/DefaultTracerFactoryProvider.java	`100% <100%> (ø)`	`4 <4> (?)`
...a/io/opentelemetry/trace/DefaultTracerFactory.java	`80% <80%> (ø)`	`4 <4> (?)`
...a/io/opentelemetry/sdk/trace/TracerSdkFactory.java	`90% <90%> (ø)`	`3 <3> (?)`
.../src/main/java/io/opentelemetry/OpenTelemetry.java	`97.22% <95.45%> (+0.25%)`	`15 <7> (ø)`	⬇️
... and 1 more

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 a02b6d3...d8f4283. Read the comment docs.

bogdandrutu

In my mind I thought that we will still have a TracerFactoryProvider (for the spi part) then the OpenTelemetry will return a TracerFactory (constructed using di) then from there you get Tracers.

I feel that the current PR changes the TracerProvider -> TracerFactory which is not what I expect.

jkwatson · 2019-10-10T02:50:21Z

Yeah, I went back and forth on which was the right way to go. I guess the decision is: do we want to control the caching of tracers at the API layer, or do we want to have that be an SDK responsibility. My gut tells me that it belongs in the SDK, but I don't feel extremely strongly about that. Thoughts?

bogdandrutu · 2019-10-10T02:57:23Z

So everything is an interface TracerFactory/Tracer so the TracerFactoryProvider will provide it's own implementation for the factory so the SDK will control the caching or anything they want.

jkwatson · 2019-10-10T03:04:39Z

Ah, I see what you're saying. Does that extra layer of indirection buy us much? SDK implementors now have to both implement a factory, and a factory factory (the provider implementation). It starts to sound like a parody of Java code. :)

bogdandrutu · 2019-10-10T03:16:14Z

Let's do that way for the moment and reevaluate in the next release when the Global instance otep will be addressed

jkwatson · 2019-10-10T04:04:40Z

sure thing. I'll update it. Maybe I'll add a FactoryFactoryProviderImpl along side it. (just kidding!!!)

jkwatson · 2019-10-11T14:51:41Z

updated with a TracerFactoryProvider

bogdandrutu

Overall LG

api/src/main/java/io/opentelemetry/OpenTelemetry.java

api/src/main/java/io/opentelemetry/trace/DefaultTracerFactory.java

api/src/main/java/io/opentelemetry/OpenTelemetry.java

jkwatson · 2019-10-14T21:09:57Z

If this is close enough to move forward with, I'll update the description and we can get it approved. Thoughts?

arminru · 2019-10-15T11:35:53Z

If this is close enough to move forward with, I'll update the description and we can get it approved. Thoughts?

Sounds good!

api/src/main/java/io/opentelemetry/trace/spi/TracerFactoryProvider.java

arminru · 2019-10-15T16:15:47Z

sdk/src/main/java/io/opentelemetry/sdk/trace/TracerSdkFactory.java

+    String key = instrumentationName + "/" + instrumentationVersion;
+    Tracer tracer = tracersByKey.get(key);
+    if (tracer == null) {
+      // todo: pass in the name & version here to the implementation to be used for purposes.


Please create an issue for tracking this TODO.

I wasn't able to find a specification on what the SDK should do with these values. There is mention of creation of a Resource that represents this data in the SDK, but nothing concrete. Did I miss it somewhere?

Regarding named tracers, the SDK spec says:

The name and version arguments supplied to the TracerFactory must be used
to create a Resource instance which is stored on the created Tracer.
The readable representations of all Span instances created by a Tracer must
provide a getLibraryResource method that returns this Resource information
held by the Tracer.

In order to allow SpanProcessors to access the tracer's name and version when processing spans, ReadableSpan will have to be extended by adding a method Resource getLibraryResource(), which returns a Resource describing the named tracer.

aha! great. I saw mention of this in the OTEP, but hadn't tracked it down in the actual spec. Thanks! Issue added: #617

api/src/main/java/io/opentelemetry/trace/TracerFactory.java

…ider.java Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

api/src/main/java/io/opentelemetry/trace/DefaultTracerFactory.java

jkwatson requested review from arminru, bogdandrutu, carlosalberto, pavolloffay, songy23 and tylerbenson as code owners October 1, 2019 23:12

arminru reviewed Oct 2, 2019

View reviewed changes

api/src/main/java/io/opentelemetry/trace/spi/TracerFactory.java Outdated Show resolved Hide resolved

bogdandrutu reviewed Oct 10, 2019

View reviewed changes

jkwatson force-pushed the named_tracer_ideas branch from 34a3aec to 5d7feaf Compare October 11, 2019 14:48

bogdandrutu reviewed Oct 11, 2019

View reviewed changes

api/src/main/java/io/opentelemetry/OpenTelemetry.java Outdated Show resolved Hide resolved

api/src/main/java/io/opentelemetry/trace/DefaultTracerFactory.java Show resolved Hide resolved

arminru reviewed Oct 14, 2019

View reviewed changes

api/src/main/java/io/opentelemetry/OpenTelemetry.java Outdated Show resolved Hide resolved

jwatson added 4 commits October 14, 2019 13:40

A rough idea of how named tracers might look in the API.

01ddd76

update to require at least a name for the tracer

ae0a625

Introduce another layer of factory for the provider

5eb4656

remove convenience tracer methods from the OpenTelemetry class.

b959240

jkwatson force-pushed the named_tracer_ideas branch from 5d7feaf to b959240 Compare October 14, 2019 20:55

jkwatson changed the title ~~A rough idea of how named tracers might look in the API.~~ Add named tracers to the API Oct 15, 2019

arminru reviewed Oct 15, 2019

View reviewed changes

jkwatson and others added 2 commits October 15, 2019 09:48

Update api/src/main/java/io/opentelemetry/trace/spi/TracerFactoryProv…

3fdb634

…ider.java Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

Update api/src/main/java/io/opentelemetry/trace/TracerFactory.java

65d9869

Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>

Add clarifying javadoc, per PR review

d8f4283

bogdandrutu approved these changes Oct 18, 2019

View reviewed changes

api/src/main/java/io/opentelemetry/trace/DefaultTracerFactory.java Outdated Show resolved Hide resolved

reverse who calls whom

11f30ed

bogdandrutu approved these changes Oct 22, 2019

View reviewed changes

bogdandrutu merged commit 60ba3b7 into open-telemetry:master Oct 22, 2019

jkwatson deleted the named_tracer_ideas branch December 16, 2019 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add named tracers to the API #584

Add named tracers to the API #584

jkwatson commented Oct 1, 2019 •

edited

Loading

arminru left a comment

jkwatson commented Oct 2, 2019

codecov-io commented Oct 2, 2019 •

edited

Loading

bogdandrutu left a comment

jkwatson commented Oct 10, 2019

bogdandrutu commented Oct 10, 2019

jkwatson commented Oct 10, 2019

bogdandrutu commented Oct 10, 2019

jkwatson commented Oct 10, 2019

jkwatson commented Oct 11, 2019

bogdandrutu left a comment

jkwatson commented Oct 14, 2019

arminru commented Oct 15, 2019

arminru Oct 15, 2019

jkwatson Oct 15, 2019

arminru Oct 16, 2019

jkwatson Oct 16, 2019

Add named tracers to the API #584

Add named tracers to the API #584

Conversation

jkwatson commented Oct 1, 2019 • edited Loading

arminru left a comment

Choose a reason for hiding this comment

jkwatson commented Oct 2, 2019

codecov-io commented Oct 2, 2019 • edited Loading

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

jkwatson commented Oct 10, 2019

bogdandrutu commented Oct 10, 2019

jkwatson commented Oct 10, 2019

bogdandrutu commented Oct 10, 2019

jkwatson commented Oct 10, 2019

jkwatson commented Oct 11, 2019

bogdandrutu left a comment

Choose a reason for hiding this comment

jkwatson commented Oct 14, 2019

arminru commented Oct 15, 2019

arminru Oct 15, 2019

Choose a reason for hiding this comment

jkwatson Oct 15, 2019

Choose a reason for hiding this comment

arminru Oct 16, 2019

Choose a reason for hiding this comment

jkwatson Oct 16, 2019

Choose a reason for hiding this comment

jkwatson commented Oct 1, 2019 •

edited

Loading

codecov-io commented Oct 2, 2019 •

edited

Loading