Fixing reliability issue with JedisMethodVisitor. #423
Conversation
|
@grlima, |
grlima
requested review from
Dmitry-Matveev,
SergeyKanzhelev,
dhaval24 and
gupele
| */ | ||
| @Deprecated | ||
| final class JedisMethodVisitor extends DefaultMethodVisitor { |
There was a problem hiding this comment.
@grlima just was thinking about the design decision. Wouldn't it be nice if we implement an interface like MethodVisitorInterface which has methods like getOnEnterMethod() and getOnEnterMethodSignature(). I believe this will provide extensibility and atleast in theory "Favor composition over inherritance" is true. Let me know your thoughts.
There was a problem hiding this comment.
it's a debatable design discussion and I do think it's a good idea, but since the existing functionality is based on inheritance (there are other methods in the super classes besides the getOn methods I introduced), I don't want to get into it at this time. We can track this and revisit the discussion at a later point.
| @@ -46,13 +46,13 @@ public JedisClassDataProvider(Map<String, ClassInstrumentationData> classesToIns | |||
| public void add() { | |||
| try { | |||
| ClassInstrumentationData data = | |||
| new ClassInstrumentationData(JEDIS_CLASS_NAME, InstrumentedClassType.OTHER) | |||
| new ClassInstrumentationData(JEDIS_CLASS_NAME, InstrumentedClassType.Redis) | |||
There was a problem hiding this comment.
From this Wiki entry it looks like agent already supports types other than Other though the configuration (and later though some method in Notification Handler or in Default Method Visitor) using Type filed rather than Kind field.
Since we are using DefaultMethodVisitor here and are changing Notification Handler as well - should we consider using existing type handling mechanism as Wiki suggests?
There was a problem hiding this comment.
If I understand this comment correctly, then you're asking for the ability to override the type via config. For Redis, I'm in favor of having the type being set out-of-box. Doing this via AI-Agent.xml would mean yet another piece of config the customer needs to set. It makes sense for generic cases classes, but since we want to support Redis out-of-box with minimal config, I would say this is not needed.
| /** | ||
| * The class is responsible for instrumenting Jedis client methods. | ||
| */ | ||
| final class JedisMethodVisitorV2 extends DefaultMethodVisitor { |
There was a problem hiding this comment.
Do we have unit tests of test infra for the other Method Visitors or ...MethodStarted() calls that we can extend with adding tests for this new one?
There was a problem hiding this comment.
Unfortunately, we don't. There are some tests for other "config" related classes such as ClassInstrumentationData and MethodInstrumentationDecision, but those test basic object assignments. For visitors, there seems to have been an attempt before to unit test those (see EnterExitClassVisitorTest), but the tests are commented out. I imagine any kind of test would have to verify the bytecode is actually instrumented after the visitor was executed, which might be a cumbersome thing to do.
Switch to a basic version which relies on DefaultMethodVisitor for most of the work. The only thing the new class (JedisMethodVisitorV2) overrides is the "onStart" method since we want the type of dependency to be "Redis" as opposed to "OTHER".