This is a POC implementation of OTEP 74, global instance management #724

jkwatson · 2019-12-30T23:56:16Z

This PR only handles the replacement of Tracer instances, but it gives a good indication of what it would look like for Meters as well.

codecov-io · 2019-12-30T23:59:40Z

Codecov Report

Merging #724 into master will decrease coverage by 0.01%.
The diff coverage is 88.5%.

@@             Coverage Diff              @@
##             master     #724      +/-   ##
============================================
- Coverage     78.58%   78.57%   -0.02%     
- Complexity      778      796      +18     
============================================
  Files           101      102       +1     
  Lines          2779     2824      +45     
  Branches        268      269       +1     
============================================
+ Hits           2184     2219      +35     
- Misses          493      500       +7     
- Partials        102      105       +3

Impacted Files	Coverage Δ	Complexity Δ
.../io/opentelemetry/trace/DefaultTracerRegistry.java	`100% <100%> (ø)`	`7 <4> (+2)`	⬆️
...ava/io/opentelemetry/SpiOpenTelemetryProvider.java	`100% <100%> (ø)`	`11 <11> (?)`
.../src/main/java/io/opentelemetry/OpenTelemetry.java	`76.47% <73.33%> (-20.97%)`	`13 <13> (-2)`
...ain/java/io/opentelemetry/trace/DefaultTracer.java	`93.22% <90%> (-2.24%)`	`15 <13> (+8)`
...elemetry/opentracingshim/SpanContextShimTable.java	`89.28% <0%> (-7.15%)`	`7% <0%> (-1%)`
...elemetry/sdk/trace/export/BatchSpansProcessor.java	`89.65% <0%> (+0.86%)`	`7% <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 bd4f373...cf65412. Read the comment docs.

bogdandrutu · 2020-01-07T01:19:14Z

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

+   * @param tracerFactory A fully configured and ready to operate {@link TracerFactory}
+   *     implementation.
+   */
+  public static void setTracerFactory(TracerFactory tracerFactory) {


If you want to offer a set we need to have thin wrappers that allow changing the implementation.

TraceFactoryWrapper extends TraceFactory {
private volatile TraceFactory implementation;

// redirects all calls to the implementation.
}

This solves the problem when users hold a static reference to the factory, but does not solve it when the user holds a reference to a Tracer.

I think in Java we already solved the otep 74 by providing dynamic loading for the TraceFactory implementation and we don't need a set method.

I don't believe that to be the case. If instrumentation gets a handle to a Tracer before the SDK has been initialized, then it will never be replaced with an implementation that has a functional Tracer implementation behind it. This sample POC provides for the API Tracer to have it's implementation swapped out when the SDK is initialized.

jkwatson · 2020-01-07T03:06:33Z

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

+    for (Entry<TracerKey, DefaultTracer> entry : existingTracers.entrySet()) {
+      TracerKey key = entry.getKey();
+      entry.getValue().setImplementation(tracerFactory.get(key.getName(), key.getVersion()));
+    }


@bogdandrutu here is where the tracer implementation is swapped out after the SDK has been initialized.

jmacd · 2020-01-21T23:23:04Z

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

@@ -73,7 +84,10 @@ public static TracerFactory getTracerFactory() {
   * @since 0.1.0
   */
  public static MeterFactory getMeterFactory() {
-    return getInstance().meterFactory;
+    if (instance.meterFactory.get() == null) {
+      setMeterFactory(SpiOpenTelemetryProvider.makeSpiMeterFactory());


I was expecting to see some logic that uses a global "deferred" factory here. I see two cases:
(1) The user is planning to call setXXXerFactory() later; they want a handle they can store now which becomes functional later
(2) The user has planned for dependency injection, in which case we can bypass the user of a deferred factory here and use the injected value.
What I see here looks different. If the value is used it forces a decision about SPI to happen--what if there's no value injected and the user is still planning to call setXXXFactory? I don't see how that will work.
What I read in the code here is that if someone accesses the global

This PR doesn't include having done anything with the Meter implementation, only Tracers.

I see now. Inside the call to makeSpiXXXFactory you get a default "deferred" impl if there's no SPI value.

jmacd · 2020-01-21T23:30:23Z

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

@@ -73,7 +84,10 @@ public static TracerFactory getTracerFactory() {
   * @since 0.1.0
   */
  public static MeterFactory getMeterFactory() {
-    return getInstance().meterFactory;
+    if (instance.meterFactory.get() == null) {
+      setMeterFactory(SpiOpenTelemetryProvider.makeSpiMeterFactory());


I see now. Inside the call to makeSpiXXXFactory you get a default "deferred" impl if there's no SPI value.

…runtime.

…er after an SDK has been installed.

bogdandrutu · 2020-02-19T23:44:48Z

Is this still needed? I think that offering the ServiceLoader option as the only option is good enough, here are the reasons why:

If there is a need for a deferred initialization an implementation can implement exactly your logic and allow implementation to be swapped.
If needed we can change the current TracerSdk to generate no-op Spans until exporter, config, idgenerator are installed (which becomes almost the same code as you have in this PR).

jkwatson · 2020-02-19T23:49:27Z

I'm unclear on the state of the relevant OTEP. Our current implementation on master doesn't implement the OTEP fully. This is only here as an example of what it would take to implement the OTEP fully for the TracerSdk. If we want to close it, I'm fine with that, since it wasn't intended to be a final implementation, in any case. :)

bogdandrutu · 2020-02-19T23:55:31Z

For the moment trying to understand if just SPI is enough. As I tried to explain in the first reason someone can implement a "Swappable" tracer implementation using SPI, by having the Swappable implementation loaded with SPI and installing the real implementation by calling into the Swappable implementation.

bogdandrutu · 2020-02-19T23:56:22Z

Currently the OTEP says that if language supports SPI we should use that and don't do all the other work.

jkwatson · 2020-02-20T00:04:21Z

Yep. I understand it. As I said, I'm fine with closing this. I'm not sure if we're going to work with Spring Framework, and archaic J2EE containers yet, but we can cross that bridge when @kbrockhoff gets to it. ;)

bogdandrutu · 2020-02-20T00:04:48Z

What I want is a hidden thing :)). I want to make you work a bit to prove me wrong/right that someone can or cannot build an implementation of the API (using SPI) that supports delayed initialization for the Tracer class.

jkwatson · 2020-02-20T00:06:14Z

Heh. I'm 100% sure it can be done. I mostly wanted to do this as an exercise to see what it might look like.

jkwatson requested review from arminru, bogdandrutu, carlosalberto, pavolloffay, songy23 and tylerbenson as code owners December 30, 2019 23:56

jkwatson mentioned this pull request Dec 30, 2019

Initialization of global variables open-telemetry/oteps#74

Closed

bogdandrutu reviewed Jan 7, 2020

View reviewed changes

jkwatson commented Jan 7, 2020

View reviewed changes

kbrockhoff mentioned this pull request Jan 13, 2020

Registries created through builders should be returned by OpenTelemetry static methods #747

Closed

jmacd reviewed Jan 21, 2020

View reviewed changes

jmacd approved these changes Jan 21, 2020

View reviewed changes

jwatson added 3 commits January 22, 2020 11:25

allow the various globals in the OpenTelemetry instance to be set at …

ceb1ecf

…runtime.

enable swapping out of the underlying implementation of a DefaultTrac…

23e5cd8

…er after an SDK has been installed.

Update for renames from upstream master

cf65412

jkwatson force-pushed the OTEP_74_POC branch from 31dcc9e to cf65412 Compare January 22, 2020 23:59

jkwatson closed this Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This is a POC implementation of OTEP 74, global instance management #724

This is a POC implementation of OTEP 74, global instance management #724

jkwatson commented Dec 30, 2019

codecov-io commented Dec 30, 2019 •

edited

Loading

bogdandrutu Jan 7, 2020

jkwatson Jan 7, 2020

jkwatson Jan 7, 2020

jmacd Jan 21, 2020

jkwatson Jan 21, 2020

jmacd Jan 21, 2020

jmacd Jan 21, 2020

bogdandrutu commented Feb 19, 2020

jkwatson commented Feb 19, 2020

bogdandrutu commented Feb 19, 2020

bogdandrutu commented Feb 19, 2020

jkwatson commented Feb 20, 2020

bogdandrutu commented Feb 20, 2020

jkwatson commented Feb 20, 2020

This is a POC implementation of OTEP 74, global instance management #724

This is a POC implementation of OTEP 74, global instance management #724

Conversation

jkwatson commented Dec 30, 2019

codecov-io commented Dec 30, 2019 • edited Loading

Codecov Report

bogdandrutu Jan 7, 2020

Choose a reason for hiding this comment

jkwatson Jan 7, 2020

Choose a reason for hiding this comment

jkwatson Jan 7, 2020

Choose a reason for hiding this comment

jmacd Jan 21, 2020

Choose a reason for hiding this comment

jkwatson Jan 21, 2020

Choose a reason for hiding this comment

jmacd Jan 21, 2020

Choose a reason for hiding this comment

jmacd Jan 21, 2020

Choose a reason for hiding this comment

bogdandrutu commented Feb 19, 2020

jkwatson commented Feb 19, 2020

bogdandrutu commented Feb 19, 2020

bogdandrutu commented Feb 19, 2020

jkwatson commented Feb 20, 2020

bogdandrutu commented Feb 20, 2020

jkwatson commented Feb 20, 2020

codecov-io commented Dec 30, 2019 •

edited

Loading