Show sample tooltips on sample graph hover by canova · Pull Request #5298 · firefox-devtools/profiler

canova · 2025-01-09T15:48:49Z

This was requested by multiple people because sometimes it's not clear to users what the squares in the sample graph are. For example latest feedback we got was in #5278. They also said that a tooltip would've helped.

Deploy preview / Production

fqueze · 2025-01-09T18:16:48Z

Looks nice, thanks!

Some feedback (can be handled in follow-ups) :

it might make the tooltip even more explicit if we put as the first line of it a line saying "Sample time: "
I never noticed it before because we didn't have a way to show the tooltip with the stack for idle samples, but the square next to the "Idle" category is white, but the vertical bar on the left side of the stack frames is grey. The frame "Native event loop idle" should probably have the vertical bar next to it white too.

julienw

It's great to see this tooltip now, we can always improve the look and fix the small issues later.

I noticed that sometimes hit testing is a bit off: my mouse cursor is clearly on the rectangle, yet the tooltip doesn't appear. It seems to happens at some page zoom levels only. If the fix is easy you can do it now, but if you don't find the source now, it's fine to look at it later.

Also I think the hit target is a bit small and we might want to increase it to some more than just the rectangle. But that could be for later too.

Thanks!

julienw · 2025-01-10T10:09:18Z

+
+    const rect = canvas.getBoundingClientRect();
+    this.setState({
+      hoveredPixelState: this._getSampleAtMouseEvent(event),


_getSampleAtMouseEvent only needs pageX (that we have in the state), so we could call it at render time instead of at event time. Very likely this would result in some performance improvement, because React renders less often than move events are triggered.
(although I believe synthetic move events might be throttled already, but likely not as much as rendering)

To be able to the tooltip in the render function we need to know if we really hit sample or not. Otherwise we can't have this code in render:

{hoveredPixelState === null ? null : ( <Tooltip mouseX={mouseX} mouseY={mouseY}> <SampleTooltipContents .../> </Tooltip> )}

but we could call _getSampleAtMouseEvent in render? or I am I missing something?

But I'm also thinking of something else: we setState mouseX and mouseY at each mousemove, which will trigger a rerender, thefore I wonder if we shouldn't set mouseX and mouseY to 0 if _getSampleAtMouseEvent returns null, then the state wouldn't change. For this to work we need to call _getSampleAtMouseEvent.

I guess both these suggestions can be categorized as "premature optimization" and it could be good to profile to see if calling _getSampleAtMouseEvent at each mousemove is really an issue. This can be done after this patch lands.

Yeah, I don't think it would make a big difference to change this but I don't have the numbers as well. I will do some profiling after I merge it and create a followup if it proves that it needs more optimizations here.

julienw · 2025-01-10T10:22:53Z

+      0.8,
+      trueIntervalPixelWidth * multiplier
+    );
+    const drawnSampleWidth = Math.min(drawnIntervalWidth, 10) / 2;


it could be better to extract this computation to a separate function, so that we're sure we always have the same one in drawCanvas and here.

I didn't want to do this initially because inside the canvas drawing we have a lot of code below that still uses these numbers that we use to calculate the drawnSampleWidth. So creating a function to calculate this would result us in calculating some of these values twice for each draw.

ah I see, then a comment here mentioning what this code is would be good, currently it's not obvious IMO.

Sure, added a comment here.

canova

Thanks to you both for the feedbacks and reviews! I've updated the PR. @julienw asking for a review just in case since I changed a few places.

it might make the tooltip even more explicit if we put as the first line of it a line saying "Sample time: "

I never noticed it before because we didn't have a way to show the tooltip with the stack for idle samples, but the square next to the "Idle" category is white, but the vertical bar on the left side of the stack frames is grey. The frame "Native event loop idle" should probably have the vertical bar next to it white too.

Yeah, they are good points. I will look at them as follow-ups!

canova · 2025-01-14T13:14:58Z

+      0.8,
+      trueIntervalPixelWidth * multiplier
+    );
+    const drawnSampleWidth = Math.min(drawnIntervalWidth, 10) / 2;


I didn't want to do this initially because inside the canvas drawing we have a lot of code below that still uses these numbers that we use to calculate the drawnSampleWidth. So creating a function to calculate this would result us in calculating some of these values twice for each draw.

canova · 2025-01-14T13:22:06Z

+
+    const rect = canvas.getBoundingClientRect();
+    this.setState({
+      hoveredPixelState: this._getSampleAtMouseEvent(event),


To be able to the tooltip in the render function we need to know if we really hit sample or not. Otherwise we can't have this code in render:

{hoveredPixelState === null ? null : ( <Tooltip mouseX={mouseX} mouseY={mouseY}> <SampleTooltipContents .../> </Tooltip> )}

codecov · 2025-01-14T14:51:52Z

Codecov Report

Attention: Patch coverage is 82.25806% with 11 lines in your changes missing coverage. Please review.

Project coverage is 86.08%. Comparing base (c2af69b) to head (ede1005).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/profile-logic/profile-data.js	62.50%	4 Missing and 2 partials ⚠️
src/components/shared/thread/SampleGraph.js	89.13%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5298   +/-   ##
=======================================
  Coverage   86.07%   86.08%           
=======================================
  Files         311      311           
  Lines       29644    29674   +30     
  Branches     8178     8191   +13     
=======================================
+ Hits        25517    25544   +27     
- Misses       3547     3548    +1     
- Partials      580      582    +2

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

julienw

thanks!

…graph Previously we were always returning the closest sample to the mouse hover/click location. Which was okay when we were clicking to select the boxes before. But now we are also showing tooltips on hover which can be misleading if they were shown when we are not hovering over them. So to make this experience less confusing, this commit changes the behavior to only select or hover a specific sample when the mouse is actually on top of the sample box that we draw.

Previously we were using the exact same algorithm as the activity graph. Activity graph tests get the value in between this and the next same time. But for these tests, we actually want the exact sample time.

…raph

canova

Thanks for the review Julien!

Also I thought I already answered this question but I forgot. We talked synchronously but adding here for visibility:

I noticed that sometimes hit testing is a bit off: my mouse cursor is clearly on the rectangle, yet the tooltip doesn't appear. It seems to happens at some page zoom levels only. If the fix is easy you can do it now, but if you don't find the source now, it's fine to look at it later.

Yeah, I noticed that as well. I looked at the code but couldn't notice something that could cause this issue. I was thinking that it could be a floating point issue but not sure where it comes either. I also see this problem on some other parts of the timeline. I will investigate it further later.

Also I think the hit target is a bit small and we might want to increase it to some more than just the rectangle. But that could be for later too.

Yeah the squares are a bit small. I will look into it too.

canova · 2025-01-15T12:54:20Z

+
+    const rect = canvas.getBoundingClientRect();
+    this.setState({
+      hoveredPixelState: this._getSampleAtMouseEvent(event),


Yeah, I don't think it would make a big difference to change this but I don't have the numbers as well. I will do some profiling after I merge it and create a followup if it proves that it needs more optimizations here.

canova · 2025-01-15T12:54:31Z

+      0.8,
+      trueIntervalPixelWidth * multiplier
+    );
+    const drawnSampleWidth = Math.min(drawnIntervalWidth, 10) / 2;


Sure, added a comment here.

Updates: [Nicolas Chevobbe] Adapt Keyboard shortcut dialog in High Contrast Mode. (#5245) [Nicolas Chevobbe] Fix sidebar-toggle in High Contrast Mode. (#5246) [Nicolas Chevobbe] Fix timeline selection overlay time / hover line in High Contrast Mode (#5247) [Zac Spitzer] fix broken link for processed profile format (#5267) [Greg Tatum] Update the memory allocation docs and add DHAT docs (#5270) [Markus Stange] Simplify some code related to thread CPU deltas (#5265) [Greg Tatum] Update dhat convertor to work better with valgrind (#5269) [Markus Stange] Rename UniqueStringArray to StringTable. (#5283) [Markus Stange] Use snapshot testing in the symbolicator CLI test. (#5284) [Markus Stange] Fix two confused upgraders which didn't expect to be run on the serialized format. (#5285) [Nicolas Chevobbe] Fix timelineSettingsHiddenTracks in High Contrast Mode. (#5250) [Julien Wajsberg] Fix some test and non-test warnings (#5294) [Nisarg Jhaveri] Support importing simpleperf trace files from Android Studio (#5212) [Sean Kim] Add HTTP response status code in the profiler network tab (#5297) [Markus Stange] Change StringTable API a bit. (#5286) [Markus Stange] Correctly declare imported simpleperf trace profiles to be of the current version. (#5312) [Markus Stange] Two small changes (#5313) [Nazım Can Altınova] Show sample tooltips on sample graph hover (#5298) Also thanks to our localizers: en-CA: chutten es-CL: ravmn es-CL: ravmn fr: Théo Chevalier ia: Melo46 ia: Melo46 sv-SE: Andreas Pettersson uk: Lobodzets zh-TW: Olvcpr423

canova mentioned this pull request Jan 9, 2025

FR: Documentation of timeline feature #5278

Open

canova requested a review from julienw January 9, 2025 15:51

canova force-pushed the tooltip-sample-graph branch from 16bcdf4 to 54e08da Compare January 9, 2025 15:51

julienw approved these changes Jan 10, 2025

View reviewed changes

canova force-pushed the tooltip-sample-graph branch from 54e08da to 3bc1afd Compare January 14, 2025 14:27

canova commented Jan 14, 2025

View reviewed changes

canova requested a review from julienw January 14, 2025 14:33

julienw approved these changes Jan 14, 2025

View reviewed changes

canova added 7 commits January 15, 2025 13:55

Show sample tooltips on sample graph hover

8f243f7

Fix the incorrect sample position calculation in the sample graph tests

7ec04ad

Previously we were using the exact same algorithm as the activity graph. Activity graph tests get the value in between this and the next same time. But for these tests, we actually want the exact sample time.

Add some tests for the changed SampleGraph behavior

ec7941f

Fix some small nits

4472715

Split the sample graph canvas code into a new component

34909ce

Add a comment explaining the copies of some variables in the sample g…

ede1005

…raph

canova force-pushed the tooltip-sample-graph branch from 3bc1afd to ede1005 Compare January 15, 2025 12:56

canova commented Jan 15, 2025

View reviewed changes

canova merged commit 73b2d84 into firefox-devtools:main Jan 15, 2025

canova deleted the tooltip-sample-graph branch January 15, 2025 13:02

julienw mentioned this pull request Jan 15, 2025

Deploy January 15, 2025 #5318

Merged

This was referenced Jan 15, 2025

Consider adding "Sample time: " to the sample tooltips in the sample graph to make it more explicit that it's a sample #5319

Closed

Add a "Sample timestamp" field to the sample tooltip in timeline #5322

Merged

canova mentioned this pull request Feb 6, 2025

An error occurred in Firefox Profiler while analysing a profile #5290

Open

Uh oh!

Conversation

canova commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fqueze commented Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

canova commented Jan 9, 2025 •

edited

Loading

fqueze commented Jan 9, 2025 •

edited

Loading

codecov Bot commented Jan 14, 2025 •

edited

Loading