-
Notifications
You must be signed in to change notification settings - Fork 829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose a new TracerSdkManagement interface on the OpenTelemetrySdk class. #1723
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you want to be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all the "production" usages except the SPI implementation class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.