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

Fix multiple spans being created for HTTPUrlConnection HEAD requests #3353

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

JonasKunz
Copy link
Contributor

@JonasKunz JonasKunz commented Oct 10, 2023

What does this PR do?

While testing with a local spring-boot app I noticed that out instrumentation created 14 spans per single RestTemplate request.

This issue occured because I instructed the RestTemplate to ignore the response body by setting the expected type to Void.class. This causes spring to send HEAD requests instead. And as it turns out, our HTTPUrlConnection instrumentation doesn't deal well with those:

The internal connected field is not switched to true for HEAD requests, causing multiple spans to be created when invoking getResponseCode multiple times.

This PR fixes this by also checking the responseCode field to infer whether the HTTPUrlConnection has already connected or not.

Checklist

  • This is a bugfix

@JonasKunz JonasKunz added the ci:jdk-compatibility Enables JDK compatibility tests in build pipeline label Oct 10, 2023
@JonasKunz JonasKunz requested a review from SylvainJuge October 10, 2023 11:09
Copy link
Member

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

Good catch !

@Advice.Origin String signature) {

//With HEAD requests the connected stays false
Copy link
Member

Choose a reason for hiding this comment

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

[minor] that's really an implementation detail here, could we also check the HTTP verb to be sure ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd argue that even for non-HEAD requests this approach is safer: If a responseCode is available, this implies that the connection has already been made. Therefore I think an additional check would not be good here.

Copy link
Member

Choose a reason for hiding this comment

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

Good for me, let's keep it as-is then !

@JonasKunz JonasKunz self-assigned this Oct 10, 2023
Copy link
Member

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Oct 10, 2023

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit d27a94b into elastic:main Oct 10, 2023
16 checks passed
@JonasKunz JonasKunz deleted the fix-urlconnection-head branch October 10, 2023 14:49
schikin pushed a commit to schikin/apm-agent-java that referenced this pull request Oct 11, 2023
…lastic#3353)

* Fix multiple spans being created for HTTPUrlConnection HEAD requests

* Added changelog

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
v1v added a commit to v1v/apm-agent-java that referenced this pull request Oct 27, 2023
* 'main' of https://github.com/elastic/apm-agent-java: (35 commits)
  buildkite: opentelemetry+elastic agent overhead benchmark on a weekly basis (elastic#3371)
  ci(action): remove unrequired action (elastic#3387)
  Bump org.springframework.boot:spring-boot-dependencies from 2.7.16 to 3.1.5 (elastic#3380)
  ci(action): archive benchmarks.jar (elastic#3386)
  remove noise (elastic#3385)
  Process otel benchmark (elastic#3384)
  Update tests for Jedis 5 (elastic#3382)
  added entry to community plugins (elastic#3383)
  added capturing s3 otel attributes from lamba S3Event (elastic#3364)
  Bump version.log4j from 2.12.4 to 2.21.0 (elastic#3366)
  Fix aws sdk instrumentation (elastic#3373)
  action: remove leftover from Jenkins (elastic#3368)
  Bump org.apache.logging.log4j:log4j-bom from 2.20.0 to 2.21.0 (elastic#3367)
  Bump io.micrometer:micrometer-core from 1.11.4 to 1.11.5 (elastic#3360)
  Add guard against invalid end timestamps (elastic#3363)
  Added otel to dependabot config, upgraded dependencies (elastic#3359)
  Bump version.byte-buddy from 1.14.8 to 1.14.9 (elastic#3361)
  Fix off-by-one error in tests which effectively disabled recycling validation (elastic#3357)
  Fix multiple spans being created for HTTPUrlConnection HEAD requests (elastic#3353)
  Bump org.ow2.asm:asm-tree from 9.5 to 9.6 (elastic#3349)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java ci:jdk-compatibility Enables JDK compatibility tests in build pipeline ready-to-merge
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants