Instrument HystrixCommand and HystrixThreadPool#306
Conversation
tylerbenson
force-pushed
the
tyler/agent-installer
branch
from
c3e22c7 to
84b7080
Compare
tylerbenson
force-pushed
the
tyler/hystrix
branch
2 times, most recently
from
07a66d7 to
be55fc7
Compare
There was a problem hiding this comment.
@realark do you think we want to allow instrumenting proxy classes?
(This isn't related to hystrix, but to the JMS tests.)
There was a problem hiding this comment.
I'm okay with instrumenting proxy classes, but usually the instrumentation fails to apply because the proxy class has no source code (for type checking etc).
There was a problem hiding this comment.
Should we use a static name for the operation name?
There was a problem hiding this comment.
This follows the same semantic of @Trace. If we want to change it here, it should be changed with @Trace too.
If so, we have to change the test to be compatible.
realark
left a comment
realark
left a comment
There was a problem hiding this comment.
Looks good, nice work!
My only concern is the operationName of the trace instrumentation. It's doing the same thing as @Trace, but this instrumentation could expose that problem because it will apply by default (rather than a user going out of their way to add @Trace).
Since the instrumentation is disabled by default I'm okay with merging, but we'll have to figure out an approach to operationName before we turn it on by default.
2 participants
1.3.15was the version they addedHystrixContextScheduler$ThreadPoolWorker.I also added a new DSL for asserting traces inside the tests. We can slowly migrate other tests as desired.