Support looking up a ContextStore from outside of Advice #3827
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| InstrumentationContext.internalSetContextStoreSupplier( | ||
| (keyClass, contextClass) -> FieldBackedProvider.getContextStore(keyClass, contextClass)); |
There was a problem hiding this comment.
I'd probably put that somewhere in InstrumentationLoader where InstrumentationContextProvider implementation gets chosen. Maybe that getContextStore() method should be a part of that provider interface?
There was a problem hiding this comment.
Moved to InstrumentationModuleInstaller
...api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InstrumentationContext.java
Outdated
Show resolved
Hide resolved
| if (contextStoreSupplier == null) { | ||
| throw new IllegalStateException(""); | ||
| } | ||
| return contextStoreSupplier.get(keyClass, contextClass); |
There was a problem hiding this comment.
I wonder if we still need to replace InstrumentationContext.get() calls in FieldBackedProvider - if the provider idea works for helper classes, wouldn't it work everywhere? Removing that part would certainly make FieldBackedProvider a bit simpler.
There was a problem hiding this comment.
I believe that it is better to keep replacing InstrumentationContext.get() calls in FieldBackedProvider because this InstrumentationContext.get() implementation isn't really efficient. It involves Class.forName and Class.getMethod which is fine in helper classes where you can do the lookup once and keep ContextStore in a field.
Resolves #766
Calling agent from instrumentation is implemented the same way as in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/javaagent-instrumentation-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/OpenTelemetrySdkAccess.java