Fix netty 4.1 instrumentation not removing future listeners #2851
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
...c/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/WrappedFutureListener.java
Outdated
Show resolved
Hide resolved
...java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/ChannelFutureInstrumentation.java
Outdated
Show resolved
Hide resolved
| public final class WrappedFutureListener<F extends Future<? super Void>> | ||
| implements GenericFutureListener<F> { | ||
|
|
||
| private static final Cache<GenericFutureListener<?>, WrappedFutureListener<?>> wrappers = |
There was a problem hiding this comment.
This seems like it can be InstrumentationContext instead of a map right?
There was a problem hiding this comment.
Yeah, I thought about it too - I don't know how it'd work with lambdas or method references, and how costly adding a field to lots of listener classes would be. (It's a good idea for a benchmark though)
There was a problem hiding this comment.
But isn't the weakmap a subset of InstrumentationContext? We use weakmap behind the scenes when not being able to add a field, and I figure when we can add the field it should basically work better.
There was a problem hiding this comment.
One problem with InstrumentationContext is that it is incapable of handling cases when the same object is used in different contexts e.g. same runnable is submitted to executor multiple times. With wrappers it should be possible to handle such cases, though the current solution doesn't.
There was a problem hiding this comment.
Okay, changed to InstrumentationContext.
One problem with InstrumentationContext is that it is incapable of handling cases when the same object is used in different contexts e.g. same runnable is submitted to executor multiple times.
Hmm, I guess that it's a separate InstrumentationContext problem that applies to the whole javaagent. We'd probably like to get it solved separately.
There was a problem hiding this comment.
One problem with InstrumentationContext is that it is incapable of handling cases when the same object is used in different contexts e.g. same runnable is submitted to executor multiple times.
Oh, that's an interesting problem, hadn't thought about that... have you seen this cause issues in real world apps (yet)?
Resolves #2705