return resource in auto configured sdk #7639
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7639 +/- ##
=========================================
Coverage 90.13% 90.13%
- Complexity 7192 7197 +5
=========================================
Files 814 814
Lines 21713 21728 +15
Branches 2127 2127
=========================================
+ Hits 19570 19585 +15
Misses 1477 1477
Partials 666 666 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zeitlinger
force-pushed
the
declarative-config-return-resource
branch
from
1b36c6e to
5f388fb
Compare
| Method create = | ||
| declarativeConfiguration.getMethod( | ||
| "create", openTelemetryConfiguration, ComponentLoader.class); | ||
| "createAutoConfiguredSdk", openTelemetryConfiguration, ComponentLoader.class); |
There was a problem hiding this comment.
Let's keep calling this create, which comes from the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#create
I think we have a good argument that the AutoConfiguredOpenTelemetrySdk is just a composite of all the required SDK components.
There was a problem hiding this comment.
create already exists with the same parameters - it returns ExtendedOpenTelemetrySdk
| import java.lang.reflect.Method; | ||
| import java.util.Objects; | ||
|
|
||
| class AutoConfiguredOpenTelemetrySdkAccess { |
There was a problem hiding this comment.
We could also move this to an internal package in the autoconfigure module. I don't think it matters too much. Just a matter of perspective: is the autoconfigure module exposing experimental access for other components to call AutoConfiguredOpenTelemetrySdk#create or is file config "breaking into" the autoconfigure module?
There was a problem hiding this comment.
Great idea - moving to incubating util also removes the createAutoConfiguredSdk method, because that's now done using reflection
zeitlinger
force-pushed
the
declarative-config-return-resource
branch
from
5f388fb to
16e1492
Compare
|
@jack-berg please check again |
| } | ||
|
|
||
| Resource getResource() { | ||
| // called via reflection from io.opentelemetry.sdk.autoconfigure.IncubatingUtil |
There was a problem hiding this comment.
Do you think this comment may be useful?
On one hand it explains why no one should remove this method, but on the other hand it will be easy to forget to update/remove it when declarative config stuff goes out of incubator.
Maybe instead of this comment it would be better to add a unit test in IncubatingUtilTest that verifies that getResource is actually called.
There was a problem hiding this comment.
the test is already there - but I get how it can get out of sync
|
|
||
| Resource getResource() { | ||
| // called via reflection from io.opentelemetry.sdk.autoconfigure.IncubatingUtil | ||
| return resource; |
There was a problem hiding this comment.
Maybe throw an exception if getResource is called before setResource to make it easier to track down ordering issues.
Alternative to #7418 that
AutoConfiguredOpenTelemetrySdkFixes open-telemetry/opentelemetry-java-instrumentation#14325