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

Tempo / Trace Viewer: Support span links #41438

Closed

Conversation

erin-noe-payne
Copy link
Contributor

What this PR does / why we need it:

This PR introduces span references or links (depending on jaeger or open telemetry nomenclature) and wires them up to the tempo datasource. Currently this PR does not have any impact on the UI - I will follow up with commits to wire up to the trace viewer. It is simply modifying the the types from the grafana/data package and plumbing through to the tempo datasource.

Screen Shot 2021-11-08 at 3 57 25 PM

Which issue(s) this PR fixes:

Fixes #41294

Special notes for your reviewer:

@grafanabot grafanabot added area/backend area/frontend type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project datasource/Tempo pr/external This PR is from external contributor labels Nov 8, 2021
@erin-noe-payne
Copy link
Contributor Author

Tracing in general seems to be standardizing around the opentelemetry spec, and Tempo uses otel protos. The grafana UI (jaeger UI TraceTimelineViewer, etc) expect to receive traces in the jaeger format, so we need to transform data along the way. With respect to this feature, the relevant differences are:

  • OTEL calls them links, Jaeger calls them references
  • OTEL calls key/value metadata attributes, Jaeger calls them tags
  • OTEL links include attributes (proto), Jaeger does not (ts types)
  • Jaeger has a concept of refType, which can be CHILD_OF or FOLLOWS_FROM (jaeger spec doc explaining the difference)
    • OTEL spans define their parent span through a parentSpanId property, Jaeger does it through a span reference with type CHILD_OF (code that transforms between these)
    • It seems like the intention of Jaeger's FOLLOWS_FROM references is roughly equivalent to the intention of OTEL links.

Currently the code seems to stick to Jaeger's nomenclature, so I am continuing that pattern.

I'm calling it out now because it has already felt a little awkward to push more Jaeger terminology into the Tempo datasource, and when I get to the UI I will need to push some OTEL concepts (link attributes) into the Jaeger data contract. Hopefully this will help with readability of the code.

@erin-noe-payne
Copy link
Contributor Author

erin-noe-payne commented Nov 9, 2021

I've updated this PR to include the UI wire up. It's incomplete but represents how the data would be plumbed through. At this point the main problem is around creating links:

  • The Trace Viewer UI has the span reference data - it knows the trace & span ids that it would need to look up in order to "follow" the reference link
  • In order to follow the reference link, I need to construct an internal link to look up the new trace / span ids against the appropriate trace datasource - ie the trace datasource we used to look up the trace we are currently viewing
  • The structure of the query you would build is different for different trace datasources, so there's some amount of manual wire up that would be needed for any given trace datasource to support following trace links
  • I'm just still trying to understand the mechanics of internal links, there's a lot going on there.

It's not exactly the same, but this seems to operate in a similar conceptual space to what @aocenas has been building with traceToLogs links - https://github.com/grafana/grafana/blob/main/public/app/features/explore/TraceView/createSpanLink.tsx

Screen Shot 2021-11-10 at 9 44 53 AM

Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

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

Hey sorry for getting to this so late. I thought I already reviewed this, but probably forget to submit or something :(. Apart from one comment and some general cleanup I think this is good. Let me know if you want/have time to continue or just or if you allow edits from maintainers I can push some changes so we can merge this.

@@ -437,6 +443,11 @@ export class UnthemedSpanBarRow extends React.PureComponent<SpanBarRowProps> {
</span>
<small className={styles.endpointName}>{rpc ? rpc.operationName : operationName}</small>
<small className={styles.endpointName}> | {label}</small>
{references.find((ref) => ref.refType === 'FOLLOWS_FROM') && (
Copy link
Member

Choose a reason for hiding this comment

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

This is probably only part I would leave out for now. Reason is we have other icons here (to logs) which is actually a link. Having non clickable icon next to it is probably confusing. At the same time as there can be multiple links we would need a dropdown menu in case of multiple refs (which would probably make sense also for the log links). In any case probably would make sense as a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed we should revisit the UI a bit to consider how to handle 'n' links. Trace to logs was 0 to 1, now going from 1 to 2 it makes sense to consider how to handle any number as we'll likely have some more internal linking use cases soon.

@foriequal0
Copy link

Hi! Any news on this? I'm interested in this feature

@erin-noe-payne
Copy link
Contributor Author

This is a draft PR and was not completed. Closing in favor of #45632, which covers complete implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/explore area/frontend datasource/Tempo pr/external This PR is from external contributor type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants