Add Muzzle reference creation for interfaces #2155
Conversation
tylerbenson
force-pushed
the
tyler/muzzle-super
branch
from
f489ebd to
bf7f42b
Compare
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java
Outdated
Show resolved
Hide resolved
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/muzzle/Reference.java
Outdated
Show resolved
Hide resolved
| for (String iface : interfaces) { | ||
| addReference( | ||
| new Reference.Builder(iface) | ||
| .withSource(refSourceClassName, 0) // We don't have a specific line number to use. |
There was a problem hiding this comment.
this is inconsistent with behaviour elsewhere in the class where the value -1 is used when no line number is known.
There was a problem hiding this comment.
I'm not sure that the existing behaviour is correct. Line numbers will be missing if code has been passed through an obfuscator, for instance, in which case MethodVisitor.visitLineNumber wouldn't be entered, and line number 0 may be a better fallback value than -1.
There was a problem hiding this comment.
This only matters for our own code, so the case of an obfuscator doesn't likely apply. I don't care between -1 and 0, but I did extract it out to a static final.
| // public static MethodBodyAdvice.SomeInterface indyMethod( | ||
| // final MethodBodyAdvice.SomeImplementation a) { | ||
| // public static MethodBodyAdvice.HasMethod indyMethod(final MethodBodyAdvice.HasMethod a) { | ||
| // Runnable aStaticMethod = MethodBodyAdvice.B::aStaticMethod; | ||
| // return a::someMethod; | ||
| // return a::requiredMethod; |
There was a problem hiding this comment.
This is also an unrelated change... I uncommented it to verify it still worked and found it didn't, so I updated it here.
dd-java-agent/testing/src/test/groovy/muzzle/ReferenceCreatorTest.groovy
Outdated
Show resolved
Hide resolved
This will allow muzzle to fail if an implemented interface is missing.
tylerbenson
force-pushed
the
tyler/muzzle-super
branch
from
bf7f42b to
8ce7658
Compare
|
I added this so that #2054 is able to distinguish a version where the implemented interface name changed/moved. |
| @@ -240,7 +240,8 @@ private void decrementRefAndMaybeWrite(boolean isRootSpan) { | |||
| } | |||
| } | |||
| if (log.isDebugEnabled()) { | |||
| log.debug("t_id={} -> expired reference. pending count={}", traceId, count); | |||
There was a problem hiding this comment.
Why was this committed with this PR?
There was a problem hiding this comment.
Yes, agreed. That seems completely unrelated and should not be in this PR.
There was a problem hiding this comment.
it is unrelated to the pr. I made this change to troubleshoot some build failures, but the build passed following this change. It seems like a useful and very minor change so I left it in.
|
@tylerbenson this PR is called "Add Muzzle reference creation for interfaces" yet a change to |
|
For reference you can see the history of me running the test multiple times to try to figure out what was wrong here: https://app.circleci.com/pipelines/github/DataDog/dd-trace-java?branch=tyler%2Fmuzzle-super |
This will allow muzzle to fail if an implemented interface is missing. Additional work should be done in the future to ensure the implementation satisfies the interfaces and abstract types. (open-telemetry/opentelemetry-java-instrumentation#1357)