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 span links for leading 0s trace ID #539

Conversation

everett980
Copy link
Collaborator

@everett980 everett980 commented Mar 5, 2020

Which problem is this PR solving?

  • When ?span=${id}@${traceID} fetches a trace such that:
trace.traceID = `0+${traceID}`

the deep linking would not be passed on.

Short description of the changes

  • Check span links for trace.traceID with leading 0s stripped out if trace.traceID is not present in spanLinks

Signed-off-by: Everett Ross <reverett@uber.com>
@everett980 everett980 requested a review from tiffon as a code owner March 5, 2020 22:33
@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #539 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #539      +/-   ##
==========================================
- Coverage   92.97%   92.95%   -0.03%     
==========================================
  Files         197      197              
  Lines        4840     4840              
  Branches     1174     1174              
==========================================
- Hits         4500     4499       -1     
- Misses        300      301       +1     
  Partials       40       40              
Impacted Files Coverage Δ
.../components/SearchTracePage/SearchResults/index.js 80.76% <ø> (ø)
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx 89.83% <0.00%> (-1.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6597d9...287c2a9. Read the comment docs.

@@ -208,7 +208,7 @@ export class UnconnectedSearchResults extends React.PureComponent<SearchResultsP
linkTo={getLocation(
trace.traceID,
{ fromSearch: searchUrl },
spanLinks && spanLinks[trace.traceID]
spanLinks && (spanLinks[trace.traceID] || spanLinks[trace.traceID.replace(/^0*/, '')])
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding a unit test for this use case.

everett980 and others added 2 commits March 9, 2020 16:55
Fix lookback test issue with dst

Signed-off-by: Everett Ross <reverett@uber.com>
@everett980 everett980 merged commit 832c316 into jaegertracing:master Mar 10, 2020
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Fix span links for leading 0s trace ID
* Add tests for deep linking leading 0s
Fix lookback test issue with dst

Signed-off-by: Everett Ross <reverett@uber.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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.

2 participants