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

Fix trace name resolution for spans with remote parents #461

Closed
wants to merge 1 commit into from
Closed

Fix trace name resolution for spans with remote parents #461

wants to merge 1 commit into from

Conversation

tmthrgd
Copy link

@tmthrgd tmthrgd commented Oct 7, 2019

When encountering a trace whose root span contains a reference to a remote parent span in a different trace, the UI would fail to determine the root span and instead display the fallback <trace-without-root-span> trace name.

This change makes it so that the first span that has no parents in the current trace is correctly considered the root span. Previously only a span with no references/parents was considered
the root span.

To simplify the logic and ensure consistency, transformTraceData was changed to use the fixed getTraceName.

Traces with remote parents are commonly used when handling HTTP or RPC requests from untrusted clients. See the opencensus-specs util/HandleUntrustedRequests.md for an example of this approach.

I understand that I should add a test for this, but I'm unsure where would be appropriate. If someone can point me to an appropriate file, I'll add that to this PR.

An example of a trace that contains a remote parent.
{
    "data": [
        {
            "traceID": "46412397683b6cee4f02fa1ac7e4d0db",
            "spans": [
                {
                    "traceID": "46412397683b6cee4f02fa1ac7e4d0db",
                    "spanID": "7ef1d2e931ada231",
                    "operationName": "operation",
                    "references": [
                        {
                            "refType": "CHILD_OF",
                            "traceID": "79b24a4a9fb5d4e46895ab64f55dfc90",
                            "spanID": "9111732227172a77"
                        }
                    ],
                    "startTime": 1257894000000000,
                    "duration": 1000,
                    "tags": [],
                    "logs": [],
                    "processID": "p1",
                    "warnings": null
                }
            ],
            "processes": {
                "p1": {
                    "serviceName": "service",
                    "tags": []
                }
            },
            "warnings": null
        }
    ],
    "total": 0,
    "limit": 0,
    "offset": 0,
    "errors": null
}

Updates #117
Updates #134

When encountering a trace whose root span contains a reference
to a remote parent span in a different trace, the UI would fail
to determine the root span and instead display the fallback
<trace-without-root-span> trace name.

This change makes it so that the first span that has no parents
in the current trace is correctly considered the root span.
Previously only a span with no references/parents was considered
the root span.

To simplify the logic and ensure consistency, transformTraceData
was changed to use the fixed getTraceName.

Traces with remote parents are commonly used when handling HTTP
or RPC requests from untrusted clients. See the opencensus-specs
util/HandleUntrustedRequests.md for an example of this approach.

An example of a trace that contains a remote parent:
{
    "data": [
        {
            "traceID": "46412397683b6cee4f02fa1ac7e4d0db",
            "spans": [
                {
                    "traceID": "46412397683b6cee4f02fa1ac7e4d0db",
                    "spanID": "7ef1d2e931ada231",
                    "operationName": "operation",
                    "references": [
                        {
                            "refType": "CHILD_OF",
                            "traceID": "79b24a4a9fb5d4e46895ab64f55dfc90",
                            "spanID": "9111732227172a77"
                        }
                    ],
                    "startTime": 1257894000000000,
                    "duration": 1000,
                    "tags": [],
                    "logs": [],
                    "processID": "p1",
                    "warnings": null
                }
            ],
            "processes": {
                "p1": {
                    "serviceName": "service",
                    "tags": []
                }
            },
            "warnings": null
        }
    ],
    "total": 0,
    "limit": 0,
    "offset": 0,
    "errors": null
}

Signed-off-by: Tom Thorogood <[email protected]>
@everett980
Copy link
Collaborator

@tmthrgd Thanks for submitting this PR.
The best place to test /src/model/trace-viewer would be a new file: /src/model/trace-viewer.test.js
/src/model/transform-trace-data unfortunately does not have a dedicated test file yet, and I think that would be beyond the scope of this PR.

Additionally, if you could convert trace-viewer to typescript (and update the suffix to .tsx) that would be great. I believe the only change necessary would be:

import { SpanData } from '../types/trace';

// eslint-disable-next-line import/prefer-default-export
export function getTraceName(spans: SpanData[]): string {

@@ -14,6 +14,6 @@

// eslint-disable-next-line import/prefer-default-export
export function getTraceName(spans) {
const span = spans.filter(sp => !sp.references || !sp.references.length)[0];
const span = spans.find(sp => !sp.references || !sp.references.some(ref => ref.traceID === sp.traceID));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like an equivalent or a superset of the previous logic.

I would rather fix this like this:

  • find all spans that either have no parent reference, or a parent reference that is not found in the trace
  • if only one such span is found, it's the root (i.e. source of "trace name")
  • if more than one such span is found, then
    • if exactly one of them has no parent refs, it's the root
    • otherwise give up (or, could still do something like pick the one with the earliest start time)

Clearly, we need a set of unit tests for this logic.

@tmthrgd
Copy link
Author

tmthrgd commented Apr 12, 2021

Looks like this was superseded and fixed by #541. 🎉

@tmthrgd tmthrgd closed this Apr 12, 2021
@tmthrgd tmthrgd deleted the trace-name branch April 12, 2021 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants