Expose a new TracerSdkManagement interface on the OpenTelemetrySdk class. #1723
Conversation
… package-access class.
…than the SDK implementation class directly.
Codecov Report
@@ Coverage Diff @@
## master #1723 +/- ##
=========================================
Coverage 85.40% 85.40%
Complexity 1384 1384
=========================================
Files 164 164
Lines 5379 5379
Branches 559 559
=========================================
Hits 4594 4594
Misses 587 587
Partials 198 198
Continue to review full report at Codecov.
|
| @@ -36,7 +36,7 @@ | |||
| * io.opentelemetry.OpenTelemetry}. However, if you need a custom implementation of the factory, you | |||
| * can create one as needed. | |||
| */ | |||
| public class TracerSdkProvider implements TracerProvider { | |||
| public class TracerSdkProvider implements TracerProvider, TracerSdkManagement { | |||
There was a problem hiding this comment.
Does the new interface allow this to be private? And then maybe we don't need to change the return types
There was a problem hiding this comment.
what do you want to be private?
There was a problem hiding this comment.
If you want to make TracerSdkProvider package-private, we would need to move the SPI implementation into this package: TracerProviderFactorySdk. That seems possible, if we're ok with moving it.
There was a problem hiding this comment.
although it's used in a lot of tests as well right now, so we'd need to do quite a bit of test refactoring.
There was a problem hiding this comment.
I removed all the "production" usages except the SPI implementation class.
There was a problem hiding this comment.
I feel like moving the SPI implementation into this package is a win, less API is nice - I would say SPI package usually defines an SPI interface, but doesn't have to or even should define an SPI implementation. Doesn't have to be in this PR though.
exporters/inmemory/src/main/java/io/opentelemetry/exporters/inmemory/InMemoryTracing.java
Outdated
Show resolved
Hide resolved
get rid of almost all production-code references to the TracerSdkProvider
| @@ -36,7 +36,7 @@ | |||
| * io.opentelemetry.OpenTelemetry}. However, if you need a custom implementation of the factory, you | |||
| * can create one as needed. | |||
| */ | |||
| public class TracerSdkProvider implements TracerProvider { | |||
| public class TracerSdkProvider implements TracerProvider, TracerSdkManagement { | |||
There was a problem hiding this comment.
I feel like moving the SPI implementation into this package is a win, less API is nice - I would say SPI package usually defines an SPI interface, but doesn't have to or even should define an SPI implementation. Doesn't have to be in this PR though.
|
If I move this to a real PR, are we ok with metrics and baggage coming as a separate set of PRs? |
jkwatson
requested review from
arminru,
bogdandrutu,
carlosalberto,
pavolloffay,
thisthat and
tylerbenson
as code owners
|
This is a great change from auto-instrumentation perspective, as it cleanly separates TracerProvider (which we instrument) from the sdk management (which we do not instrument) 👍 |
|
do we want this change for 0.9.0, or should we wait until we can do the same with metrics and baggage before releasing in 0.10.0? |
Rather than expose the TracerSdkProvider directly, this introduces a new interface for managing the Tracing SDK. This will make it more obvious that the OpenTelemetrySdk access methods are not to be used for instrumentation purposes, but for operating the SDK itself.
This is a draft of how to resolve #1635 .
Note: this only covers the Tracing side of the house. Metrics and Baggage probably want the same treatment, but I wanted to keep this relatively limited to make sure we're ok with moving in this direction.