Skip to content

Avoid instrumenting HttpsURLConnectionImpl#1447

Merged
eyalkoren merged 3 commits intoelastic:masterfrom
eyalkoren:HttpUrlConnection-new-instr-fix
Oct 21, 2020
Merged

Avoid instrumenting HttpsURLConnectionImpl#1447
eyalkoren merged 3 commits intoelastic:masterfrom
eyalkoren:HttpUrlConnection-new-instr-fix

Conversation

@eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Oct 20, 2020 •
edited
Loading

What does this PR do?

Fixes a problem with the instrumentation of HttpUrlConnection: sun.net.www.protocol.https.HttpsURLConnectionImpl uses a delegate UrlConnection. It's own responseCode field is not updated, instead its getResponseCode() delegates. Since our instrumentation relies on the field's value in order to end the HTTP span, it fails to end it.

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
    • I have updated CHANGELOG.asciidoc
    • I have added tests that would fail without this fix
    • I have have tested manually that this scenario (using HTTPS calls though UrlConnection without calling java.net.HttpURLConnection#disconnect would fail without this fix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else
felixbarny
felixbarny reviewed Oct 20, 2020
View reviewed changes
...lugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java
@Override
public ElementMatcher<? super TypeDescription> getTypeMatcher() {
return hasSuperType(is(HttpURLConnection.class));
return hasSuperType(is(HttpURLConnection.class)).and(not(named("sun.net.www.protocol.https.HttpsURLConnectionImpl")));
Copy link
Member

@felixbarny felixbarny Oct 20, 2020

Choose a reason for hiding this comment

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

Suggested change
return hasSuperType(is(HttpURLConnection.class)).and(not(named("sun.net.www.protocol.https.HttpsURLConnectionImpl")));
return hasSuperType(is(HttpURLConnection.class))
.and(not(named("sun.net.www.protocol.https.HttpsURLConnectionImpl")))
.and(declaresField(named("connected")));
Copy link
Contributor Author

@eyalkoren eyalkoren Oct 20, 2020

Choose a reason for hiding this comment

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

Why declaresField(named("connected"))? It can be an inherited and not a declared field.
Copy link
Contributor Author

@eyalkoren eyalkoren Oct 21, 2020

Choose a reason for hiding this comment

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

@felixbarny This cannot work, as the connected field belongs to java.net.URLConnection (abstract). Let me know if you think we can merge it.
Copy link
Member

@felixbarny felixbarny Oct 21, 2020

Choose a reason for hiding this comment

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

Yes, forget what I said. @Advice.FieldValue("connected") boolean connected is guaranteed to work as the connected field is always accessible as we're searching for sub-types of HttpUrlConnection that are also sub-types of URLConnection
@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #1447 into master will increase coverage by 30.93%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1447       +/-   ##
=============================================
+ Coverage     28.88%   59.82%   +30.93%     
- Complexity       14       91       +77     
=============================================
  Files             6      388      +382     
  Lines            90    17503    +17413     
  Branches         10     2403     +2393     
=============================================
+ Hits             26    10471    +10445     
- Misses           61     6315     +6254     
- Partials          3      717      +714
Impacted Files Coverage Δ Complexity Δ
...rlconnection/HttpUrlConnectionInstrumentation.java 27.50% <100.00%> (ø) 0.00 <0.00> (?)
...bytebuddy/PatchBytecodeVersionTo51Transformer.java 100.00% <0.00%> (ø) 0.00% <0.00%> (?%)
.../kafka/ConsumerRecordsIteratorInstrumentation.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...lastic/apm/agent/impl/context/AbstractContext.java 77.27% <0.00%> (ø) 0.00% <0.00%> (?%)
...c/apm/agent/configuration/converter/ByteValue.java 89.47% <0.00%> (ø) 0.00% <0.00%> (?%)
.../main/java/co/elastic/apm/agent/util/JmxUtils.java 61.90% <0.00%> (ø) 0.00% <0.00%> (?%)
...nt/configuration/converter/ByteValueConverter.java 75.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...va/co/elastic/apm/agent/impl/transaction/Span.java 82.14% <0.00%> (ø) 0.00% <0.00%> (?%)
...stic/apm/agent/util/PotentiallyMultiValuedMap.java 98.43% <0.00%> (ø) 0.00% <0.00%> (?%)
...astic/apm/agent/metrics/builtin/CGroupMetrics.java 69.23% <0.00%> (ø) 0.00% <0.00%> (?%)
... and 373 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d10db0c...82a1b38. Read the comment docs.
@ghost
Copy link

ghost commented Oct 20, 2020 •
edited by ghost
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1447 updated]

  • Start Time: 2020-10-21T13:19:03.275+0000

  • Duration: 44 min 53 sec

Test stats 🧪

Test Results
Failed 0
Passed 1615
Skipped 12
Total 1627

felixbarny
felixbarny approved these changes Oct 21, 2020
View reviewed changes
...lugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java
@Override
public ElementMatcher<? super TypeDescription> getTypeMatcher() {
return hasSuperType(is(HttpURLConnection.class));
return hasSuperType(is(HttpURLConnection.class)).and(not(named("sun.net.www.protocol.https.HttpsURLConnectionImpl")));
Copy link
Member

@felixbarny felixbarny Oct 21, 2020

Choose a reason for hiding this comment

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

Yes, forget what I said. @Advice.FieldValue("connected") boolean connected is guaranteed to work as the connected field is always accessible as we're searching for sub-types of HttpUrlConnection that are also sub-types of URLConnection
@eyalkoren eyalkoren merged commit 1a888fd into elastic:master Oct 21, 2020
@eyalkoren eyalkoren deleted the HttpUrlConnection-new-instr-fix branch October 21, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@eyalkoren @codecov-io @felixbarny