Allow scanning instrumented reactor publishers and only allow registe… #5755
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mateuszrzeszutek
approved these changes
Apr 6, 2022
| @Test | ||
| void fluxParentsAccessible() { | ||
| UnicastProcessor<String> source = UnicastProcessor.create(); | ||
| Flux<String> mono = ContextPropagationOperator.runWithContext(source, Context.root()); |
There was a problem hiding this comment.
Nit
Suggested change
| Flux<String> mono = ContextPropagationOperator.runWithContext(source, Context.root()); | |
| Flux<String> flux = ContextPropagationOperator.runWithContext(source, Context.root()); |
anuraaga
commented
Apr 6, 2022
| @@ -98,17 +101,22 @@ public static Context getOpenTelemetryContext( | |||
| * reactive stream. This should generally be called in a static initializer block in your | |||
| * application. | |||
| */ | |||
| public void registerOnEachOperator() { | |||
| public synchronized void registerOnEachOperator() { | |||
There was a problem hiding this comment.
When trying to use AtomicBoolean I got this bizzare exception in KotlinCoroutinesInstrumentationTest. Synchronizing for these should be fine anyways, but is it an agent bug?
[otel.javaagent 2022-04-06 07:40:51:716 +0000] [otel-javaagent-transform-safe-logger] DEBUG io.opentelemetry.javaagent.tooling.AgentInstaller$TransformLoggingListener - Failed to handle io.opentelemetry.instrumentation.reactor.ContextPropagationOperator for transformation on classloader jdk.internal.loader.ClassLoaders$AppClassLoader@7ad041f3
java.lang.IllegalStateException: Cannot assign private static final java.util.concurrent.atomic.AtomicBoolean io.opentelemetry.instrumentation.reactor.ContextPropagationOperator.enabled to boolean
at net.bytebuddy.asm.Advice$OffsetMapping$ForField.resolve(Advice.java:2317)
at net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$ForMethodEnter.doApply(Advice.java:8684)
at net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$ForMethodEnter$WithDiscardedEnterType.doApply(Advice.java:8786)
at net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$ForMethodEnter.apply(Advice.java:8642)
at net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$AdviceMethodInliner.visitMethod(Advice.java:8332)
at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1353)
at org.objectweb.asm.ClassReader.accept(ClassReader.java:744)
at org.objectweb.asm.ClassReader.accept(ClassReader.java:424)
at net.bytebuddy.asm.Advice$Dispatcher$Inlining$Resolved$AdviceMethodInliner.apply(Advice.java:8325)
at net.bytebuddy.asm.Advice$AdviceVisitor.onAfterExceptionTable(Advice.java:10579)
at net.bytebuddy.utility.visitor.ExceptionTableSensitiveMethodVisitor.considerEndOfExceptionTable(ExceptionTableSensitiveMethodVisitor.java:49)
at net.bytebuddy.utility.visitor.ExceptionTableSensitiveMethodVisitor.visitLabel(ExceptionTableSensitiveMethodVisitor.java:81)
There was a problem hiding this comment.
There was a problem hiding this comment.
since it's a public class, better to avoid synchronizing on this?
trask
approved these changes
Apr 7, 2022
RashmiRam
pushed a commit
to RashmiRam/opentelemetry-auto-instr-java
that referenced
this pull request
May 23, 2022
open-telemetry#5755) * Allow scanning instrumented reactor publishers and only allow registering once. * Avoid atomicboolean * Clean * Bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ring once.
While working on some other test code, I noticed that we add a context propagating publisher to the chain that does not allow scanning it's parent, unlike all the standard reactor publishers. While not strictly necessary based on the publisher spec, it's good to not lose this functionality.
Also noticed it seems possible to register hooks twice so added a guard.