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 causality test with multiple concurrent requests #2156

Merged
merged 10 commits into from
Feb 5, 2021

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Feb 1, 2021

Several things happen in this PR, sorry for such a soup. I can split it into several PR if needed...

First, a new "causality test" was introduced to the HttpClientTest. It stresses our instrumentations of http clients in highly concurrent settings. Please read the test comments.

Second, this new test has to be disabled for a lot of http clients. This is very concerning, see #2155

Third, Reactor Netty 0.9 test was converted to the subclass of HttpClientTest.

Fourth, integration with Reactor Netty 1.0 was introduced.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, a new "causality test" was introduced to the HttpClientTest. It stresses our instrumentations of http clients in highly concurrent settings. Please read the test comments.

❤️

Second, this new test has to be disabled for a lot of http clients. This is very concerning, see #2155

😢

Third, Reactor Netty 0.9 test was converted to the subclass of HttpClientTest.

👍

Fourth, integration with Reactor Netty 1.0 was introduced.

🎉

@iNikem
Copy link
Contributor Author

iNikem commented Feb 3, 2021

@mateuszrzeszutek @trask Can you help me? I don't understand why muzzle fails on Reactor Netty with

MUZZLE PASSED NettyInstrumentationModule BUT FAILURE WAS EXPECTED

> Task :instrumentation:reactor-netty-0.9:javaagent:muzzle-AssertFail-io.projectreactor.netty-reactor-netty-1.0.3 FAILED

Why does it fail due to NettyInstrumentationModule?

@mateuszrzeszutek
Copy link
Member

Hmm, it looks like for fail directives the instrumentation classloader is missing the reactor-netty instrumentation module:

(example for 1.0)

> Task :instrumentation:reactor-netty-1.0:javaagent:muzzle-AssertFail-io.projectreactor.netty-reactor-netty-0.9.12.RELEASE FAILED
CHECKING NettyInstrumentationModule
MUZZLE PASSED NettyInstrumentationModule BUT FAILURE WAS EXPECTED

> Task :instrumentation:reactor-netty-1.0:javaagent:muzzle-AssertPass-io.projectreactor.netty-reactor-netty-http-1.0.3
CHECKING NettyInstrumentationModule
CHECKING ReactorNettyInstrumentationModule

There must be something wrong with how MuzzlePlugin prepares classloaders

@iNikem
Copy link
Contributor Author

iNikem commented Feb 3, 2021

@mateuszrzeszutek I look at io.opentelemetry.javaagent.tooling.muzzle.matcher.MuzzleGradlePluginUtil#assertInstrumentationMuzzled and I see that

        if (passed && !assertPass) {
          System.err.println(
              "MUZZLE PASSED "
                  + instrumentationModule.getClass().getSimpleName()
                  + " BUT FAILURE WAS EXPECTED");
          throw new RuntimeException("Instrumentation unexpectedly passed Muzzle validation");

Meaning that even if there are 2 modules present, the plugin throws exception and fails the build as soon as any of them pass muzzle. Which does not make sense in my case. I depend on classes from netty instrumentation, thus NettyInstrumentationModule is present on my classpath, but it certainly passes all muzzle checks, regardless of Reactor Netty version.

@mateuszrzeszutek
Copy link
Member

Meaning that even if there are 2 modules present, the plugin throws exception and fails the build as soon as any of them pass muzzle.

Oh, that's true! I was so concentrated on the CL & gradle plugin thing that I missed the loop 😅
Yes, I suppose that's where the problem is - this passed && !assertPass if should be outside of the loop.

@iNikem
Copy link
Contributor Author

iNikem commented Feb 3, 2021

I went another way... Let's see...

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much for pushing on this much-needed test type.

@iNikem iNikem requested a review from trask February 3, 2021 19:38
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great test addition 👍

Two last suggestions, no worries if you disagree please ignore.

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

Successfully merging this pull request may close these issues.

None yet

6 participants