Allow multiple instances of OpenTelemetry primary objects #350
Conversation
pavolloffay
requested review from
bogdandrutu,
carlosalberto,
SergeyKanzhelev,
songy23,
tylerbenson and
yurishkuro
as code owners
|
/cc @fmhwong - Please give us feedback for this. |
Codecov Report
@@ Coverage Diff @@
## master #350 +/- ##
===========================================
- Coverage 79.43% 79.24% -0.2%
- Complexity 571 576 +5
===========================================
Files 65 65
Lines 1921 1951 +30
Branches 192 195 +3
===========================================
+ Hits 1526 1546 +20
- Misses 338 346 +8
- Partials 57 59 +2
Continue to review full report at Codecov.
|
|
will do |
| private static <T> T loadSpi(Class<T> providerClass) { | ||
| private static <T> T loadSpi(Class<T> providerClass, ClassLoader classLoader) { | ||
| if (classLoader.getParent() != null) { | ||
| return loadSpi(providerClass, classLoader.getParent()); |
There was a problem hiding this comment.
Please add a comment why you're making a recursive call. Why do you need the root classloader?
There was a problem hiding this comment.
+1 will update once we can confirm this works well in multi deployment app servers.
pavolloffay
force-pushed
the
global-registry-app-servers
branch
from
94a959a to
d641a16
Compare
|
@fmhwong I have fixed your comments. Could you please confirm this will work in OpenLiberty? |
|
What is the status for this? |
|
We are waiting for @fmhwong feedback if this API works well in OpenLiberty. |
|
I haven't got this to work in Open Liberty. Looks like this should work. I don't think we need to recursive get the parent of the classloader as classloader (at least in Liberty) will lookup from parent first . |
|
status on this? |
|
I think this could go in per @fmhwong comment. Just to recap in an app server the instrumentation resides in the server submodules and it's not part of the application deployment. We need a mechanism for the app server to use a different tracer per deployment/app (.war). A different tracer is needed because each app might use a different tracing config (sampling, reporting) but also, more importantly, it should use a different service name. open-telemetry/oteps#16 is also related topic, however I don't think it would solve this use case, but in contrast, we would have to provide maybe similar mechanisms for resolving the tracer components as we do here. |
Signed-off-by: Pavol Loffay <[email protected]>
pavolloffay
force-pushed
the
global-registry-app-servers
branch
from
d641a16 to
7bddc58
Compare
pavolloffay
changed the title
|
I have rebased the PR |
|
This PR is super old, and once again needs a rebase. Should we keep kicking this can down the road, or close it, or what? |
|
There's been no activity on this PR since September. Is this still needed? Should we close this and revisit when we have a concrete need? |
|
I suggest we close it and have any of the involved parties create a new PR if/when needed. |
Resolves #308
Signed-off-by: Pavol Loffay [email protected]