grafana / explore-profiles

Explore Profiles is a native Grafana application designed to integrate seamlessly with Pyroscope, the open-source continuous profiling platform, providing a smooth, query-less experience for browsing and analyzing profiling data.
GNU Affero General Public License v3.0
17 stars 1 forks source link

feat: create new browser history entry on some user actions #128

Closed ifrost closed 2 weeks ago

ifrost commented 2 weeks ago

✨ Description

Related issue(s):

Fixes #121

πŸ“– Summary of the changes

Adds basic support for browser history. A new history is created upon user actions:

Here's how it works:

https://github.com/user-attachments/assets/9f45e476-385c-489f-b7cf-fc6b74a630f1

Technical details

The approach is to make sure we add new user actions to the history stack. Just before the user action handler is called we create a new empty entry in the stack and then UrlSyncManager takes care of replacing that history with any updates to variables. This will include multiple updates, and chain of updates. Any changes to URL that are not coming from the user action will be automatically added to the current history.

Alternative approaches:

1) Push history change on each variable change or every time the URL changes.

That can work fine only when there's one variable updated upon user action. When there are multiple updates, they need to be somehow batched into single entry and it's tricky because the update is asynchronous. It's also visible during loading initial state as it usually requires multiple vars to be updated.

2) Use drilldowns

That works fine but requires some boilerplate just to support it in our case (SceneApp and SceneAppPage). It works great when it's used with native tabs coming with scenes but we don't use it for exploration type. Hence, to support drilldowns it would require even more boilerplate to create correct URLs for each exploration type (basically duplicating something that scenes handles internally with preserverUrlKeys

3) Change the variable via URL directly

Instead of calling variable.changeValueTo we can change the value directly by modifying the URL. Dashboards do it to handle history (example). This way there's no need to depend on location.partial happening after pushing a new entry (like in this PR). The drawback might be that if a variable name is not unique it's hard to determine it's name as it's mapped by UrlSyncManager/UniqueUrlKeyMapper.

πŸ§ͺ How to test?

It requires manual testing. Please try changing elements mentioned above (and others) and see if back/forward buttons behave as expected.

github-actions[bot] commented 2 weeks ago

Unit test coverage

Lines Statements Branches Functions
Coverage: 10%
10.63% (476/4477) 8.24% (136/1650) 7.99% (109/1363)
ifrost commented 2 weeks ago

cc @torkelo @dprokop, would be great to get your feedback on this approach as well to check if we're not setting ourselves for failure and if there's a better approach.

grafakus commented 2 weeks ago

Thank you for the PR! It works as expected except in two cases:

  1. In "Labels": after clicking on the "Select" panel action: image

We should also call prepareHistory when changing the groupBy variable value

  1. In "Labels" and "Flame graph", when there are both the "Service" and the "Profile type" dropdowns, here's a short video: https://github.com/user-attachments/assets/e18ac321-0d42-4a11-8b36-62a7cffeb338
ifrost commented 2 weeks ago

Thanks @grafakus πŸ™Œ

  1. In "Labels": after clicking on the "Select" panel action: We should also call prepareHistory when changing the groupBy variable value

Great catch! I've added prepping history directly toselectLabel. It's easy to make errors like this, though and forgot that variable may be change in other place, though :( I wonder if there's some other pattern that could ensure all user actions go via the same route (e.g. I noticed that select label has no user tracking, should we add it?)

Should be fixed by 584db4b03be598ca415cacded567e59f273e0069

  1. In "Labels" and "Flame graph", when there are both the "Service" and the "Profile type" dropdowns.

Another nice catch! Turns out when variables are updated from the URL both loading and value property are updated straight away and options are loaded with a delay. However, once options are loaded we do not re-render the Cascader (it's forced only when value and loading prop change). It all feels a bit hacky but seems like it's all because of the way Cascader works. There's no way to control it and pass new options and value without forcing a re-render.

Changed in e55d05e6230fb9da6ed19fe8dab9fbe0b711dc5e

Alternatively we can also revert adding a new history for service and profile for now and think how to unhack Cascader first πŸ€”

grafakus commented 2 weeks ago

@ifrost thank you for making the changes!

I noticed that select label has no user tracking, should we add it?

The tracking is centralized in SelectAction

Alternatively we can also revert adding a new history for service and profile for now and think how to unhack Cascader first πŸ€”

I suppose what's lacking is to be able to control the Cascader value... In the meantime, if we end up creating a key by concatenating several values and stringifying options, I'd rather go with a simpler hack to force a render each time:

    return (
      <Cascader
        // TODO: ...
        key={Math.random()}
...

It shouldn't be a big perf issue, wdyt?

ifrost commented 2 weeks ago

It shouldn't be a big perf issue, wdyt?

It should be fine. We may get some additional renders (2 or 3) on first paint but shouldn't have a big impact. Essentially we need to re-render Cascader when value, loading state or options change which is anyway is the only props that cause the re-render of the parent component (and hence Cascader too) so it should have almost identical outcome. Updated in c05ef5792110000c5ed3e83a816c99986ef0bcfa

grafakus commented 2 weeks ago

Thank you for helping on this @ifrost!