grafana / pyroscope

Continuous Profiling Platform. Debug performance issues down to a single line of code
https://grafana.com/oss/pyroscope/
GNU Affero General Public License v3.0
9.79k stars 583 forks source link

Persist UI preferences #707

Closed eh-am closed 2 years ago

eh-am commented 2 years ago

Related to https://github.com/pyroscope-io/pyroscope/issues/591

It would be nice to have certain data persisted across page refreshes:

We can assume that their preferences will hold for ALL pages, so we only need to store/persist the value once.

This means, for example, that we don't need to store fitMode individually for single view/comparison/diff views. If I set "Tail first" for single view, that should be carried over the other pages, like the Diff one. Same for table | both | flamegraph view.

It's still a good idea to model in such a way similar to the sidebar, where we actively distinguish that the user HAS set a value (as opposed to using the default). That is so that once we can change the default once we add more modes (eg if fitMode is pristine, show the new default)

One challenge is the comparison mode, we should still allow the 2 flamegraphs to have different values: image Notice how one uses Head and another uses Tail. We will need a certain logic that prefers whatever the local state is, and only fallback to the "store" one.


One side effect of moving that to the store is that it makes it somewhat easier to implement https://github.com/pyroscope-io/pyroscope/issues/606

Suffice to say, this should still work with PYROSCOPE_ENABLE_EXPERIMENTAL_ADHOC_UI=true which should be released soon

eh-am commented 2 years ago

@shaleynikov asked me about the implementation

https://github.com/pyroscope-io/pyroscope/blob/main/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphRenderer.tsx#L109-L112

2 options: option 1:

option 2:

shaleynikov commented 2 years ago

@eh-am since Jan 11, flamegraph has been moved to a separate package, so I don't see how we can't follow same approach as we had with sidebar. What's the best way of accomplishing it now?

I see it like adding this https://github.com/astoilkov/use-local-storage-state to an internal state variables. What do you think?

eh-am commented 2 years ago

Not relevant at this point.