Flamegraph: Move to package #73113
Flamegraph: Move to package #73113
Conversation
aocenas
force-pushed
the
aocenas/flamegraph/package
branch
from
5c61b8b
to
68e7fc5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // the language from the backend and use the right regex but right now we just try all of them from most to least | ||
| // specific. | ||
| const matchers = [ | ||
| ['phpspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.php+)(?<line_info>.*)$/], |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| // specific. | ||
| const matchers = [ | ||
| ['phpspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.php+)(?<line_info>.*)$/], | ||
| ['pyspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.py+)(?<line_info>.*)$/], |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| const matchers = [ | ||
| ['phpspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.php+)(?<line_info>.*)$/], | ||
| ['pyspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.py+)(?<line_info>.*)$/], | ||
| ['rbspy', /^(?<packageName>(.*\/)*)(?<filename>.*\.rb+)(?<line_info>.*)$/], |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
|
(Open the links below in a new tab to go to the correct steps)
|
github-actions
bot
added
the
levitate breaking change
|
Shouldn't this live in |
|
can't it be used via PanelRenderer or scenes VizPanel? |
|
Sorry for late reply:
I talked to front end platform and they were quite strongly against adding more visualization into grafana/ui package. Also if we want to speed up the development of plugins we probably want to make them less dependant on bigger packages like grafana/ui which may have less frequent releases.
This should be used both in app plugin but also in separate pyroscope web app (and maybe in flamegraph.com) which do not have any grafana runtime stuff. |
|
@academo sorry just now pushed the readme |
|
@aocenas I would love to have a clear why not And @staton-hyse11 would be interesting to hear what you think? Additionally please add your squad as owners for this package after this line Line 325 in 433c76f Hooking into publishing pipeline should be done by frontend platform I guess, probably some CI scripts need to be adjusted. |
|
@tolzhabayev I added some more context to the description in this PR |
|
The question of putting this into grafana/ui was discussed separately and we agreed to continue with the PR as it is. |
| data={props.dataFrames[0]} | ||
| stickyHeader={true} | ||
| getTheme={() => config.theme2} | ||
| onTableSymbolClick={() => interaction('table_item_selected')} |
There was a problem hiding this comment.
To remove grafana/runtime the interaction reporting is injected like this now.
| <FlameGraph | ||
| data={props.dataFrames[0]} | ||
| stickyHeader={true} | ||
| getTheme={() => config.theme2} |
There was a problem hiding this comment.
Injected theme so we don't rely on having it in the react context
There was a problem hiding this comment.
Will there be cases where the flamegraph will run without the Theme context? Is the idea that this package can be used outside of Grafana?
There was a problem hiding this comment.
Yes in the pyroscope web app.
| value={selectedView} | ||
| onChange={setSelectedView} | ||
| /> | ||
| {extraHeaderElements && <div className={styles.extraElements}>{extraHeaderElements}</div>} |
There was a problem hiding this comment.
Extra elements rendered here. In pyroscope used to inject a button to download the profile in various formats.
|
Works locally with testdb & pyroscope/phlare backend, but do you have more details to test this in the app? |
Moving the Flamegraph code into a separate package so it can bu published in NPM and used outside of Grafana. Main usecase right now is the Pyroscope web app:
See the draft PR in the pyro web app for usage.
Main part of the changes here are obviously that the code of the flamegraph was moved from the core grafana to a separate
package/directory.There are also some other notable changes I will also try to highlight in the code:
grafana/runtime. This does not make sense in a component package anyway and was used mainly for interaction reporting. This was moved to an injectable hooks that fire on specific interactions.getThemeprop so theme can be injected instead of relying onuseThemeand it's access to theme in react context.stickyHeaderprop to control the header css. Before it relied on knowing which part of Grafana was this used in which again does not make sense in a package.extraHeaderElements+verticalprops. Allows more customization for usecases inside the pyroscope app.Wider context
why new package?
We want to unify the flamegraph components across grafana and Pyroscope web app. Because of that we need to be able to import the component into non grafana envirnment.
why not adding it to grafana/ui
The grafana/ui is probably wider than it should be. Adding visualizations would make it too big to act as a general component library. Visualizations also mostly act as a small self contained applications so may not fit the same abstraction. Also we want to eventually have the flexibility of having separate release for visualizations panel plugins and if they depended on grafana/ui with most of their code they would still be tied to it.
Testing
The basic test would be to see if the flamegraph still works ok inside grafana either with devenv pyroscope block or using test data source.
To see it used in Pyroscope app this is a bit harder right now. You have to setup local npm server and publish the package there. See the packages readme for instructions, then you can use the Pyroscope PR, install it there and try it out.