POC UVloop tracing, support for DNS traces and create Task traces #160
Conversation
This is the first PR that implements POC that gives to uvloop the capacity of emit traces that later can be collected by the upper layers. What does this commit implement? - DNS traces triggered by the calls to the `getaddrinfo` loop method. - Create task traces emited by the `create_task` loop method. - The enabler and disabler context manager called `tracing`. - The collector interface used to gather the trace events called `TracingCollector. - Capacity of install many nested collectors This POC assumes that the caller is the last responsible implementing a context that put together traces that belong to different tasks. Take a look to this snippet [1] Regarding the overhead of this POC, initial figures say that the performance footprint while there are no collectors installed is almost negligible [2]. (master) Cost per task 5.2499999999417925e-06 (traces) No collectors: Cost task 5.240000000048895e-06 (traces) default method: Cost per task 5.3500000000349245e-06 (traces) Collector method: Cost per task 5.409999999974389e-06 There is still some pending stuff realted to this PR that is not solved: - More robust tests for the DNS instrumentalization, right now params are not asserted. - Instrumentalize `_getnameinfo`. - More testing regarding `AddrInfoRequest`. But before I would like to gather your toughts with this MR, I will make the proper comments in some parts of the code that can delicated. [1] https://gist.github.com/pfreixes/5e9cccc4f5881de003243879edd5c470 [2] https://gist.github.com/pfreixes/18f0c678a9a018be0f376d4bef6d722a
| @@ -821,13 +824,17 @@ cdef class Loop: | |||
| except Exception as ex: | |||
| if not fut.cancelled(): | |||
| fut.set_exception(ex) | |||
| __send_trace(TRACE_DNS_REQUEST_END, None) | |||
There was a problem hiding this comment.
Some questions regarding this
- do we have to use the same trace for the happy path and for the none happy path?
- do we have to pass as a parm the exception?
I'm more inclined to think that decoupling both signals to two different things will help us to make the interface cleaner, perhaps having __send_trace(TRACE_DNS_REQUEST_END_EX, ex).
| else: | ||
| if not fut.cancelled(): | ||
| fut.set_result(data) | ||
| __send_trace(TRACE_DNS_REQUEST_END, data) |
There was a problem hiding this comment.
unfortunately, the _getaddrinfo can be called with the unpack enabled or disabled. with the current PR, the user won't be able to know explicitly wich format meets the argument.
I more inclined to think that we must give always the unpack version of the result that is the format returned by the public method getaddrinfo.
|
|
||
|
|
||
| cdef class TracingCollector: | ||
| cpdef dns_request_begin(self, host, port, family, type, proto, flags): |
There was a problem hiding this comment.
should we force an immutable version for all of the params?
|
@1st1 comments are welcomed. If at some point we believe that we can move on with this feature - that most likely will be composed of many small MR - I would create a feature branch to don't block master. |
|
This one can probably be closed now, right? |
|
Yeps, closing right now |
This is the first PR that implements POC that gives to uvloop the
capacity of emit traces that later can be collected by the upper layers.
What does this commit implement?
getaddrinfoloopmethod.
create_taskloop method.tracing.`TracingCollector.
This POC assumes that the caller is the last responsible for implementing
a context that put together with traces that belong to different tasks. Take
a look at this snippet [1]
Regarding the overhead of this POC, initial figures say that the performance
footprint, while there are no collectors installed, is almost negligible [2].
(master) Cost per task 5.2499999999417925e-06
(traces) No collectors: Cost task 5.240000000048895e-06
(traces) default method: Cost per task 5.3500000000349245e-06
(traces) Collector method: Cost per task 5.409999999974389e-06
There is still some pending stuff related to this PR that is not solved:
_getnameinfo.AddrInfoRequest.But before I would like to gather your thoughts with this MR, I will make the proper comments in some parts of the code that can delicate.
[1] https://gist.github.com/pfreixes/5e9cccc4f5881de003243879edd5c470
[2] https://gist.github.com/pfreixes/18f0c678a9a018be0f376d4bef6d722a