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

Process FOLLOWS_FROM spans in TraceView #335

Merged
merged 4 commits into from
Mar 7, 2019

Conversation

rubenvp8510
Copy link
Collaborator

@rubenvp8510 rubenvp8510 commented Feb 22, 2019

Which problem is this PR solving?

Short description of the changes

  • I noticed that there is FOLLOWS_FROM attributes on some spans that wasn't take into account when the indentation of the spans is computes on the TreeView.

Not sure if this is the proper way of handling this, I left two comments for two assumptions that I did. if some of the maintainers can review please. @tiffon @everett980

Thanks!

@rubenvp8510 rubenvp8510 changed the title Process FOLLOW_UP spans in TraceView Process FOLLOWS_FROM spans in TraceView Feb 22, 2019
@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #335 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
+ Coverage   83.17%   83.18%   +0.01%     
==========================================
  Files         145      145              
  Lines        3221     3224       +3     
  Branches      656      658       +2     
==========================================
+ Hits         2679     2682       +3     
  Misses        434      434              
  Partials      108      108
Impacted Files Coverage Δ
...ts/TracePage/TraceTimelineViewer/SpanTreeOffset.js 100% <100%> (ø) ⬆️

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 75a7069...b7e21e6. Read the comment docs.

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

I left a comment with a suggestion. What do you think?

@rubenvp8510
Copy link
Collaborator Author

I like the algorithm you put in the comments, simpler that mine, at the beginning I thought we will give an special treatment to the FOLLOWS_FROM, that is why I split it in two different things, but after an analysis of my implementation and your code it seems like from the UI perspective (or at least for this view) we won't do a distinction. Correct me if this is wrong.

I'll simplify my code.

@rubenvp8510
Copy link
Collaborator Author

I also think that we need to put some tests for this, to avoid regressions.

@rubenvp8510 rubenvp8510 force-pushed the Issue-333 branch 3 times, most recently from 96ecc7e to a60fd50 Compare February 25, 2019 19:00
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
@rubenvp8510
Copy link
Collaborator Author

@tiffon @everett980 I added one test to take FOLLOWS_FROM into account. I think this is ready for another review, if there is no more comments and you decide it's ok, then it is ready for merge.

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 👍

@ghost ghost assigned tiffon Mar 7, 2019
@ghost ghost added the review label Mar 7, 2019
@tiffon tiffon merged commit caf91a7 into jaegertracing:master Mar 7, 2019
@ghost ghost removed the review label Mar 7, 2019
everett980 pushed a commit to everett980/jaeger-ui that referenced this pull request Mar 7, 2019
* Process FOLLOWS_FROM spans in TraceView

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

* Add test for FOLLOWS_FROM span relation for SpanTreeOffset

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Process FOLLOWS_FROM spans in TraceView

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

* Add test for FOLLOWS_FROM span relation for SpanTreeOffset

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Process FOLLOWS_FROM spans in TraceView

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

* Add test for FOLLOWS_FROM span relation for SpanTreeOffset

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.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.

Timeline visualization regression
4 participants