Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some extra check to muzzle that allow it to check all TypeInstrumentation's transform methods are valid #10652

Open
123liuziming opened this issue Feb 23, 2024 · 3 comments

Comments

@123liuziming
Copy link
Contributor

123liuziming commented Feb 23, 2024

Is your feature request related to a problem? Please describe.

For example, the rocketmq 5.0.0 does not support the PublishingMessageImplInstrumentation's transform method. The org.apache.rocketmq.client.java.impl.producer.PublishingSettings is only supported since 5.0.1, which is named
"ProducerSettings" in 5.0.0. The muzzle check can not handle this.
image
image

Describe the solution you'd like

I want to make an enhancement to muzzle-check, to check all the mismatches in all TypeInstrumentation's transform methods.
The effect of executing the ./gradlew :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle command should be as shown below

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.0 FAILED
FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.RocketMqInstrumentationModule mismatches:
-- io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.ConsumeServiceInstrumentation$ConstructorAdvice:52 Missing method org.apache.rocketmq.client.java.impl.ClientImpl#getEndpoints()Lorg/apache/rocketmq/client/java/route/Endpoints;
-- <no-source> io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.PublishingMessageImplInstrumentation: method unmatched: (isConstructor() and isPublic() and hasParameter(hasTypes(with(0 matches erasure(name(equals(org.apache.rocketmq.client.apis.message.Message)))))) and hasParameter(hasTypes(with(1 matches erasure(name(equals(org.apache.rocketmq.client.java.impl.producer.PublishingSettings)))))) and hasParameter(hasTypes(with(2 matches erasure(is(boolean)))))),
-- <no-source> io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.ConsumeServiceInstrumentation: method unmatched: (isConstructor() and isPublic() and hasParameter(hasTypes(with(1 matches erasure(name(equals(org.apache.rocketmq.client.apis.consumer.MessageListener)))))) and hasParameter(hasTypes(with(3 matches erasure(name(equals(org.apache.rocketmq.client.java.hook.MessageInterceptor))))))),

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.6
Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.6 elapsed time: 3548ms

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.2
Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.2 elapsed time: 2911ms

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.3
Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.3 elapsed time: 2798ms

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.1 FAILED
FAILED MUZZLE VALIDATION: io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.RocketMqInstrumentationModule mismatches:
-- io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.ConsumeServiceInstrumentation$ConstructorAdvice:52 Missing method org.apache.rocketmq.client.java.impl.ClientImpl#getEndpoints()Lorg/apache/rocketmq/client/java/route/Endpoints;

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.5
Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.5 elapsed time: 6932ms

> Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.4
Task :instrumentation:rocketmq:rocketmq-client:rocketmq-client-5.0:javaagent:muzzle-AssertPass-org.apache.rocketmq-rocketmq-client-java-5.0.4 elapsed time: 2188ms

Describe alternatives you've considered

No response

Additional context

No response

@123liuziming 123liuziming added enhancement New feature or request needs triage New issue that requires triage labels Feb 23, 2024
@trask
Copy link
Member

trask commented Feb 27, 2024

hi @123liuziming! can you describe the problem you're trying to solve a bit more? is this just a debugging option you want to add? I believe there are several instrumentations that use multiple method matchers to handle different library versions

@steverao steverao added needs author feedback Waiting for additional feedback from the author and removed enhancement New feature or request needs triage New issue that requires triage labels Feb 28, 2024
@123liuziming
Copy link
Contributor Author

For example, the RocketMQ 5.0 plugin should use multiple method matchers to handle 5.0.0 version and [5.0.1) version, but the plugin does not do that, which results in some instrumentation failures when using RocketMQ version 5.0.0.
I 'm trying to add some extra check while doing muzzle-check to solve the problem above. For example, if the result of muzzle-check of RocketMQ plugin is is shown like below:

-- <no-source> io.opentelemetry.javaagent.instrumentation.rocketmqclient.v5_0.PublishingMessageImplInstrumentation: method unmatched: (isConstructor() and isPublic() and hasParameter(hasTypes(with(0 matches erasure(name(equals(org.apache.rocketmq.client.apis.message.Message)))))) and hasParameter(hasTypes(with(1 matches erasure(name(equals(org.apache.rocketmq.client.java.impl.producer.PublishingSettings)))))) and hasParameter(hasTypes(with(2 matches erasure(is(boolean)))))),

We will find out that the transform in PublishingMessageImplInstrumentation is not right, there is no method that has the type like below:
image

I believe the extra check in muzzle will help us find the problem if we transform a wrong method. Under these circumstances we should use multiple method matchers to handle different library versions.

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Feb 28, 2024
@123liuziming
Copy link
Contributor Author

I 'm trying to add some extra check in io.opentelemetry.javaagent.tooling.muzzle.ClassLoaderMatcher. To match the class in RocketMQ and other library and the MethodMatcher in transform method of TypeInstrumentation. I 've finished a POC.

@123liuziming 123liuziming changed the title Enhancements to muzzle-check allow it to check that all TypeInstrumentation's transform methods are valid Add some extra check to muzzle that allow it to check all TypeInstrumentation's transform methods are valid Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants