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: Encode IDs as hexadecimal when downloading traces #66001

Merged

Conversation

kousikmitra
Copy link
Contributor

What is this feature?

While downloading traces as json the ids are getting converted to base64. This change will remove the conversion and export ids in hex format instead. So that encoding remain consistent across.

Why do we need this feature?

[Add a description of the problem the feature is trying to solve.]

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #63003

Special notes for your reviewer:

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.

@kousikmitra kousikmitra requested a review from a team as a code owner April 5, 2023 10:22
@grafanabot grafanabot added area/frontend datasource/Tempo pr/external This PR is from external contributor labels Apr 5, 2023
@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pr-codeql-analysis-javascript.yml:analyze. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

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.

LGTM! Thanks for the contribution @kousikmitra

@adrapereira adrapereira added this to the 10.0.0 milestone Apr 13, 2023
@adrapereira adrapereira changed the title Remove converting tempo trace id to base64 Tempo: Encode IDs as hexadecimal when downloading traces Apr 13, 2023
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.

There's actually a couple of issues, sorry!

Apart from my comment you also need to fix this test:

FAIL public/app/plugins/datasource/tempo/datasource.test.ts (22.543 s)
  ● Tempo data source › should handle json file upload

    expect(received).toBe(expected) // Object.is equality

    Expected: "60ba2abb44f13eae"
    Received: "RPE+rg=="

      197 |     expect(field.name).toBe('traceID');
      198 |     expect(field.type).toBe(FieldType.string);
    > 199 |     expect(field.values.get(0)).toBe('60ba2abb44f13eae');
          |                                 ^
      200 |     expect(field.values.length).toBe(6);
      201 |   });
      202 |

      at public/app/plugins/datasource/tempo/datasource.test.ts:199:33

traceID: transformBase64IDToHexString(span.traceId),
spanID: transformBase64IDToHexString(span.spanId),
parentSpanID: transformBase64IDToHexString(span.parentSpanId || ''),
traceID: span.traceId.length > 16 ? span.traceId.slice(16) : span.traceID,
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo here, it should be span.traceId.

Suggested change
traceID: span.traceId.length > 16 ? span.traceId.slice(16) : span.traceID,
traceID: span.traceId.length > 16 ? span.traceId.slice(16) : span.traceId,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, fixed.

@kousikmitra
Copy link
Contributor Author

kousikmitra commented Apr 13, 2023

Updated the mock json data.

After this change the expectation for importing tempo trace from json is, the json data should have traceID, spanID and parentSpanId all in hex format instead of base64.

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.

Thank you for the changes, approved!

@adrapereira adrapereira merged commit e5e0a1c into grafana:main Apr 13, 2023
12 checks passed
@kousikmitra kousikmitra deleted the kousikmitra/fix-tempo-id-encoding branch April 13, 2023 18:12
alexmobo pushed a commit that referenced this pull request Apr 14, 2023
* Remove converting tempo trace id to base64

* fix typo

* Update id format to hex for tempo json mock
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend datasource/Tempo no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tempo] Use consistent encoding when downloading traces as JSON
4 participants