Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should instrumentations be able to interact with or know about other instrumentations #369

Closed
owais opened this issue Mar 12, 2021 · 4 comments
Labels

Comments

@owais
Copy link
Contributor

owais commented Mar 12, 2021

There are some cases where an instrumentation may generate a completely valid span on it's own but the span may not make sense when looking at the overall trace. Couple of examples:

WSGI + django/flask/etc

If WSGI and django instrumentations both are enabled, WSGI will extract context from incoming request and generate a SERVER span. Django will extract context from incoming request headers and again generate SERVER span.

If the incoming request has tracing context present in it, the two spans will be siblings instead of the WSGI span being parent of the Django span.

If the incoming request does not have any tracing context present in it, the two spans will not even be part of the same trace and the same request will generate multiple traces.

One possible fix for this is for django (and other server side instrumentations) to check if a parent span is active in the current context and not extract parent context from request headers in this case.

Q. Would there be a situation where there might be a parent span present but not generated for the current in-flight request? Let's say a long lived span representing the process/worker lifetime. It is unlikely but something we should try to answer.

Example:
https://cloud-native.slack.com/archives/C01PD4HUVBL/p1614339078002900
https://cloud-native.slack.com/archives/C01PD4HUVBL/p1614340059009000

urllib3/requests + http.client

urllib3 and requests instrumentations both generate CLIENT spans and inject tracing context into HTTP headers. Both libraries use http.client internall and we may also have an instrumentation for http.client. Such an instrumentation would obviously generate a CLIENT span and inject tracing context again. If both requests/urllib3 and http.client instrumentations are enabled, this will result in a parent > child span pair where both spans are of type CLIENT.

Solution proposed in a PR (#299) is for urllib to suppress http instrumentation by setting a well-known key in context. While this solves the problem of having multiple CLIENT spans, it completely silences the lower level instrumentation which might be able to add a lot more contextual information (DNS resolution time/cache hit/miss, etc) to the spans. I'm not sure if this is the ideal solution.

Another possible solution might be for urllib3 and requests to never generate CLIENT spans and specify the http.client instrumentation as a dependency. So whenever urllib3 or requests is installed, it always installs http.client instrumentation as well.

Example: #299 (comment)


Both of these cases scream for some kind of system that either let's intrumentations discover each other and modify their behaviour, or let's them inspect parent/child spans and then modify their own span accordingly. I think letting instrumentations discover each other might not be as complex as it might sound and solve both problems mentioned above. Would love to hear other thoughts.

@owais owais mentioned this issue Mar 12, 2021
6 tasks
@owais
Copy link
Contributor Author

owais commented Mar 26, 2021

One simple solution for this would be for such child spans to modify parent spans as they see fit. For example, instead of urllib3 instrumentation silencing http.client instrumentation, http.client spans should check if the parent span has type set to CLIENT and update it to INTERNAL instead. Similarly in case of Django + WSGI, Django instrumentation should check if there is an active span present in the current context and perhaps also if the span's type is set to SERVER. In case it finds such a span in current context, it should use the span as parent instead of extracting parent context from incoming request. I think Java and Node do something similar but I'll need to look deeper and confirm it.

We'll need to come up with some heuristics for such cases I'm fairly sure we should be able to solve most if not all of such cases by letting a child span look at parent before deciding on it's own fields/attributes or even modify it's parent's fields/attributes.

@github-actions
Copy link

This issue was marked stale due to lack of activity. It will be closed in 30 days.

@lzchen
Copy link
Contributor

lzchen commented Apr 27, 2021

@owais
Has this issue been brought up to the SIG?

@owais
Copy link
Contributor Author

owais commented Apr 27, 2021

Yes, we brought it up once but did not have a decision. We then proposed two concrete solution to specific issues

#456 and #445

There were no objections to 445 and everyone agreed to do it.

446 is a bit more involved as it needs changes to the API spec.

@owais owais closed this as completed Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants