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

Fixes TraceTimelineViewer span details render issue #629

Merged
merged 2 commits into from
Sep 5, 2020

Conversation

rubenvp8510
Copy link
Collaborator

Which problem is this PR solving?

  • This fixes Issue Span bar broken on click #628 , a regression when refactored VirtualizedTraceView to remove deprecated method componentWillReceiveProps and use the componentDidUpdate instead.

Short description of the changes

  • The component was using componentWillReceiveProps to derive data from properties and store in the instance attributes to avoid recompute it in each render call, With the migration, that process was changed to componentDidUpdate, but this is not correct, RowState computation on that method doesn't trigger a new render process, so we ended up seeing something not in sync with the RowState. This change uses memoization for rowState to avoid recompute it each time.

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #629 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
+ Coverage   93.54%   93.59%   +0.05%     
==========================================
  Files         227      227              
  Lines        5901     5902       +1     
  Branches     1485     1483       -2     
==========================================
+ Hits         5520     5524       +4     
+ Misses        340      337       -3     
  Partials       41       41              
Impacted Files Coverage Δ
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx 95.08% <100.00%> (+2.51%) ⬆️

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 5b4ca26...ac10a5f. Read the comment docs.

@objectiser
Copy link
Contributor

Tested with hotrod example, seems to fix the problem.

@yurishkuro
Copy link
Member

@tklever please review

@tklever
Copy link
Contributor

tklever commented Aug 27, 2020

I pulled this code locally and I can no longer replicate the "empty box" problem. This looks good (to my limited expertise).

I cooked up a couple tests last night to lock in @rubenvp8510's work in this PR, specifically to ensure that the memoization he added is preserved and can't be accidentally removed. When this PR (or some version of it) lands, I'll submit some unit tests so we (or I in this case :-( ) can't reintroduce this regression in the future.

@rubenvp8510 rubenvp8510 changed the title Fixes TraceTimelineViewer span details Fixes TraceTimelineViewer span details render issue Sep 5, 2020
@rubenvp8510 rubenvp8510 self-assigned this Sep 5, 2020
@rubenvp8510
Copy link
Collaborator Author

@yurishkuro Could you approve this change please? Thanks!

@yurishkuro yurishkuro merged commit 7094f85 into jaegertracing:master Sep 5, 2020
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
* Fixes TraceTimelineViewer span details

Signed-off-by: Ruben Vargas <[email protected]>

* perform deep comparision for memoized ViewBoundsFunc and GetCssClasses

Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: vvvprabhakar <[email protected]>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
* Fixes TraceTimelineViewer span details

Signed-off-by: Ruben Vargas <[email protected]>

* perform deep comparision for memoized ViewBoundsFunc and GetCssClasses

Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: vvvprabhakar <[email protected]>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Fixes TraceTimelineViewer span details

Signed-off-by: Ruben Vargas <[email protected]>

* perform deep comparision for memoized ViewBoundsFunc and GetCssClasses

Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: vvvprabhakar <[email protected]>
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.

5 participants