Don't trace if it's a http request made by the exporter#289
Don't trace if it's a http request made by the exporter#289c24t merged 25 commits intocensus-instrumentation:masterfrom
Conversation
| _tracer = execution_context.get_opencensus_tracer() | ||
| try: # pragma: NO COVER | ||
| # Don't trace if it's a http request made by the exporter | ||
| if self._dns_host == _tracer.exporter.host_name: |
There was a problem hiding this comment.
Probably we can make the mechanism more generic and allow httplib to accept a list of paths + hosts to not be traced. I think this is reasonable request.
@lmolkova @SergeyKanzhelev what do you think?
There was a problem hiding this comment.
Good idea. To note: the request integration has the same issues, so it would need something similar (I can implement it as well in the same PR as soon as we agree on this part)
There was a problem hiding this comment.
Any leads on how to implement that? I don't think integrations are currently using options/parameters, so I'm not sure what would be the best way to do it.
There was a problem hiding this comment.
We have a blacklist url check method here: https://github.com/census-instrumentation/opencensus-python/blob/master/opencensus/trace/ext/utils.py#L40
And in Flask and Django integration, we allow users to provide a list of blacklisted URLs, and we will disable tracing for the blacklisted ones. Probably in here we could reuse that method.
In the exporters we actually didn't define exporter.host_name in the base class, could you add the param if we need it for every exporter?
There was a problem hiding this comment.
host_name is 'localhost' for several exporters by default: zipkin, jaeger, proto. Tracing calls to localhost could be legit and useful and we should prevent tracking based on something more unique like 'host:port'.
There was a problem hiding this comment.
I also wonder how this list should be shared between different instrumentations: requests, httplib, grpc and who is responsible for configuring it?
My assumption: each exporter should register endpoints in this list. Otherwise users are responsible for finding out all endpoints for exporters they use and registering them in all relevant instrumentations.
Changed name to be more explicit about what it's doing
geobeau
force-pushed
the
fix-recursion
branch
from
b050d9c to
0edecbb
Compare
geobeau
force-pushed
the
fix-recursion
branch
from
5a40cd4 to
04da048
Compare
geobeau
force-pushed
the
fix-recursion
branch
from
f33ecd5 to
1840cd8
Compare
|
Open for another review. I changed it to be more configurable and expanded it to |
geobeau
force-pushed
the
fix-recursion
branch
from
5ad3e8f to
e354219
Compare
| return False | ||
|
|
||
|
|
||
| def disable_tracing_hostname(url, blacklist_hostnames=None): |
There was a problem hiding this comment.
Can this be combined with the disable_tracing_url function in this file?
There was a problem hiding this comment.
I am not sure. They both disable tracing but one is working on paths (blacklist path starting with an element from the list) and the other one with hostname (blacklist hostname/port exactly matching) with different default value. I think it's nice to have them separated for readability.
There was a problem hiding this comment.
@liyanhui1228 Do you stand on your position on this?
There was a problem hiding this comment.
It's fine to separate them :)
|
Anything blocking merge ? @liyanhui1228 @reyang |
|
Hey @geobeau, I'm working with @liyanhui1228. Unless @bogdandrutu or @lmolkova have any more comments on the diff it LGTM (pending picking up recent changes on master), and I can merge it. |
|
Cool, thanks @c24t ! |
|
Looks good to me |
|
See #382 to fix CI timeouts. |
|
It looks like async transport worker threads created in (at least) test_stackdriver_stats.py are each blocking for a few seconds as the test process exits, causing it to hang for long enough for CI to fail. We're likely seeing this now because I increased the export interval in #354 -- lowering the export interval or grace period fixes the long wait time for the threads to exit. I don't know why we're only timing out on this PR, and not the others since #354. Ideally we should be cleaning these threads up as the tests that create them exit, and I think the transport should flush immediately on exit instead of waiting (up to the duration of the grace period) for the next scheduled export. But this is a problem for another PR. |
|
Thanks for the fix! |
7 participants
So the issue is that when we trace httplib we also trace calls made by the exporter which make an infinite loop. The issue was reported in #135. This fix should prevent tracing when we see the same host for the request.