Skip to content

Split global functionality into a separate class#2233

Merged
bogdandrutu merged 1 commit into
open-telemetry:masterfrom
bogdandrutu:global
Dec 15, 2020
Merged

Split global functionality into a separate class#2233
bogdandrutu merged 1 commit into
open-telemetry:masterfrom
bogdandrutu:global

Conversation

@bogdandrutu

@bogdandrutu bogdandrutu commented Dec 8, 2020

Copy link
Copy Markdown
Member

This ensure clear separation of functionality. This PR does not change any functionality, it just restructures the code to separate global functionality from
the Default implementation. Also helps to ensure that Global initialization does not happen by mistake when working only with the default implementation.

Also it helps with methods like OpenTelemetry.get() and OpenTelemetry.set() to understand they are interacting with global instance

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

@codecov

codecov Bot commented Dec 8, 2020

Copy link
Copy Markdown

Codecov Report

Merging #2233 (c8a214a) into master (44186bc) will decrease coverage by 0.12%.
The diff coverage is 83.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2233      +/-   ##
============================================
- Coverage     87.82%   87.70%   -0.13%     
+ Complexity     2431     2429       -2     
============================================
  Files           268      270       +2     
  Lines          8157     8163       +6     
  Branches        906      906              
============================================
- Hits           7164     7159       -5     
- Misses          660      672      +12     
+ Partials        333      332       -1     
Impacted Files Coverage Δ Complexity Δ
...ava/io/opentelemetry/api/DefaultOpenTelemetry.java 100.00% <ø> (+2.85%) 6.00 <0.00> (-11.00) ⬆️
.../main/java/io/opentelemetry/api/OpenTelemetry.java 22.22% <0.00%> (-72.78%) 4.00 <0.00> (-10.00)
...y/sdk/testing/exporter/InMemoryMetricExporter.java 100.00% <ø> (ø) 8.00 <0.00> (ø)
...java/io/opentelemetry/api/GlobalOpenTelemetry.java 95.83% <95.83%> (ø) 14.00 <14.00> (?)
...opentelemetry/api/DefaultOpenTelemetryBuilder.java 100.00% <100.00%> (ø) 9.00 <0.00> (ø)
.../all/src/main/java/io/opentelemetry/api/Utils.java 100.00% <100.00%> (ø) 5.00 <5.00> (?)
...ntelemetry/exporter/otlp/OtlpGrpcSpanExporter.java 92.30% <100.00%> (ø) 5.00 <0.00> (ø)
...io/opentelemetry/opencensusshim/SpanConverter.java 81.74% <100.00%> (ø) 28.00 <0.00> (ø)
...opentelemetry/opentracingshim/OpenTracingShim.java 100.00% <100.00%> (ø) 3.00 <0.00> (ø)
...elemetry/sdk/logging/export/BatchLogProcessor.java 82.83% <100.00%> (ø) 6.00 <0.00> (ø)
... and 8 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 44186bc...c8a214a. Read the comment docs.

* @see MeterProvider
* @see ContextPropagators
*/
public final class GlobalOpenTelemetry {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a big improvement. I think we need to compare with removing the getGlobalTracer type methods in favor of OpenTelemetry.get().getTracer() or OpenTelemetry.getGlobal().getTracer() or getGlobalOpentelemetry().getTracer() (static import). I think they're all fine, latter might lead to more testable user code by reducing the number of static methods involved.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I would still not have the "global" functionality on the interface OpenTelemetry. Having this separation is a win in my opinion. Not sure I understand what comparison you are asking me to do.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@anuraaga any further thoughts at this point? I'm inclined to this this PR the 👍 , but want to make sure your concerns are considered.

@jkwatson

jkwatson commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

I think this is a fantastic change. It's definitely a big breaking change, though. Should we go through a deprecation cycle on the global methods on the OpenTelemetry API to give users a chance to adapt? This seems like a big enough break that it warrants a deprecation cycle.

@bogdandrutu

Copy link
Copy Markdown
Member Author

@jkwatson done.

@jkwatson jkwatson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@jkwatson

Copy link
Copy Markdown
Contributor

@bogdandrutu this needs a rebase. Thanks!

This ensure clear separation of functionality. This PR does not change any functionality, it just restructures the code to separate global functionality from
the Default implementation. Also helps to ensure that Global initialization does not happen by mistake when working only with the default implementation.

Also it helps with methods like `OpenTelemetry.get()` and `OpenTelemetry.set()` to understand they are interacting with global instance

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu

Copy link
Copy Markdown
Member Author

@jkwatson done.

@bogdandrutu bogdandrutu merged commit 25bf879 into open-telemetry:master Dec 15, 2020
@bogdandrutu bogdandrutu deleted the global branch December 15, 2020 18:27
bogdandrutu added a commit to bogdandrutu/opentelemetry-java that referenced this pull request Dec 15, 2020
This ensure clear separation of functionality. This PR does not change any functionality, it just restructures the code to separate global functionality from
the Default implementation. Also helps to ensure that Global initialization does not happen by mistake when working only with the default implementation.

Also it helps with methods like `OpenTelemetry.get()` and `OpenTelemetry.set()` to understand they are interacting with global instance

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit to bogdandrutu/opentelemetry-java that referenced this pull request Dec 15, 2020
This ensure clear separation of functionality. This PR does not change any functionality, it just restructures the code to separate global functionality from
the Default implementation. Also helps to ensure that Global initialization does not happen by mistake when working only with the default implementation.

Also it helps with methods like `OpenTelemetry.get()` and `OpenTelemetry.set()` to understand they are interacting with global instance

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
bogdandrutu added a commit that referenced this pull request Dec 15, 2020
This ensure clear separation of functionality. This PR does not change any functionality, it just restructures the code to separate global functionality from
the Default implementation. Also helps to ensure that Global initialization does not happen by mistake when working only with the default implementation.

Also it helps with methods like `OpenTelemetry.get()` and `OpenTelemetry.set()` to understand they are interacting with global instance

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants