Add a transform to filter samples by marker name by canova · Pull Request #4610 · firefox-devtools/profiler

canova · 2023-05-08T14:11:47Z

Fixes #4590.
This PR is ready for feedback and review

This PR adds a marker context menu item called Filter samples by “<marker name>” markers.

It also changes the per-thread selectors creation pipeline so we can have derived marker selectors for the transform computation. Previously, we created the thread selectors, and then the marker selectors .It made it impossible to get the derived markers for transform applied thread computation. Now we do the thread selectors in 2 steps. We create basic thread selectors first, then we craate the marker selectors, and then we create the more advanced thread selectors that require markers to be computed.

Things I would like to get feedback on:

Wording on the context menu item. This is the best I could come up with but happy to get feedback for improvements.
How we get the "marker name". Currently this is a bit ad hoc. Because we can either have names in the marker payload (e.g. UserTiming markers) or in the name itself. So how I get is that, first I check if marker payload has a name and use it if it has. Otherwise I get the marker name. This is how it's done marker.data && marker.data.name ? marker.data.name : marker.name. We can possible use marker schema for this, but not sure what to pick there as we have multiple labels.
The context menu item icon. I found it from the photon icons here.

It could be better to look at the commits one by one.

Here's a screenshot of the new context menu item:

Here's a screenshot of the transform navigator:

flodolo · 2023-05-09T10:41:15Z

+# It filters the samples by the marker name.
+# Variables:
+#   $markerName (String) - Name of the marker that is selected.
+MarkerContextMenu--filter-samples-by-marker-name = Filter samples by “<strong>{ $markerName }</strong>” markers


Is it just me or this could read better with with? Also, plural or singular marker?

Filter samples with “<strong>{ $markerName }</strong>” marker

Good question, I wasn't sure which one was best but I found this answer: https://english.stackexchange.com/a/392465
It says:

To choose either 'by' or 'with', you need to see if the phrase/word that comes next indicates a method or an instrument. If you want to show a method, you use by. If it's an instrument with which the activity was done, you use with

And here's the part that fits our example:

In your case, 'names' are a method used for filtering users list, not an instrument. So, you have to use 'by'. See the examples below:

I want to filter the users list by names

So I think it's "by", but this is only one answer and it could be wrong. Happy to get more feedback from other people.

For plurality, I chose the plural "markers" because there could be multiple markers with the same name and we account all the markers with that name while filtering.

After rereading the first paragraph of the answer, "instrument" sounds closer to our sentence though. with could be more suitable for this case. 🤔
@julienw @mstange what do you think?

After talking about this in the matrix channel, I've thought a bit about this and decided to use Filter samples within “{ $markerName }” markers (even though I was opposed to it at first :) )

I wanted to use it because I really wanted a one word alternative to the others.
The closes ones I was considering were Filter samples taken during "{ $markerName }" markers and Filter samples overlapping with “{ $markerName }” markers but I felt like they are long.

Let me know what you think!

…ransforms Before the 'filter-samples' transform, we didn't need to get the derived markers to be able to compute any thread related pre-thread selectors. But with 'filter-samples' transform, we need to get the derived markers to be able to compute the transform filtered thread because this transform needs to find the markers that match the filter and get the interval range out of them. So because of this, I had to split the thread related selectors so we can get the marker selectors in between them and have the marker selectors in the second phase of thread selectors. Previously the dependency graph was like this: ``` thread selectors | v stack and sample + marker selectors | v composed selectors ``` Now it's like this: ``` basic thread selectors | v marker selectors | v rest of the thread selectors | v stack and sample selectors | v composed selectors ``` We can't put these thread selectors that require markers into the `composed` selectors because stack and sample selectors require those selectors earlier in the pipeline and I wanted to keep the thread selectors still in their file to keep the pipeline in a single file.

…name

julienw

Thanks, this seems to work very well!

I left a few comments, but my main concern is how we serialize the transform to the URL, that we could use the stringIndex for the marker name. Tell me what you think!

julienw · 2023-05-11T16:44:31Z

 }

+.markerContextMenuIconFilterSamples {
+  background-image: url(firefox-profiler-res/img/svg/filter.svg);


It doesn't look great when the line is selected. Maybe try to invert it on hover (see below). Otherwise we may need to tweak it so that it doesn't have an alpha layer anymore.

Also I'm not sure this is the best icon, I don't see how it relates to droping or markers. But we can change it later so let's not focus on this. Or maybe we can just reuse the icon we use for "drop samples"? Indeed we're not in the same context menu.

Yeah, I'm not super happy about the icon but that's the only one I could find. I didn't want to use "drop function" icon because that uses the tree visualization to convey the meaning which is correct since we apply this to call tree. But we drop samples directly here. But maybe that's fine?

Here's how it looks with the inverter colors, what do you think?:

julienw · 2023-05-11T16:47:56Z

+          name: 'measure-2',
+          entryType: 'measure',
+        },
+      ],


optional: these markers do not look exactly like real profiles, because the name should probably be DOMEvent for dom events, and UserTiming for user timing markers. Then of course you'd have to find another marker type for Marker B so that it's different than the other UserTiming.

Yeah, to be able to identify quickly I did this, but I can change them.

julienw · 2023-05-11T16:49:23Z

+                }}
+                elems={{ strong: <strong /> }}
+              >
+                <>


My understanding is that you're using fragments so that <Localized> properly handles the tags inside the message. Can you please add a comment about this, otherwise it's a bit surprising.

Yes, that's the reason. Added a comment here.

julienw · 2023-05-12T09:31:57Z

+      // This is also the case for drop-function transform. We need to have a
+      // generic mechanism for: if the selected call node (after the transformation
+      // has been applied to the call path) is not present in the call tree, run
+      // some generic code that finds a close-by call node which is present.


Can you please file a bug and add the number here?

Filed it and added it here.

julienw · 2023-05-12T10:15:02Z

+        // Filter string may include "-" characters, so we need to join them back.
+        const filterString = filter.join('-');


Now that we use the marker name, what do you think of using the index in the stringTable instead of the fullstring? If we want to change to a string in the future, we can still change the prefix (m -> M) for example. This would make the filter string a lot shorter and also would remove the problem of the dash character.

Yes, that makes sense! Change it to use string table index.

canova · 2023-05-15T15:04:40Z

Thanks for the review! It should be ready for another review now.

codecov · 2023-05-15T15:11:45Z

Codecov Report

Patch coverage: 84.96% and project coverage change: -0.10 ⚠️

Comparison is base (36bad68) 88.63% compared to head (36a374b) 88.53%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4610      +/-   ##
==========================================
- Coverage   88.63%   88.53%   -0.10%     
==========================================
  Files         293      294       +1     
  Lines       25990    26159     +169     
  Branches     6998     7051      +53     
==========================================
+ Hits        23035    23159     +124     
- Misses       2749     2791      +42     
- Partials      206      209       +3

Impacted Files	Coverage Δ
src/components/shared/MarkerSettings.js	`85.71% <ø> (ø)`
src/components/shared/PanelSearch.js	`71.42% <ø> (ø)`
src/selectors/per-thread/markers.js	`93.63% <ø> (ø)`
src/selectors/per-thread/stack-sample.js	`96.96% <ø> (ø)`
src/utils/text-measurement.js	`100.00% <ø> (ø)`
src/components/shared/MarkerContextMenu.js	`84.56% <50.00%> (-1.80%)`	⬇️
src/components/shared/CallNodeContextMenu.js	`89.24% <60.00%> (-0.72%)`	⬇️
src/profile-logic/transforms.js	`91.42% <65.93%> (-3.00%)`	⬇️
src/app-logic/url-handling.js	`87.07% <75.00%> (-0.31%)`	⬇️
src/components/marker-chart/Canvas.js	`93.30% <75.00%> (-0.27%)`	⬇️
... and 16 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

julienw · 2023-05-24T14:04:17Z

I'm removing my r request because this feature will need to be changed according to our last week discussion.

canova · 2023-05-25T13:06:38Z

Thanks for all the feedback, I'm closing this in favor of #4631. I changed the behavior quite a bit, that's why I wanted to start from scratch.

canova force-pushed the filter-by-marker branch 4 times, most recently from 68d5b71 to 7ff0a57 Compare May 8, 2023 19:33

canova marked this pull request as ready for review May 9, 2023 09:59

canova requested a review from a team as a code owner May 9, 2023 09:59

canova requested a review from julienw May 9, 2023 10:00

flodolo reviewed May 9, 2023

View reviewed changes

canova force-pushed the filter-by-marker branch 2 times, most recently from f017857 to cb0d410 Compare May 9, 2023 14:13

flodolo approved these changes May 9, 2023

View reviewed changes

canova force-pushed the filter-by-marker branch 2 times, most recently from a1570d0 to 8741c07 Compare May 9, 2023 15:31

canova added 5 commits May 9, 2023 20:00

Add a new filter-samples transform that can filter samples by marker …

3aafc3e

…name

Add a marker context menu item to filter samples by marker name

382740d

Add a filter icon for the context menu item

3276a1a

Add tests for "filter-samples" transform

d3361d7

canova force-pushed the filter-by-marker branch from 8741c07 to d3361d7 Compare May 9, 2023 18:00

julienw reviewed May 12, 2023

View reviewed changes

Filter js and native allocation stacks as well

100dcc8

canova mentioned this pull request May 15, 2023

After transforms, if the selected call node is not present in the call tree find a close-by call node to select #4618

Open

canova added 2 commits May 15, 2023 11:03

Fix smaller review nits

ad5f150

Use string table index instead of the full string

9905613

canova force-pushed the filter-by-marker branch from 8c1290c to 9905613 Compare May 15, 2023 15:03

canova requested a review from julienw May 15, 2023 15:04

Change the icon for the context menu item

36a374b

julienw removed their request for review May 24, 2023 14:03

canova mentioned this pull request May 25, 2023

Implement marker filter menu and "drop samples outside of marker filter" transform #4631

Merged

canova closed this May 25, 2023

		// Filter string may include "-" characters, so we need to join them back.
		const filterString = filter.join('-');

Conversation

canova commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flodolo May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw 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!

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!

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!

canova commented May 15, 2023

Uh oh!

codecov Bot commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw commented May 24, 2023

Uh oh!

canova commented May 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

canova commented May 8, 2023 •

edited

Loading

flodolo May 9, 2023 •

edited

Loading

canova May 9, 2023 •

edited

Loading

codecov Bot commented May 15, 2023 •

edited

Loading