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.

Sign up for GitHub

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

Jump to bottom

jetty-httpclient erroneously returns an empty response when instrumented #3826

Closed
seshness opened this issue Aug 11, 2021 · 6 comments · Fixed by #3831
Closed

jetty-httpclient erroneously returns an empty response when instrumented #3826

seshness opened this issue Aug 11, 2021 · 6 comments · Fixed by #3831
Assignees
@robododge
Labels
bug Something isn't working

Comments

@seshness
Copy link

Describe the bug
When I make a request with jetty-httpclient, with instrumentation applied, the response is empty.
When I make the same request without instrumentation, the response is non-empty as expected.

Steps to reproduce

  1. Create a project with jetty-httpclient
  2. Run this test with the jvm argument -javaagent:<path to opentelemetry-javaagent-all.jar>
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.api.ContentResponse;
import org.junit.jupiter.api.Test;

public class JettyClientTest {
    @Test
    public void testJettyResponse() throws Exception {
        HttpClient httpClient = new HttpClient();
        httpClient.start();

        ContentResponse response = httpClient.GET("http://google.com");
        assert response.getContentAsString().length() != 0;
    }
}

What did you expect to see?
When I make this request with curl or without instrumentation I see a non-empty response:

<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="http://www.google.com/">here</A>.
</BODY></HTML>

What did you see instead?
With instrumentation enabled the response is empty. response.getContentAsString() produces an empty string.

What version are you using?
org.eclipse.jetty:jetty-client:9.4.40.v20210413
opentelemetry-javaagent-all.jar at v1.4.1

Environment
Compiler: AdoptOpenJDK-11.0.11+9 (build 11.0.11+9)
OS: macOS 10.15.7
Runtime (if different from JDK above): AdoptOpenJDK-11.0.11+9 (build 11.0.11+9)
OS (if different from OS compiled on): macOS 10.15.7

Additional context
I tried reproducing with a few other jetty-httpclient versions too:

jetty-httpclient version works as expected?
9.4.40.v20210413 ❌
9.4.43.v20210629 ❌
10.0.6 ✅
11.0.6 ✅
@seshness seshness added the bug Something isn't working label Aug 11, 2021
@trask
Copy link
Member

trask commented Aug 12, 2021

cc: @robododge

@robododge
Copy link
Contributor

Hi @seshness, thanks for pointing this out.

I replicated this situation. There is definitely an issue with Jetty httpclient and redirects. A couple of things:

  1. If you truly want to see the 301 html as pasted in the bug, you will need to disable follow redirects. httpClient.setFollowRedirects(false);. Setting false, with or without, instrumentation will result in the 301 html snippet. The default for Jetty httpclient is to attempt to follow the redirect and make another http call.
  2. When allow follow redirect us true, I have confirmed there is empty content when instrumentation is active only.

So, yes, it appears to be an issue during an attempt to follow redirects when instrumentation is active.

@robododge
Copy link
Contributor

@trask please assign

@laurit
Copy link
Contributor

laurit commented Aug 12, 2021

My understanding is that the problem is with JettyClientWrapUtil.wrapResponseListeners. A listener can implement multiple interfaces, if the listener implements Response.CompleteListener our wrapping ends up effectively removing all other interfaces from that listener which breaks FutureResponseListener. I have a pr almost ready (waiting for it to build).

@laurit laurit mentioned this issue Aug 12, 2021
Merged
@seshness
Copy link
Author

Thanks for the quick fix @laurit & @robododge! ⚡
I tried the PR on both the minimal repro case from earlier and in my larger application, and it looks like it works.

@robododge
Copy link
Contributor

robododge commented Aug 13, 2021 •
edited
Loading

Thanks @laurit, I pulled down your PR and tested as well. It seems this problem occurs regardless of redirect handling when getContentAsString() is called because the onContent() callback was not getting invoked.

You covered the ResponseCallbacks, but I found another place where wrapping of the callbacks is not complete, the RequestListeners callback wrapping is missing some callback interfaces, should we add that fix to this PR?

Edit: new PR created

@robododge robododge mentioned this issue Aug 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@robododge robododge

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Improve wrapping jetty response listener laurit/opentelemetry-java-instrumentation
4 participants
@robododge @trask @seshness @laurit