httplib: add patching for httplib and http.lib by brettlangdon · Pull Request #137 · DataDog/dd-trace-py

brettlangdon · 2016-12-19T00:01:02Z

This PR adds in tracing support for the base httplib.HTTPConnection (Py2) and http.lib.HTTPConnection (Py3) classes.

Patching these classes allows us expand our support for tracing external HTTP requests.

This tracing is setup similar to the tracing for requests. There is no service being set, and we are capturing the basic tags of http.method, http.status_code, and http.url.

from ddtrace.contrib.httplib import patch
patch()

# Python 2
import urllib
resp = urllib.urlopen('http://www.datadog.com/')

import urllib2
resp = urllib2.urlopen('http://www.datadog.com/')

# Python 3
import urllib.request
resp = urllib.request.urlopen('http://www.datadog.com/')

import urllib.request
resp = urllib.request.urlopen('http://www.datadog.com/')

brettlangdon · 2016-12-19T18:31:08Z

Reading through source a little more, it might be better to patch urllib.urlopen, httplib.HTTPConnection (http.client.HTTPConnection for PY3), and httplib.HTTPSConnection (http.client.HTTPSConnection for PY3).

urllib2.urlopen (PY2) / urllib.urlopen (PY3) utilize the httplib/http.client classes for making HTTP connections. This is the same case for urllib3, it uses httplib/http.client.

brettlangdon · 2016-12-22T15:31:24Z

If I were to have this cover urllib/httplib (PY2) and http.client (PY3), urllib seems like a weird name for the contrib.

Should I break this up into two? urllib, and httplib? or should I just rename to httplib, and let the one off urllib.urlopen (PY2) be the odd one out?

brettlangdon · 2017-02-15T21:23:31Z

I have decided to refactor this code as my previous comments have suggested, tracing urllib (Py2), httplib (Py2) and http.client (Py3) while also renaming the module from urllib to httplib.

brettlangdon · 2017-02-16T00:42:04Z

This PR is updated as previously mentioned, now called httplib and traces httplib.HTTPConnection (Py2) and http.lib.HTTPConnection (Py3).

I have updated the title and description to match these changes.

brettlangdon · 2017-03-01T22:09:57Z

@palazzem I just updated this to handle the "tracing the tracer" issue we noticed. Not 100% sure the way I am handling that case is ideal, would love your feedback (see the changes in api.py).

Other than that, this should be good to review.

palazzem · 2017-03-02T08:44:47Z

thank you @brettlangdon ! will keep you updated! 👍

LotharSee · 2017-05-03T08:28:47Z

We are internally working on a specification for HTTP spans in general (what the service, the resource should be, common metadata fields, ...).
Once we will have that, we should be able to fully review and merge this one!

brettlangdon · 2017-05-03T12:26:28Z

\0/

Let me know if you need me to make any changes.

palazzem

That's an impressive contribution! Thanks a lot especially because it could be really useful!

Basically I've left some nitpicks around and some minor changes that may improve the code. What I think we should handle during this pass is:

updating the patch so that we use the Pin object in our wrappers
instead of adding / removing the Pin or the datadog_tracer, we may check if the request is going through our agent. I think this is a bit better because it's not changing our core API

After that I think we can do another quick check and see if everything is OK so we can merge that one to master.

Again, thanks for your code and for trying this PR!

palazzem · 2017-05-26T10:50:39Z

I think you can put these imports in our compat.py module like that one: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/compat.py#L8

palazzem · 2017-05-26T10:53:34Z

Because the patched methods are the same and only the module name is different, I think you can use the one available in the compat: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/compat.py#L20 right?

palazzem · 2017-05-26T11:01:48Z

nitpick: I think we can update the patch() with something similar to https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/tornado/patch.py#L4:

from wrapt import wrap_function_wrapper as _w from ...util import unwrap as _u # to patch _w('httplib', 'HTTPConnection.getresponse', _wrap_getresponse) _w('httplib', 'HTTPConnection.putrequest', _wrap_putrequest) # to unpatch u(httplib.HTTPConnection, 'getresponse') u(httplib.HTTPConnection, 'putrequest')

it should work right? Also, I think it's better to add a check in both patch() and unpatch() so that we're sure calling the patch twice doesn't instrument the module again: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/tornado/patch.py#L16-L19

palazzem · 2017-05-26T11:04:56Z

while I'm OK with these logs, I think it's better to use log.debug() so that if there is an exception in this code-block for a high traffic endpoint, application logs are not flooded by this log line. What do you think?

palazzem · 2017-05-26T11:06:11Z

I think we can remove this section and keep only the Usage section.

palazzem · 2017-05-26T11:33:23Z

instead of using this manual approach, we may replace the whole getattr and setattr using the Pin object. Basically it's doing the same but with the Pin we're sure the patching system is the same among all integrations. You can check an example here:

to check if the pin is available: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/aiohttp/template.py#L13-L15

to set the pin: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/aiohttp/patch.py#L29

Later you can pin.tracer.trace().

palazzem · 2017-05-26T11:41:08Z

We may use something like _datadog_span so it's not "visible" in the sense of "please don't touch this attribute".

palazzem · 2017-05-26T11:44:40Z

Unfortunately, I think that this exact snippet will not work because in this patch we don't explicitly set a service name, so the agent will drop all the traffic generated by a plain script that contains this example. Here we may need to update the code and the example to use our Pin object (more details in other comments) to set a service.

palazzem · 2017-05-26T11:51:15Z

if you're using the Pin object, this will become _datadog_pin (more details in another comment). Anyway can't we avoid this change and do a check before doing the request? In the Ruby client we have an implementation at integration-level that basically checks if the request is going through our agent (hostname and port are taken from the tracer itself). Ref: https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/contrib/http/patcher.rb#L17-L36

Later on here we can extend the check so that we trace only if there is a Pin, it's enabled and it's not a request to our agent. I think it should be easy to add / update a test for this case.

What do you think?

palazzem · 2017-05-26T11:59:58Z

not sure if what I'm saying makes sense or not, but these tests shouldn't be the same for Python 2 and Python 3? Or there are functionalities in one of both that I'm missing? If tests are the same, we may provide a Python mixin that includes only the tests (py2 and py3 compatible), so that only the setup() etc, are different.

Do you think it's a viable approach?

brettlangdon · 2017-05-26T12:24:35Z

Awesome! Thanks for the feedback. Quite a bit here but it all sounds like good changes to me.

…

On May 26, 2017 8:05 AM, "Emanuele Palazzetti" ***@***.***> wrote: ***@***.**** commented on this pull request. That's an impressive contribution! Thanks a lot especially because it could be really useful! Basically I've left some nitpicks around and some minor changes that may improve the code. What I think we should handle during this pass is: - updating the patch so that we use the Pin object in our wrappers - instead of adding / removing the Pin or the datadog_tracer, we may check if the request is going through our agent. I think this is a bit better because it's not changing our core API After that I think we can do another quick check and see if everything is OK so we can merge that one to master. Again, thanks for your code and for trying this PR! ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > @@ -0,0 +1,106 @@ +# Standard library +import logging + +# Third party +import wrapt + +# Project +import ddtrace +from ...compat import PY2 +from ...ext import http as ext_http + +if PY2: I think you can put these imports in our compat.py module like that one: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/compat.py#L8 ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + method, path = args[:2] + scheme = 'https' if isinstance(instance, HTTPSConnection) else 'http' + port = ':{port}'.format(port=instance.port) + if (scheme == 'http' and instance.port == 80) or (scheme == 'https' and instance.port == 443): + port = '' + url = '{scheme}://{host}{port}{path}'.format(scheme=scheme, host=instance.host, port=port, path=path) + span.set_tag(ext_http.URL, url) + span.set_tag(ext_http.METHOD, method) + except Exception: + log.warning('error applying request tags', exc_info=True) + + return func(*args, **kwargs) + + +wrap_modules = [] +if PY2: Because the patched methods are the same and only the module name is different, I think you can use the one available in the compat: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/compat.py#L20 right? ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + import httplib + wrap_modules = [ + (httplib.HTTPConnection, 'getresponse', _wrap_getresponse), + (httplib.HTTPConnection, 'putrequest', _wrap_putrequest), + ] +else: + import http.client + wrap_modules = [ + (http.client.HTTPConnection, 'getresponse', _wrap_getresponse), + (http.client.HTTPConnection, 'putrequest', _wrap_putrequest), + ] + + +def patch(): + """ patch the built-in urllib/httplib/httplib.client methods for tracing""" + for cls, func_name, wrapper in wrap_modules: nitpick: I think we can update the patch() with something similar to https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/ contrib/tornado/patch.py#L4: from wrapt import wrap_function_wrapper as _wfrom ...util import unwrap as _u # to patch _w('httplib', 'HTTPConnection.getresponse', _wrap_getresponse) _w('httplib', 'HTTPConnection.putrequest', _wrap_putrequest) # to unpatch u(httplib.HTTPConnection, 'getresponse') u(httplib.HTTPConnection, 'putrequest') it should work right? Also, I think it's better to add a check in both patch() and unpatch() so that we're sure calling the patch twice doesn't instrument the module again: https://github.com/DataDog/dd- trace-py/blob/master/ddtrace/contrib/tornado/patch.py#L16-L19 ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + return resp + finally: + try: + # Get the span attached to this instance, if available + span = getattr(instance, 'datadog_span', None) + if not span: + return + + if resp: + span.set_tag(ext_http.STATUS_CODE, resp.status) + span.error = int(500 <= resp.status) + + span.finish() + delattr(instance, 'datadog_span') + except Exception: + log.warning('error applying request tags', exc_info=True) while I'm OK with these logs, I think it's better to use log.debug() so that if there is an exception in this code-block for a high traffic endpoint, application logs are not flooded by this log line. What do you think? ------------------------------ In ddtrace/contrib/httplib/__init__.py <#137 (comment)>: > @@ -0,0 +1,26 @@ +""" +Patch the built-in httplib/http.client libraries to trace all HTTP calls. + +Patched modules: I think we can remove this section and keep only the Usage section. ------------------------------ In ddtrace/contrib/httplib/__init__.py <#137 (comment)>: > @@ -0,0 +1,26 @@ +""" +Patch the built-in httplib/http.client libraries to trace all HTTP calls. + +Patched modules: + + - httplib.HTTPConnection (Python 2) + - httplib.client.HTTPConnection (Python 3) + + +Usage:: + + # Patch all supported modules/functions + from ddtrace.contrib.httplib import patch I think we can suggest: from ddtrace import patch patch(httlib=True) also you may want to add that value in the monkey.py module: https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/monkey.py#L28 so that it will be available even with patch_all() Anyway, I'd suggest to keep the default activation to False like we're doing for the request library. ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + span.finish() + delattr(instance, 'datadog_span') + except Exception: + log.warning('error applying request tags', exc_info=True) + + +def _wrap_putrequest(func, instance, args, kwargs): + # Use any attached tracer if available, otherwise use the global tracer + tracer = getattr(instance, 'datadog_tracer', ddtrace.tracer) + + # DEV: We explicitly set the instance tracer to `None` when using HTTPConnection internally + if not tracer or not tracer.enabled: + return func(*args, **kwargs) + + try: + trace_name = 'httplib.request' if PY2 else 'http.client.request' nit: we may call it span_name because it's more related to this span rather than the whole trace. Also you may move the if-else check outside the loop so it's evaluated once. ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + if not span: + return + + if resp: + span.set_tag(ext_http.STATUS_CODE, resp.status) + span.error = int(500 <= resp.status) + + span.finish() + delattr(instance, 'datadog_span') + except Exception: + log.warning('error applying request tags', exc_info=True) + + +def _wrap_putrequest(func, instance, args, kwargs): + # Use any attached tracer if available, otherwise use the global tracer + tracer = getattr(instance, 'datadog_tracer', ddtrace.tracer) instead of using this manual approach, we may replace the whole getattr and setattr using the Pin object. Basically it's doing the same but with the Pin we're sure the patching system is the same among all integrations. You can check an example here: - to check if the pin is available: https://github.com/DataDog/dd- trace-py/blob/master/ddtrace/contrib/aiohttp/template.py#L13-L15 <https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/aiohttp/template.py#L13-L15> - to set the pin: https://github.com/DataDog/dd- trace-py/blob/master/ddtrace/contrib/aiohttp/patch.py#L29 <https://github.com/DataDog/dd-trace-py/blob/master/ddtrace/contrib/aiohttp/patch.py#L29> Later you can pin.tracer.trace(). ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + + +def _wrap_putrequest(func, instance, args, kwargs): + # Use any attached tracer if available, otherwise use the global tracer + tracer = getattr(instance, 'datadog_tracer', ddtrace.tracer) + + # DEV: We explicitly set the instance tracer to `None` when using HTTPConnection internally + if not tracer or not tracer.enabled: + return func(*args, **kwargs) + + try: + trace_name = 'httplib.request' if PY2 else 'http.client.request' + + # Create a new span and attach to this instance (so we can retrieve/update/close later on the response) + span = tracer.trace(trace_name, span_type=ext_http.TYPE) + setattr(instance, 'datadog_span', span) We may use something like _datadog_span so it's not "visible" in the sense of "please don't touch this attribute". ------------------------------ In ddtrace/contrib/httplib/__init__.py <#137 (comment)>: > + +Patched modules: + + - httplib.HTTPConnection (Python 2) + - httplib.client.HTTPConnection (Python 3) + + +Usage:: + + # Patch all supported modules/functions + from ddtrace.contrib.httplib import patch + patch() + + # Python 2 + import urllib + resp = urllib.urlopen('http://www.datadog.com/') Unfortunately, I think that this exact snippet will not work because in this patch we don't explicitly set a service name, so the agent will drop all the traffic generated by a plain script that contains this example. Here we may need to update the code and the example to use our Pin object (more details in other comments) to set a service. ------------------------------ In ddtrace/api.py <#137 (comment)>: > @@ -75,5 +75,8 @@ def send_services(self, services): def _put(self, endpoint, data): conn = httplib.HTTPConnection(self.hostname, self.port) + # Explicitly set/override tracer to `None `to disable `httplib` tracing + # DEV: Otherwise we'll get into an infinite loop of tracing the tracer + setattr(conn, "datadog_tracer", None) if you're using the Pin object, this will become _datadog_pin (more details in another comment). Anyway can't we avoid this change and do a check before doing the request? In the Ruby client we have an implementation at integration-level that basically checks if the request is going through our agent (hostname and port are taken from the tracer itself). Ref: https://github.com/DataDog/dd-trace-rb/blob/master/lib/ ddtrace/contrib/http/patcher.rb#L17-L36 Later on here <https://github.com/DataDog/dd-trace-py/pull/137/files#diff-b7b923cbf57be67da4667519da874510R55> we can extend the check so that we trace only if there is a Pin, it's enabled and it's not a request to our agent. I think it should be easy to add / update a test for this case. What do you think? ------------------------------ In tests/contrib/httplib/test_httplib.py <#137 (comment)>: > @@ -0,0 +1,590 @@ +# Standard library +import contextlib +import sys +import unittest + +# Third party +import wrapt + +# Project +from ddtrace.compat import PY2 +from ddtrace.contrib.httplib import patch, unpatch +from ...test_tracer import get_dummy_tracer + + +if PY2: not sure if what I'm saying makes sense or not, but these tests shouldn't be the same for Python 2 and Python 3? Or there are functionalities in one of both that I'm missing? If tests are the same, we may provide a Python mixin that includes only the tests (py2 and py3 compatible), so that only the setup() etc, are different. Do you think it's a viable approach? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#137 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQloReARyIJ8BMY_IFnmqyW8c9d1J3Cks5r9sAhgaJpZM4LQRkb> .

palazzem · 2017-05-26T12:26:15Z

Cool @brettlangdon! Let us know if we can help you someway to bring that PR ready to be merged! Thanks again!

brettlangdon · 2017-05-26T16:03:16Z

@palazzem I have updated based on your comments. Let me know if I missed or misinterpreted anything.

palazzem

In the overall it's great! The current state is:

the integration doesn't create new services when called
the used resource it's always the same (httplib.request or http.client.request )

keeping the cardinality of resources and services low. I think it's a good start until we have a specification for HTTP spans like @LotharSee has suggested.

Keeping the PR on hold for a latest review (we're shipping another minor release in the meantime), but for me it's good to go! Will put the 👍 after discussing about the current state of HTTP spans specification.

(Note: just updated the code to let tests pass)

palazzem · 2017-05-29T15:26:52Z

that's great!

palazzem · 2017-05-29T16:14:07Z

Need to think about that, because while it's easy to attach a global PIN to the httplib module, it may create issues if you want to create two new services from two different endpoints. Maybe it's better to have one PIN for instance but it may make more difficult to update PIN settings.

For now no changes are required. I left this comment just to keep it in mind before the merge.

Yeah, I think I borrowed this pattern/idea from the tornado patching, However, it should be easy enough to also patch the constructor for HTTPConnection and attach the pin there?

Yes actually for tornado it was "quite tough" to do some "PIN-patching". If it's not too complicated and if you think that there are real use cases (e.g. application A that sends requests to service B and C and wants to create service-A and service-B in the dashboard), maybe we can attach the pin in the constructor wrapper so we're sure that overriding the PIN object will not globally change the state of httplib.

This is just a nitpick and feel free to give your opinion on that. Thanks!

I am 50/50 on it, I personally don't think I would update the Pin/service info for any of this patching. It always felt more like an "inner workings" tracing rather than "service" tracing to me. However, I can see the argument to attach service info to the Pin, so ¯_(ツ)_/¯

Happy to adjust the PR if we want, should be more than easy enough addition to make if we think it'll be more useful/versatile for other users.

Yea probably if we move the code in that way, it could be generic enough to handle both cases. So let's say: "the default behavior is seeing it only as an inner workings tracing; for advanced usage you can set the PIN service / any other field before using the connection".

If it sounds good to you and it doesn't increase the code complexity, I think that change could help for the future.

brettlangdon · 2017-05-29T16:20:44Z

\0/

…

On May 29, 2017 12:19 PM, "Emanuele Palazzetti" ***@***.***> wrote: ***@***.**** commented on this pull request. In the overall it's great! The current state is: - the integration doesn't create new services when called - the used resource it's always the same (httplib.request or http.client.request) keeping the cardinality of resources and services low. I think it's a good start until we have a specification for HTTP spans like @LotharSee <https://github.com/lotharsee> has suggested. Keeping the PR on hold for a latest review (we're shipping another minor release in the meantime), but for me it's good to go! (Note: just updated the code to let tests pass) ------------------------------ In tests/contrib/httplib/test_httplib.py <#137 (comment)>: > + self.assertIsInstance(httplib.HTTPConnection.putrequest, wrapt.BoundFunctionWrapper) + self.assertIsInstance(httplib.HTTPConnection.getresponse, wrapt.BoundFunctionWrapper) + + def test_unpatch(self): + """ + When unpatching httplib + we restore the correct module/methods + """ + original_putrequest = httplib.HTTPConnection.putrequest.__wrapped__ + original_getresponse = httplib.HTTPConnection.getresponse.__wrapped__ + unpatch() + + self.assertEqual(httplib.HTTPConnection.putrequest, original_putrequest) + self.assertEqual(httplib.HTTPConnection.getresponse, original_getresponse) + + def test_should_skip_request(self): that's great! ------------------------------ In ddtrace/contrib/httplib/patch.py <#137 (comment)>: > + api = pin.tracer.writer.api + return request.host == api.hostname and request.port == api.port + + +def patch(): + """ patch the built-in urllib/httplib/httplib.client methods for tracing""" + if getattr(httplib, '__datadog_patch', False): + return + setattr(httplib, '__datadog_patch', True) + + # Patch the desired methods + setattr(httplib.HTTPConnection, 'getresponse', + wrapt.FunctionWrapper(httplib.HTTPConnection.getresponse, _wrap_getresponse)) + setattr(httplib.HTTPConnection, 'putrequest', + wrapt.FunctionWrapper(httplib.HTTPConnection.putrequest, _wrap_putrequest)) + Pin(app='httplib', service=None, app_type=ext_http.TYPE).onto(httplib) Need to think about that, because while it's easy to attach a global PIN to the httplib module, it may create issues if you want to create two new services from two different endpoints. Maybe it's better to have one PIN for instance but it may make more difficult to update PIN settings. For now no changes are required. I left this comment just to keep it in mind before the merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#137 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQlodXZRED-MhKxizjQCAEeHIZl5npvks5r-vASgaJpZM4LQRkb> .

brettlangdon · 2017-05-30T12:49:40Z

@palazzem I am having trouble updating these tests here to support attaching a Pin to each httplib.HTTPConnection instance. The problem is I don't seem to have direct access to the HTTPConnection instance before the request is made.

Any thoughts/suggestions here?

For the other tests I am doing:

request = httplib.HTTPConnection(*args, **kwargs) Pin.override(request, tracer=self.tracer)

Sure! feel free to push your change with these tests that don't pass, I will take a look as soon as I can so we can complete that PR 💯

brettlangdon · 2017-05-30T15:25:48Z

@palazzem I updated the code here, has the new patching per instance, and the failing tests for urllib/urllib2

Also, I snuck in some HTTPS tests as well, realized we didn't have them.

palazzem · 2017-05-30T15:36:30Z

Great @brettlangdon thank you!

…cer with a dummy one

palazzem · 2017-06-05T15:53:31Z

@brettlangdon I've pushed a context manager (6a28ed2 on top of your PR) for those test cases that use functions such as urlopen(). It replaces the global tracer with a new one before making the call and then the original one is restored after.

Another solution may be to Mock some inner functions, but I prefer to not use Mock in these tests and to go with a temporary replacement of the global tracer. In that way, we're sure that if we make some changes in the future and the global tracer is not used anymore, those tests will stop working.

Do you think it's fair enough? In that case you can incorporate this change in your PR! Then a final review should be only paperwork 👍

brettlangdon · 2017-06-05T15:57:21Z

@palazzem that solution looks great. Good thinking!

Do you want me to merge that change into my PR and update or you should be able to push on top of this PR (I think)?

brettlangdon · 2017-06-12T18:44:19Z

\0/

…

On Jun 12, 2017 2:42 PM, "Emanuele Palazzetti" ***@***.***> wrote: Merged #137 <#137>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#137 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQloc56Y1i_aAGmM1eMmDNRbnx77oHMks5sDYaFgaJpZM4LQRkb> .

…llocator (#17664) ## Description This PR fixes a segmentation fault in the memory allocation profiler that occurs when a hook call races with `memalloc` start/stop operations. The issue arises from concurrent access to the saved allocator struct, which could be partially written while being read, resulting in`NULL` function pointers being dereferenced. The key indicator in that case is that `#1 0x0000000000000000` frame -- we are trying to execute a null function pointer. ```` Error UnixSignal: Process terminated with SEGV_MAPERR (SIGSEGV) #0 0x00007ff3c303a8d4 #1 0x0000000000000000 memalloc_alloc (/go/src/github.com/DataDog/apm-reliability/dd-trace-py/ddtrace/profiling/collector/_memalloc.cpp:68) #2 0x00007ff39dcb3b20 memalloc_alloc (/go/src/github.com/DataDog/apm-reliability/dd-trace-py/ddtrace/profiling/collector/_memalloc.cpp:68) #3 0x00007ff39dcb3b20 memalloc_malloc(void*, unsigned long) (/go/src/github.com/DataDog/apm-reliability/dd-trace-py/ddtrace/profiling/collector/_memalloc.cpp:80) #4 0x00007ff3c3087e1b PyUnicode_New #5 0x00007ff3c30889f4 #6 0x00007ff3c3170c84 #7 0x00007ff3c316b931 #8 0x00007ff3c31aaac8 #9 0x00007ff3c31033ac #10 0x00007ff3c310e2a6 PyObject_CallMethodObjArgs #11 0x00007ff3c310e46d #12 0x00007ff3c31a96c2 #13 0x00007ff3c3102fd7 PyObject_Vectorcall #14 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #15 0x00007ff3c323c094 #16 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #17 0x00007ff3c323c094 #18 0x00007ff3c30e997d PyObject_CallOneArg #19 0x00007ff3c306a480 _PyObject_GenericGetAttrWithDict #20 0x00007ff3c30c620d PyObject_GetAttr #21 0x00007ff3c32309e7 _PyEval_EvalFrameDefault #22 0x00007ff3c323c094 #23 0x00007ff3c312880e #24 0x00007ff3c30e917c _PyObject_MakeTpCall #25 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #26 0x00007ff3c323c094 #27 0x00007ff3c312880e #28 0x00007ff3c30e917c _PyObject_MakeTpCall #29 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #30 0x00007ff3c323c094 #31 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #32 0x00007ff3c323c094 #33 0x00007ff3c317d0fd #34 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #35 0x00007ff3c323c094 #36 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #37 0x00007ff3c323c094 #38 0x00007ff3c317d1b5 #39 0x00007ff3c3102fd7 PyObject_Vectorcall #40 0x00007ff3c3232f4a _PyEval_EvalFrameDefault #41 0x00007ff3c3240da5 #42 0x00007ff3c324112d #43 0x00007ff3c3233be1 _PyEval_EvalFrameDefault #44 0x00007ff3c323c094 #45 0x00007ff3c317d1b5 #46 0x00007ff3c3102fd7 PyObject_Vectorcall #47 0x00007ff3c3232f4a _PyEval_EvalFrameDefault #48 0x00007ff3c323c094 #49 0x00007ff3c31033ac #50 0x00007ff3c310358d PyObject_CallFunctionObjArgs #51 0x00007ff3bf7eb91d WraptBoundFunctionWrapper_call (/project/src/wrapt/_wrappers.c:3750) #52 0x00007ff3c3104055 _PyObject_Call #53 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #54 0x00007ff3c323c094 #55 0x00007ff3c317d23c #56 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #57 0x00007ff3c323c094 #58 0x00007ff3c310416f _PyObject_Call #59 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #60 0x00007ff3c3240da5 #61 0x00007ff3c324112d #62 0x00007ff3c3233be1 _PyEval_EvalFrameDefault #63 0x00007ff3c323c094 #64 0x00007ff3c317d1b5 #65 0x00007ff3c3102fd7 PyObject_Vectorcall #66 0x00007ff3c3232f4a _PyEval_EvalFrameDefault #67 0x00007ff3c323c094 #68 0x00007ff3c317d1b5 #69 0x00007ff3c310416f _PyObject_Call #70 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #71 0x00007ff3c323c094 #72 0x00007ff3c317d1b5 #73 0x00007ff3c310416f _PyObject_Call #74 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #75 0x00007ff3c323c094 #76 0x00007ff3c31033ac #77 0x00007ff3c310358d PyObject_CallFunctionObjArgs #78 0x00007ff3bf7eb91d WraptBoundFunctionWrapper_call (/project/src/wrapt/_wrappers.c:3750) #79 0x00007ff3c30e917c _PyObject_MakeTpCall #80 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #81 0x00007ff3c323c094 #82 0x00007ff3c317d518 #83 0x00007ff3c3155963 #84 0x00007ff3c315393d #85 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #86 0x00007ff3c323c094 #87 0x00007ff3c317d0fd #88 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #89 0x00007ff3c323c094 #90 0x00007ff3c317d0fd #91 0x00007ff3c317d518 #92 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #93 0x00007ff3c323c094 #94 0x00007ff3c30e9371 _PyObject_FastCallDictTstate #95 0x00007ff3c30e958d _PyObject_Call_Prepend #96 0x00007ff3c3109150 #97 0x00007ff3c3104055 _PyObject_Call #98 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #99 0x00007ff3c323c094 #100 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #101 0x00007ff3c30e958d _PyObject_Call_Prepend #102 0x00007ff3c3109150 #103 0x00007ff3c30e917c _PyObject_MakeTpCall #104 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #105 0x00007ff3c323c094 #106 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #107 0x00007ff3c30e958d _PyObject_Call_Prepend #108 0x00007ff3c3109150 #109 0x00007ff3c30e917c _PyObject_MakeTpCall #110 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #111 0x00007ff3c323c094 #112 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #113 0x00007ff3c30e958d _PyObject_Call_Prepend #114 0x00007ff3c3109150 #115 0x00007ff3c30e917c _PyObject_MakeTpCall #116 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #117 0x00007ff3c323c094 #118 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #119 0x00007ff3c30e958d _PyObject_Call_Prepend #120 0x00007ff3c3109150 #121 0x00007ff3c30e917c _PyObject_MakeTpCall #122 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #123 0x00007ff3c323c094 #124 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #125 0x00007ff3c30e958d _PyObject_Call_Prepend #126 0x00007ff3c3109150 #127 0x00007ff3c30e917c _PyObject_MakeTpCall #128 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #129 0x00007ff3c323c094 #130 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #131 0x00007ff3c30e958d _PyObject_Call_Prepend #132 0x00007ff3c3109150 #133 0x00007ff3c30e917c _PyObject_MakeTpCall #134 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #135 0x00007ff3c323c094 #136 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #137 0x00007ff3c30e958d _PyObject_Call_Prepend #138 0x00007ff3c3109150 #139 0x00007ff3c30e917c _PyObject_MakeTpCall #140 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #141 0x00007ff3c323c094 #142 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #143 0x00007ff3c30e958d _PyObject_Call_Prepend #144 0x00007ff3c3109150 #145 0x00007ff3c30e917c _PyObject_MakeTpCall #146 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #147 0x00007ff3c323c094 #148 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #149 0x00007ff3c30e958d _PyObject_Call_Prepend #150 0x00007ff3c3109150 #151 0x00007ff3c30e917c _PyObject_MakeTpCall #152 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #153 0x00007ff3c323c094 #154 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #155 0x00007ff3c30e958d _PyObject_Call_Prepend #156 0x00007ff3c3109150 #157 0x00007ff3c30e917c _PyObject_MakeTpCall #158 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #159 0x00007ff3c323c094 #160 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #161 0x00007ff3c30e958d _PyObject_Call_Prepend #162 0x00007ff3c3109150 #163 0x00007ff3c30e917c _PyObject_MakeTpCall #164 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #165 0x00007ff3c323c094 #166 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #167 0x00007ff3c30e958d _PyObject_Call_Prepend #168 0x00007ff3c3109150 #169 0x00007ff3c30e917c _PyObject_MakeTpCall #170 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #171 0x00007ff3c323c094 #172 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #173 0x00007ff3c30e958d _PyObject_Call_Prepend #174 0x00007ff3c3109150 #175 0x00007ff3c30e917c _PyObject_MakeTpCall #176 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #177 0x00007ff3c323c094 #178 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #179 0x00007ff3c30e958d _PyObject_Call_Prepend #180 0x00007ff3c3109150 #181 0x00007ff3c30e917c _PyObject_MakeTpCall #182 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #183 0x00007ff3c323c094 #184 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #185 0x00007ff3c30e958d _PyObject_Call_Prepend #186 0x00007ff3c3109150 #187 0x00007ff3c30e917c _PyObject_MakeTpCall #188 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #189 0x00007ff3c323c094 #190 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #191 0x00007ff3c323c094 #192 0x00007ff3c30e92f1 _PyObject_FastCallDictTstate #193 0x00007ff3c30e958d _PyObject_Call_Prepend #194 0x00007ff3c3109150 #195 0x00007ff3c30e917c _PyObject_MakeTpCall #196 0x00007ff3c32335a2 _PyEval_EvalFrameDefault #197 0x00007ff3c323c094 #198 0x00007ff3c317d0fd #199 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #200 0x00007ff3c323c094 #201 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #202 0x00007ff3c323c094 #203 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #204 0x00007ff3c323c094 #205 0x00007ff3c3233dd3 _PyEval_EvalFrameDefault #206 0x00007ff3c323c094 #207 0x00007ff3c317d23c #208 0x00007ff3c31a7ec5 #209 0x00007ff3c301ac77 #210 0x00007ff3c357c573 ```` The fix implements two key changes. 1. **Hook functions (`memalloc_alloc`, `memalloc_realloc`)**: Snapshot the allocator struct locally before use and guard indirect function calls with `NULL` checks. This prevents crashes if a partially-written struct is observed during a start/stop race. 2. **Start/stop operations (`memalloc_start`, `memalloc_stop`)**: Use local variables and single assignments when publishing the allocator struct to `global_memalloc_ctx.pymem_allocator_obj`. This ensures concurrent hook calls observe either the old or new struct, never a partially-written intermediate state. The real root cause is that `PyMem_GetAllocator` is not documented as atomic, and the struct could be read field-by-field while being written to concurrently. By using local copies and single assignments, we ensure atomicity at the C level and prevent observation of inconsistent state. Co-authored-by: thomas.kowalski <thomas.kowalski@datadoghq.com>

clutchski added this to the 0.6.0 milestone Feb 14, 2017

brettlangdon force-pushed the brettlangdon/patch.urllib.sqwished branch from 8a730d9 to ef8b055 Compare February 16, 2017 00:37

brettlangdon changed the title ~~urllib: add patching for urllib.urlopen, urllib2.urlopen, and urllib.request.urlopen~~ httplib: add patching for httplib and http.lib Feb 16, 2017

palazzem self-requested a review February 20, 2017 12:46

palazzem added integrations wip and removed wip labels Feb 20, 2017

palazzem removed this from the 0.6.0 milestone Mar 1, 2017

brettlangdon force-pushed the brettlangdon/patch.urllib.sqwished branch from ef8b055 to 5db0778 Compare March 1, 2017 22:08

palazzem self-assigned this Mar 2, 2017

brettlangdon force-pushed the brettlangdon/patch.urllib.sqwished branch from 5db0778 to 115d131 Compare March 7, 2017 14:54

palazzem modified the milestone: 0.7.0 Mar 9, 2017

palazzem modified the milestones: 0.7.0, 0.8.0 Mar 29, 2017

palazzem modified the milestones: 0.8.0, 0.9.0 Apr 10, 2017

palazzem reviewed May 26, 2017

View reviewed changes

brettlangdon force-pushed the brettlangdon/patch.urllib.sqwished branch from 115d131 to 8ea2617 Compare May 26, 2017 16:02

palazzem reviewed May 29, 2017

View reviewed changes

brettlangdon commented May 30, 2017

View reviewed changes

httplib: add patching for httplib and http.lib

93a79af

brettlangdon force-pushed the brettlangdon/patch.urllib.sqwished branch from 34bf926 to 93a79af Compare May 30, 2017 15:25

[httplib] provide a override context manager to switch the global tra…

6a28ed2

…cer with a dummy one

palazzem merged commit 6a28ed2 into DataDog:master Jun 12, 2017

palazzem pushed a commit that referenced this pull request Jun 12, 2017

Merge pull request #137 from brettlangdon/patch.urllib

5b684b6

palazzem mentioned this pull request Jun 12, 2017

[enhancement] aiohttp client tracing #285

Closed

Conversation

brettlangdon commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettlangdon commented Dec 19, 2016

Uh oh!

brettlangdon commented Dec 22, 2016

Uh oh!

brettlangdon commented Feb 15, 2017

Uh oh!

brettlangdon commented Feb 16, 2017

Uh oh!

brettlangdon commented Mar 1, 2017

Uh oh!

palazzem commented Mar 2, 2017

Uh oh!

LotharSee commented May 3, 2017

Uh oh!

brettlangdon commented May 3, 2017

Uh oh!

palazzem left a comment

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!

brettlangdon commented May 26, 2017 via email

Uh oh!

palazzem commented May 26, 2017

Uh oh!

brettlangdon commented May 26, 2017

Uh oh!

palazzem left a comment • 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!

palazzem May 30, 2017 • 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!

brettlangdon commented May 29, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palazzem May 30, 2017 • 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!

brettlangdon commented May 30, 2017

Uh oh!

palazzem commented May 30, 2017

Uh oh!

palazzem commented Jun 5, 2017

brettlangdon commented Dec 19, 2016 •

edited

Loading

palazzem left a comment •

edited

Loading

palazzem May 30, 2017 •

edited

Loading

palazzem May 30, 2017 •

edited

Loading