Skip to content

Reduce load time of trace page by deferring critical path tooltip#2718

Merged
yurishkuro merged 2 commits intojaegertracing:mainfrom
DamianMaslanka5:disable-tooltip
Apr 28, 2025
Merged

Reduce load time of trace page by deferring critical path tooltip#2718
yurishkuro merged 2 commits intojaegertracing:mainfrom
DamianMaslanka5:disable-tooltip

Conversation

@DamianMaslanka5
Copy link
Copy Markdown
Contributor

@DamianMaslanka5 DamianMaslanka5 commented Apr 12, 2025

I am opening this PR without an issue, I understand that is might not be an acceptable change, feel free to close this PR if you don't agree with this change.

Improve performance (~300ms for 500 spans) by loading tooltip only on hover

Before

criticalPathTooltip

After

criticalPathNoTooltip

@DamianMaslanka5 DamianMaslanka5 requested a review from a team as a code owner April 12, 2025 16:47
@DamianMaslanka5 DamianMaslanka5 requested review from mahadzaryab1 and removed request for a team April 12, 2025 16:47
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.79%. Comparing base (6356d5b) to head (ef0fc70).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2718   +/-   ##
=======================================
  Coverage   96.78%   96.79%           
=======================================
  Files         256      256           
  Lines        7937     7944    +7     
  Branches     2045     1994   -51     
=======================================
+ Hits         7682     7689    +7     
- Misses        254      255    +1     
+ Partials        1        0    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I understand what you're trying to do (effectively reduce the size of the DOM), but the resulting behavior is unintuitive, from a user perspective the tooltip would work or not "randomly" (unless they know about 500 spans threshold).

Isn't there another mechanism in React where the tooltip doesn't need to be physically added to the DOM but still can be shown dynamically?

@yurishkuro yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Apr 13, 2025
@DamianMaslanka5
Copy link
Copy Markdown
Contributor Author

I understand what you're trying to do (effectively reduce the size of the DOM), but the resulting behavior is unintuitive, from a user perspective the tooltip would work or not "randomly" (unless they know about 500 spans threshold).

I agree this is not perfect.

Isn't there another mechanism in React where the tooltip doesn't need to be physically added to the DOM but still can be shown dynamically?

Thanks for the question, it made me think a bit more. Tbh I am not very familiar with react, so I am open to all suggestions.


I made changes to create the tooltip only on hover 41ef333, I just added another commit, and removed previous changes in the same commit, just remember to squash the commits when merging (or I can squash this locally and force push), I am not sure if you squash all commits by default, on merge

Render time in profiler looks good

image

Tooltips work correctly

firefox_f27lZW4NM5

Improve performance (~300ms for 500 spans) by loading
critical path tooltip only on hover.

Signed-off-by: Damian Maslanka <damian.maslanka5@gmail.com>
@DamianMaslanka5
Copy link
Copy Markdown
Contributor Author

@yurishkuro, I updated this branch

  1. I added test to avoid reducing code coverage
  2. I removed the initial commit, which disabled the tooltip when 500 spans existed. Now tooltip is loaded only when hovering mouse over critical path.

</div>
}
>
<div
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we avoid duplicating this code segment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, see #2718 (comment)

I will change this tomorrow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in ef0fc70 I moved the duplicated code to const criticalPath

Signed-off-by: Damian Maslanka <damian.maslanka5@gmail.com>
@yurishkuro yurishkuro changed the title Reduce load time of trace page Reduce load time of trace page by deferring critical path tooltip Apr 28, 2025
@yurishkuro yurishkuro added this pull request to the merge queue Apr 28, 2025
Merged via the queue into jaegertracing:main with commit 6fbbbcb Apr 28, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants