httplib support #19

thehesiod · 2018-02-13T21:02:32Z

This is what we're going to use in production. It's important for tracing things like the old google API, which uses httplib2 which uses httplib.

NOTE: perhaps this should replace the requests patch as requests uses httplib

fixes: #20

thehesiod · 2018-02-13T21:07:00Z

oo, should get the unittests setup via Travis/etc

thehesiod · 2018-02-14T20:18:52Z

one question for this PR is it has three segments, I'm not sure I can coalesce it to two given redirects

haotianw465

Thank you so much for the PR and we really appreciate your time for adding extra support. I have left a few comments. No major issue so I expect this can be merged soon.

haotianw465 · 2018-02-16T00:07:23Z

aws_xray_sdk/core/patcher.py

@@ -10,6 +10,7 @@
    'requests',
    'sqlite3',
    'mysql',
+    'httplib',  # TODO: perhaps requests should map to this below


I would keep the requests patch for now. If a user's intention is to only patch requests but has other libraries depend on httplib, then patching at a lower level may result in unexpected behavior. IMO the SDK should only patch httplib when users explicit do so.

A small paragraph on https://github.com/aws/aws-xray-sdk-python/blob/master/docs/thirdparty.rst explaining httplib/httplib2/requests dependency and which to patch could be very helpful.

removed comment, and added some docs

haotianw465 · 2018-02-16T00:09:48Z

aws_xray_sdk/ext/httplib/patch.py

+
+
+# ? is not a valid entity, and we don't want things after the ? for the segment name
+def _strip_url(url: str):


Could you move this helper method to aws_xray_sdk.ext.util? You use it on requests patch as well. It can be further used to strip any url string on any library patcher.

BTW as a helper method a None check could be useful in case the caller forgets to do the check.

moved, didn't put the None check as that's a fairly low-level API and you'll get an exception of operating on a 'None' already...thoughts?

I would suggest make it no-op if input is None. This becomes a general helper under a utility module so it could be used on logging, other patching code etc by other developers. It is bad if the SDK throws a None type error from this part.

Also a nitpick could you remove the prefix _ since its usage is not private?

haotianw465 · 2018-02-16T00:13:21Z

aws_xray_sdk/ext/httplib/patch.py

+    subsegment.put_http_meta(http.METHOD, xray_data.method)
+    subsegment.put_http_meta(http.URL, xray_data.url)
+    subsegment.put_http_meta(http.STATUS, instance.status)
+    subsegment.apply_status_code(instance.status)


You don't have to manually call apply_status_code. put_http_meta has logic to adjust fault/error/throttle flags if a status code is added.

ahh, probably copied from an old version of the AWS SDK, removed

haotianw465 · 2018-02-16T00:14:09Z

aws_xray_sdk/ext/httplib/patch.py

+        xray_data = _XRay_Data(method, instance.host, xray_url)
+        setattr(instance, _XRAY_PROP, xray_data)
+
+        return xray_recorder.record_subsegment(


Is there any reason to record this local computation as a subsegment?

IIRC if the connect fails, getresponse will not be called.

haotianw465 · 2018-02-16T00:17:49Z

aws_xray_sdk/ext/httplib/patch.py

+
+    wrapt.wrap_function_wrapper(
+        'http.client',
+        'HTTPConnection.getresponse',


You are patching both HTTPConnection.getresponse and HTTPResponse.read. Is the latter some duplicate work since the former already set url, method and status?

not sure I follow, on the read you're reading the body, which you may not do on 204s, and you want all the information from the requests to be marked on the subsegment of the read in case the response segment is sampled out.

Understood.

haotianw465 · 2018-02-16T00:20:53Z

tests/ext/httplib/test_httplib.py

+    url = 'http://{}/status/{}?foo=bar&baz=foo'.format(BASE_URL, status_code)
+    requests.get(url)
+    subsegment = xray_recorder.current_segment().subsegments[1]
+    assert subsegment.name == url


Can this assertion pass when query string is stripped?

Based on your patching code you use the string before "?" as the subsegment name.
So to verify the http subsegment is named correctly, should the test asserts name == http://{}/status/{} and pass?

haotianw465 · 2018-02-16T00:30:43Z

tests/ext/httplib/test_httplib.py

+    xray_recorder.clear_trace_entities()
+
+
+def test_ok():


Not sure your intention on using requests syntax to test httplib patch. This unit test is valid given the knowledge of requests depends on httplib. But I feel like this is a little bit anti-pattern since the scope is beyond httplib and involves requests implementation.

rewrote to use httplib

haotianw465 · 2018-02-16T00:31:28Z

tests/ext/requests/test_requests.py

@@ -28,7 +28,7 @@ def construct_ctx():

 def test_ok():
    status_code = 200
-    url = 'http://{}/status/{}'.format(BASE_URL, status_code)
+    url = 'http://{}/status/{}?foo=bar'.format(BASE_URL, status_code)
    requests.get(url)
    subsegment = xray_recorder.current_segment().subsegments[0]
    assert subsegment.name == url


Same question about the segment name assertion

thehesiod · 2018-02-16T01:15:32Z

is it possible to quickly add travis or circle ci support to this project? It's pretty simple and free and I think will help a lot, here's an example: https://github.com/aio-libs/aiobotocore/blob/master/.travis.yml

haotianw465 · 2018-02-16T01:35:15Z

The PR looks good on most part. But I would like to point out some concerns on the structure of the subsegments this patching code is generating.

In a steady state one http outbound call is broken into two subsegments. One is for pre_req and one is for getresponse. These two are siblings with the same name (the url without query string). The service graph statistics will be impacted on both call rate and avg latency on that node.

Ideally in a successfully case there should be only one subsegment representing one complete round trip. In a failure case there is one subsegment representing the failed connection.

I understand the ideal case could be a challenge given this is a low level library and a logical operation is broken into several functions. But on the X-Ray service graph level one endpoint should be abstracted to one node with correct statistics.

Another question is that you are saying one http call will generate three subsegments? From the PR I can only see two.

thehesiod · 2018-02-16T03:21:15Z

ya I agree, I'm not sure the best way to proceed. Perhaps the _prep_request segment can be cancelled if _xray_traced_http_client is reached, thoughts? Further, we need to make sure each redirect is captured...I think http_request_processor + http_response_processor takes care of this. I'll add a unittest after we decide what to do.

the third segment will come in if you call read() on the body of the response.

haotianw465 · 2018-02-19T19:50:11Z

I agree on only keep the subsegment for actual internet round trip on a successful case. Also read() is probably not necessary here as it is purely cache read? Otherwise your avg latency will be cut almost in half due to those read() operations.

thehesiod · 2018-02-21T00:08:02Z

ok cool, will update ASAP. Read should read the body of the response, normally the headers come first, followed by optionally reading the body (remember the body can be megabytes in length). So 204s will have no body and no subsegment, whereas something like downloading a 2MB file will.

thehesiod · 2018-02-21T00:08:56Z

another reason is that you may perform logic between getting the headers and the read and don't want to reflect that processing time in your read segment.

thehesiod · 2018-02-21T00:11:09Z

furthermore you may have multiple reads (it can get complicated quickly) :) If you have a fixed-size connection pool now imagine you may want even more segments:

wait for connector to be available from pool
connect time
wait for headers
3.1) if redirect, goto 2
(optional) read(s) depending on your chunk size and content length

haotianw465 · 2018-02-21T01:07:54Z

Agreed. It really depends on the use cases and having the SDK to be able to provide more insights on a single logical operation is always better. But this could be a good first step.

The main thing to keep in mind is the granularity on the segments structure. You can take a look at X-Ray Go SDK as an example: https://github.com/aws/aws-xray-sdk-go. In the screenshot shown at landing page you can see the DynamoDB call as a top level parent and its children subsegments provides insights on the timing on DNS lookup and SSL handshake and retries etc. But you only drill down to this level of details when you see a "slow DynamoDB operation".

As long as the top level segment capture provides necessary information so service back-end can aggregate them correctly, more children subsegments are always useful.

thehesiod · 2018-02-21T01:11:53Z

cool, will get to the changes soon hopefully, working with two broken arms at the moment due to VB accident :)

thehesiod · 2018-02-22T23:54:19Z

@haotianw465 so I take it back, after investigating this a bit more, I realized that each subsegment is a separate operation, note the following in the test:

    conn.request(method, path)  # sends headers
    resp = conn.getresponse()  # waits for response

this could then further have a resp.read()

so my current subsegment layout I believe is correct, where you have one subsegment per operation, which can be separated by user code.

haotianw465 · 2018-02-23T00:00:56Z

OK cool. In that case user will need to use @xray_recorder.capture() annotation to group httplib operations if necessary.

thehesiod · 2018-02-23T00:06:23Z

ya I just realized I can't create a segment because I won't know when it should end beyond when the instance gets GC'd ;)

haotianw465 · 2018-02-23T00:09:08Z

You will have to do the following per README.md about function capture:

@xray_recorder.capture('example.com')
def myfunc():
    conn.request(method, path)  # sends headers
    resp = conn.getresponse()
    resp.read()

Then you will have one subsegment named "example.com" which has three children representing those three operations, if this is what you expect.

thehesiod · 2018-02-23T00:10:52Z

cool, so anything left?

haotianw465 · 2018-02-23T00:12:37Z

No it looks good. I will approve and merge this PR.

haotianw465 · 2018-02-23T00:31:33Z

The httplib_test failed under Python2.7. Did you run it successfully on your side?

thehesiod · 2018-02-23T00:32:00Z

will try now

thehesiod · 2018-02-23T00:39:51Z

fixes in #23, let me know if you want in a clean branch, or squashing is ok on your side

haotianw465 · 2018-02-23T01:00:16Z

No worries. Already merged.

thehesiod · 2018-02-23T02:55:52Z

@haotianw465 looks like I forgot to update changelist.rst and add this as a new unrelease feature

haotianw465 · 2018-02-23T19:35:45Z

You can have a PR adding this feature to "unreleased".

The other thing I noticed is that since httplib_test monkey patches httplib during test run, it breaks other two tests: requests_test under py2 and pynamodb_test under py2/py3. This is because before adding httplib support, all the SDK patched libraries are independent. But httplib is a dependency of requests and pynamodb.

The fix is to have some unpatch logic on httplib patcher so that upon httplib_test finish it reverts the module in the runtime to the original one so other tests are not impacted. I'd really appreciate if you have extra time to this fix.

The broken tests also show us how you can deliberately patch httlib (even you don't directly use it) to get more insights for some libraries depend on it. But also this would be dangerous as per https://github.com/aws/aws-xray-sdk-python/blob/master/aws_xray_sdk/ext/httplib/patch.py#L57 this line throws an error if _prep_request is not called. If an upstream library (in this case requests) use its own prepare_request method, this will break. It would be better if xray_data = getattr(instance, _XRAY_PROP) also handles key doesn't exist so that all three methods patched in httplib can be called independently.

These are some useful information I think I could share with you since you mentioned on patching httplib for X-Ray usage.

thehesiod · 2018-02-23T20:13:24Z

ya, this module is missing unpatch logic from all the modules, will open a PR with fixes

thehesiod · 2018-02-23T20:47:35Z

ok opened #24, however I wasn't able to reproduce your second issue in python3, is this a python2 only issue, or specific requests issue?

haotianw465 · 2018-02-23T21:10:30Z

I saw the following on tox run at py2.7

.tox/py27/lib/python2.7/site-packages/requests/models.py:745: in generate
    for chunk in self.raw.stream(chunk_size, decode_content=True):
.tox/py27/lib/python2.7/site-packages/urllib3/response.py:436: in stream
    data = self.read(amt=amt, decode_content=decode_content)
.tox/py27/lib/python2.7/site-packages/urllib3/response.py:384: in read
    data = self._fp.read(amt)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

wrapped = <bound method HTTPResponse.read of <httplib.HTTPResponse instance at 0x10c1d0440>>, instance = <httplib.HTTPResponse instance at 0x10c1d0440>, args = (10240,)
kwargs = {}

    def _xray_traced_http_client_read(wrapped, instance, args, kwargs):
>       xray_data = getattr(instance, _XRAY_PROP)
E       AttributeError: HTTPResponse instance has no attribute '_xray_prop'

This might be a py2 specific issue and I don't have time to fully look at requests source code but this just reminds me of patched httplib requires the upstream library to call HTTPConnection._send_request so _XRAY_PROP can be set.

I'm not sure if this is the one you was unable to reproduce.

thehesiod · 2018-02-23T21:25:59Z

seems to be fixed with 5816eab in my new PR

haotianw465 · 2018-02-23T22:10:51Z

For unit tests unpatch should solve all the problems. My point is that the following patch code

def patch():
    wrapt.wrap_function_wrapper(
        httplib_client_module,
        'HTTPConnection._send_request',
        _send_request
    )

    wrapt.wrap_function_wrapper(
        httplib_client_module,
        'HTTPConnection.getresponse',
        _xray_traced_http_getresponse
    )

    wrapt.wrap_function_wrapper(
        httplib_client_module,
        'HTTPResponse.read',
        _xray_traced_http_client_read
    )

In order to capture the 2nd and 3rd operation correctly, 1st must be called, as 2nd and 3rd operation assumes an attribute _XRAY_PROP set by 1st. This is not an issue for using httplib itself since its standard usage is to call the 1st at the beginning. Just putting a note here, not a blocking issue.

thehesiod · 2018-02-23T23:07:12Z

ya I also tested this with requests and reproduced the issue you saw, and fixed with the above commit. From the impl of HTTPConnection I think it'll always get called or else you'd be rewriting major portions of the underlying class

initial httplib patch

8a3e2d7

fix unittests

c720c08

thehesiod changed the title ~~[WIP] initial httplib patch~~ httplib patch Feb 13, 2018

thehesiod changed the title ~~httplib patch~~ httplib support Feb 13, 2018

thehesiod added 2 commits February 14, 2018 10:35

fix warnings

e333e97

strip url in requests as well

53a5778

haotianw465 suggested changes Feb 16, 2018

View reviewed changes

haotianw465 added the enhancement label Feb 16, 2018

thehesiod added 2 commits February 15, 2018 17:06

updates based on review

bcec9d5

add some docs

ffd771c

refactor based on review

def2116

rename to getresponse to be clearer

d1e051d

haotianw465 approved these changes Feb 23, 2018

View reviewed changes

haotianw465 merged commit 0a9ce14 into aws:master Feb 23, 2018

haotianw465 mentioned this pull request May 30, 2018

patch_all should take a flag for an alternative patching strategy #63

Closed



		# ? is not a valid entity, and we don't want things after the ? for the segment name
		def _strip_url(url: str):

httplib support #19

httplib support #19

Uh oh!

Conversation

thehesiod commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thehesiod commented Feb 13, 2018

Uh oh!

thehesiod commented Feb 14, 2018

Uh oh!

haotianw465 left a comment

Choose a reason for hiding this comment

Uh oh!

haotianw465 Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haotianw465 Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haotianw465 Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haotianw465 Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thehesiod commented Feb 16, 2018

Uh oh!

haotianw465 commented Feb 16, 2018

Uh oh!

thehesiod commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haotianw465 commented Feb 19, 2018

Uh oh!

thehesiod commented Feb 21, 2018

Uh oh!

thehesiod commented Feb 21, 2018

Uh oh!

thehesiod commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

thehesiod commented Feb 13, 2018 •

edited

Loading

haotianw465 Feb 16, 2018 •

edited

Loading

haotianw465 Feb 16, 2018 •

edited

Loading

haotianw465 Feb 16, 2018 •

edited

Loading

haotianw465 Feb 16, 2018 •

edited

Loading

thehesiod commented Feb 16, 2018 •

edited

Loading

thehesiod commented Feb 21, 2018 •

edited

Loading

thehesiod commented Feb 22, 2018 •

edited

Loading