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.

Sign up for GitHub

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

Jump to bottom

Tempo: Represent OTLP Span Intrinsics correctly #69394

Merged
merged 10 commits into from
Jun 21, 2023
Merged

Tempo: Represent OTLP Span Intrinsics correctly #69394

merged 10 commits into from
Jun 21, 2023

Conversation

joey-grafana
Copy link
Contributor

@joey-grafana joey-grafana commented Jun 1, 2023 •
edited
Loading

What is this feature?

Adds a span intrinsics section and moves all intrinsic tags to that section.

Why do we need this feature?

To represent OTLP Span Intrinsics correctly.

Who is this feature for?

Tempo users.

Which issue(s) does this PR fix?:

Fixes #59607

This also fixes a few issues not reported;

  • span kind internal not in downloaded trace file
  • status.code of unset is ignored in uploaded file but exists in original trace
  • tagsToAttributes(span.tags) throws if span.tags is undefined and export breaks

Special notes for your reviewer:

Screenshot 2023-06-13 at 09 08 15

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@joey-grafana joey-grafana added this to the 10.1.x milestone Jun 1, 2023
@joey-grafana joey-grafana self-assigned this Jun 1, 2023
@grafanabot grafanabot added area/explore type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project area/backend labels Jun 1, 2023
joey-grafana
joey-grafana commented Jun 1, 2023
View reviewed changes
public/app/plugins/datasource/tempo/resultTransformer.ts Outdated
Comment on lines 194 to 198
key: 'otel.status_code',
value: SpanStatusCode[span.status.code],
});
if (span.status.message) {
spanTags.push({ key: 'otel.status_description', value: span.status.message });
intrinsics.push({ key: 'otel.status_description', value: span.status.message });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we call these status.code & status.message in the backend but when transforming here we rename to otel.status_code and otel.status_description - not fully happy with that approach

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

Backend code coverage report for PR #69394
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023 •
edited
Loading

Frontend code coverage report for PR #69394

Plugin Main PR Difference
explore 87.1% 86.94% -.16%

@joey-grafana joey-grafana marked this pull request as ready for review June 1, 2023 15:54
@joey-grafana joey-grafana requested review from a team, grafanabot and lwandz13 as code owners June 1, 2023 15:54
@joey-grafana joey-grafana requested review from joshhunt, tskarhed and mckn and removed request for a team, grafanabot, lwandz13, joshhunt, tskarhed and mckn June 1, 2023 15:54
adrapereira
adrapereira previously requested changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@adrapereira adrapereira left a comment

Choose a reason for hiding this comment

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

In order to represent the OTLP intrinsics correctly we have to make a few more changes to this PR:

  1. error = true shouldn't be displayed in the intrinsics section because it's not a valid intrinsic attribute. It may be a part of other sections if the system has a tag called error but in our case I think we're adding it "manually" to denote an error, which is wrong under OTLP.
  2. span.kind is not a valid intrinsic. kind is an intrinsic though so we can show it instead
  3. same for status.code. However status is a valid intrinsic
  4. Please review all the comments of TraceView: Represent OTLP Span Intrinsics correctly #59607 to make sure we're making all the necessary and accurate changes.

@aocenas
Copy link
Member

aocenas commented Jun 7, 2023

I don't think it makes sense to hide the intrinsics as another table section. I think this comment with the image makes more sense: #59607 (comment). Basically, intrinsics are well-known parts of the span that are always there while attributes is a variable length list of key-value pairs. This should be apparent from the UI and needs a different representation imho.

@joey-grafana
Copy link
Contributor Author

joey-grafana commented Jun 13, 2023 •
edited
Loading

Thanks for the reviews :)

I believe we had discussed in stand up that we were first going to move the intrinsics to their own section and then modify/rename them accordingly after some more discussion. In any case, I've updated this PR and removed the intrinsics section and instead display the values in the span details along with service, duration etc.

I've removed error & span.kind has also been updated. status on the other hand needs to be handled differently. We assume that status is either ok, error or unset but OTEL specifies (+ source) that status contains a message and a code and we also add each of these as intrinsics on the span. The values are represented as otel.status_description & otel.status_code (+ source). This distinction is especially important when downloading/uploading traces.

Screenshot 2023-06-13 at 09 08 15

@joey-grafana joey-grafana requested a review from aocenas June 20, 2023 11:46
aocenas
aocenas approved these changes Jun 20, 2023
View reviewed changes
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.

Small comment about the accessor functions but otherwise look good.

public/app/plugins/datasource/tempo/resultTransformer.ts Outdated Show resolved Hide resolved
@joey-grafana joey-grafana dismissed adrapereira’s stale review June 21, 2023 10:38

Has been reviewed by other team member as original unavailable

@joey-grafana joey-grafana merged commit 00ec9fc into main Jun 21, 2023
@joey-grafana joey-grafana deleted the joey/span-intrinsics branch June 21, 2023 10:39
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
@joey-grafana @LudoVio
Tempo: Represent OTLP Span Intrinsics correctly (#69394)
a266093 
* Span intrinsics

* Update intrinsics and add to span details

* Remove intrinsics section

* Update tests

* Update status code text

* Self review

* Move previously intrinsic values to span

* Remove few methods
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
@chenrui333 chenrui333 mentioned this pull request Aug 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aocenas aocenas aocenas approved these changes

@adrapereira adrapereira adrapereira left review comments

Assignees

@joey-grafana joey-grafana

Labels
add to changelog area/backend area/datasource/frontend area/explore area/frontend datasource/Tempo no-backport Skip backport of PR type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project
Projects
None yet
Milestone
10.1.0
Development

Successfully merging this pull request may close these issues.

TraceView: Represent OTLP Span Intrinsics correctly
5 participants
@joey-grafana @aocenas @adrapereira @ricky-undeadcoders @grafanabot