Disable shared span context by default#11232
Conversation
|
/test istio-integ-k8s-tests |
|
/test istio-pilot-multicluster-e2e |
douglas-reid
left a comment
There was a problem hiding this comment.
/lgtm
Should this also be in release-1.1 ?
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: douglas-reid, objectiser If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@douglas-reid Thought it might be too late for 1.1 branch, but if not would be good to get the fix in. #11281 created. |
|
/assign @hklai |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
not stale |
|
/assign geeknoid |
|
agree with @objectiser that this should also end up in 1.1, which it has. The question is now how does this migrate into master. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@objectiser: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
The envoy proxy (used by Istio proxy) creates server spans that have the same span id as the value supplied with the trace context. This can be disabled through configuration, so that all spans create their own unique id.
Usually having shared span context (i.e. where the client span in service A shares the same id as the server span from called service B) is not an issue - if the envoy proxy creating the client span only creates the one span. However there are cases where other tasks performed in the envoy proxy are also creating their own spans - which have the parent id of the client span.
In these cases, the child span can be incorrectly identified as being "caused by" the server span that had the shared id. To illustrate, this is a view of a trace instance from current master:
As you will see, it is implying that the
productpageservice has called theistio-ingressgateway.By disabling the shared span context feature, the individual client and server spans have the correct parent span references, e.g.
When shared span context is disabled, this has no adverse effect on the representation shown in zipkin: