Flamegraph: Add table filtering for Flamegraph panel #78962
Conversation
|
|
Rperry2174
changed the title
| const unfilteredRows = screen.queryAllByText('SomeOtherTextNotInSearch'); | ||
| expect(unfilteredRows.length).toBe(0); // Expect not to find rows that should be filtered out |
There was a problem hiding this comment.
We should also search for this rows text before we perform the filter to make sure it does exist before a search term is input.
It should also be text that we know exists in the table so it is found before we input a search term.
| table[label].totalRight = table[label].totalRight ? table[label].totalRight + valueRight : valueRight; | ||
|
|
||
| // Check if the label includes the search string | ||
| if (!search || label.toLowerCase().includes(search.toLowerCase())) { |
There was a problem hiding this comment.
We may want to do some kind of fuzzy search in the future but I think this works nicely for a first step.
as it turns out this is how it works currently in production as well I suspect this is on purpose. It seems the search ignores certain characters and does a sort of fuzzy matching approach 
I guess we need to do the same logic in the table (or at least be consistent with whatever we're doing) |
There was a problem hiding this comment.
I heard you had a chat with @aocenas about fuzzy search, that you may want to add it to this PR. Let me know if that's not the case.
|
|
||
| // Verify that rows with some other text are not shown after filter | ||
| const rowsAfterFilter = screen.queryAllByText('compress/gzip.(*Writer).Write'); | ||
| expect(rowsAfterFilter.length).toBe(0); |
There was a problem hiding this comment.
We would be more likely to catch an issue with the filtering if we also check this value before we apply the search filter to make sure it exists. That way, after we apply the search filter we expect it not to exist but we know that it did previously exist.
|
After doing some more digging I think this is going to be to complicated for me to implement as the scope is getting increasingly larger. It seems to be working fine(ish), but:
For these reasons I'm going to close and leave this to someone who is better equipped! |
| await userEvent.type(searchInput, 'Handler serve'); | ||
|
|
||
| // We have to wait for filter to take effect | ||
| await waitFor(() => { |
There was a problem hiding this comment.
@Rperry2174 maybe this is why you had trouble with the tests. The filter is debounced so the filtering happens async and the test needs to wait for it to have an effect. There is no explicit synchonization so the way to do it is this a bit crude "try/wait/try until some deadline" helper that the library provides.
Rperry2174
requested review from
Clarity-89 and
JoaoSilvaGrafana
and removed request for
a team
Addresses grafana/pyroscope#2786
Screen.Recording.2023-12-01.at.10.16.46.AM.mov