Downstream Http Calls should use hostname rather than full URL as subsegment name #192
Conversation
…e hostname, not the full URL Added unit tests for requests, httplib, aiohttp to validate this.
| return None | ||
| url_parse = urlparse(url) | ||
| hostname = url_parse.hostname | ||
| return hostname if hostname else url # If hostname is none, we return the regular URL; indication of malformed url |
There was a problem hiding this comment.
Is this fall-through behavior consistent with other SDKs? If we ask an HTTP client for a URL and get a malformed answer I'm not sure why we'd want to pass it back to the trace -- seems like a repeat of the issue this is intended to solve.
There was a problem hiding this comment.
Node does "unknown host" - https://github.com/aws/aws-xray-sdk-node/blob/master/packages/core/lib/patchers/http_p.js#L67
There was a problem hiding this comment.
this is in the case where the URL passed in might not necessarily be a parseable URL. Since customers can randomly invoke something like: requests.get("INVALID_DOMAIN_NAME"), we'd still want to be able to capture these types of errors and indicate failure in the subsegment. Otherwise, the subsegment won't be generated at all.
The behavior on our SDKs are mixed; some of them throw NP exceptions because the hostname technically doesn't exist, and therefore doesn't produce anything (which seems like a bug to me).
With that being said, I think doing something similar to Node would be the best of both worlds. Using a known "bad hostname" name, and then still capturing the exception in the subsegment.
Codecov Report
@@ Coverage Diff @@
## master #192 +/- ##
=========================================
+ Coverage 83.33% 83.4% +0.07%
=========================================
Files 77 77
Lines 2910 2923 +13
=========================================
+ Hits 2425 2438 +13
Misses 485 485
Continue to review full report at Codecov.
|
Issue #, if available:
#191
Description of changes:
Subsegment names generated by requests, httplib, aiohttp should be the hostname, not the full URL
Added unit tests for requests, httplib, aiohttp to validate this.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.