Skip to content
This repository has been archived by the owner on Jul 19, 2023. It is now read-only.

feat(frontend): new app selector #712

Merged
merged 17 commits into from
May 30, 2023
Merged

feat(frontend): new app selector #712

merged 17 commits into from
May 30, 2023

Conversation

eh-am
Copy link
Collaborator

@eh-am eh-am commented May 24, 2023

Related grafana/pyroscope#2055

Example (updated)

Screen.Recording.2023-05-29.at.17.32.48.mov

It also supports mixing and matching pyroscope_app and __service_name__:

Screen.Recording.2023-05-30.at.11.33.02.mov

Features

  • It adds an Application Selector which leads user to make a more targeted query by default, similar to what OG Pyroscope does.
  • If the user still wants to write their own (complex) queries, it's still possible.
  • It supports both pyroscope_app and __service_name__ as "indexes"

Caveats/Limitations

  • It uses the /querier.v1.QuerierService/Series to create a list of apps, which returns more data than needed.
  • Also it only returns data that's in memory, ie. recently ingested apps. Related to [ui] 'ProfileID/Applications' only shows data that has been ingested recently pyroscope#2071
  • Parsing an "App" from a query does not work when the query is non trivial (eg uses of !~ etc). In this PR it doesn't make too much difference, as the only thing that happens is not populating the dropdown correctly (which to be honest, it shouldn't, after all, a query like cpu{app="myapp.*"} doesn't really match to an "App"
  • It does not preserve tags when changing apps. For example, given a current query cpu{mytag="foo", pyroscope_app="myapp"} and clicking on myapp2, even if myapp2 shares the exact tags, the new query will completely strip mytag="foo": cpu{pyroscope_app="myapp2"}
  • The pyroscope_app/__service_name__ tag is present in the app selector.
  • There's no filtering mechanism like in OG Pyroscope.

Comment on lines +72 to +80
const response = await requestWithOrgID('/querier.v1.QuerierService/Series', {
method: 'POST',
body: JSON.stringify({
matchers: [],
}),
headers: {
'content-type': 'application/json',
},
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonswine I wonder if there isn't a better way to get this data...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite, more like if we shouldn't create an endpoint that does most of this work (create an "App" that combines most of its labels). Plus I don't know how bad it currently is performance wise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure what the method is supposed to achieve.

  • Do you just want to get a flat list of unique values for pyroscope_app (or soon __service_name__): Then you should rather use LabelValues:
    curl -vL -d '{"name":"__service_name__"}' -X POST -H "content-type: application/json" http://localhost:4100/querier.v1.QuerierService/LabelValues
    It also supports matchers just like Series
  • Or do you need the full label set?: Then series is the way to go

Copy link
Collaborator Author

@eh-am eh-am May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need the full label set, since I need __service_name__ + profileID (to be able to create a "query") + other metadata fields, like __type__ and __name__ (to be able to display more info about what you are selecting) :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see then maybe we should have a specific endpoint that allows that better.

Two ideas:

  • Series but with the ability to give it the labels we are interested in e.g. labelNames=[...] (I prefer that)
  • LabelValues with the ability to give it multiple labelNames

I would prefer both over a specialized endpoint for the menu

I think in the meantime it is probably good enough with series. Speaking with cyril earlier today we also discussed how maybe we would like to change the navigation longer term (let's catch-up on this later)

@eh-am eh-am changed the title [WIP] feat(frontend): new app selector feat(frontend): new app selector May 30, 2023
@eh-am eh-am marked this pull request as ready for review May 30, 2023 11:01
Copy link
Contributor

@Rperry2174 Rperry2174 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!! Thanks @eh-am

@Rperry2174 Rperry2174 merged commit b3aea2d into main May 30, 2023
17 checks passed
@Rperry2174 Rperry2174 deleted the feat/app-selector branch May 30, 2023 15:55
simonswine pushed a commit to simonswine/pyroscope that referenced this pull request Jun 30, 2023
Features:
* Adds an Application Selector to enable users to make more targeted queries by default, similar to OG Pyroscope.
* Users can still write their own complex queries if desired.
* Supports both "pyroscope_app" and "service_name" as indexes.

Caveats:
* Utilizes the /querier.v1.QuerierService/Series to create a list of apps, which returns more data than necessary.
* Only returns data that is currently in memory, specifically recently ingested apps. (Related to [ui] 'ProfileID/Applications' only shows data that has been ingested recently https://github.com/grafana/phlare/issues/630)
* Parsing an "App" from a non-trivial query (e.g., using !~) does not function correctly. In this PR, it primarily affects the dropdown population, which should not match a query like "cpu{app="myapp.*"}" to an "App" accurately.
* Does not preserve tags when switching between apps. For example, if the current query is "cpu{mytag="foo", pyroscope_app="myapp"}" and the user clicks on "myapp2", even if "myapp2" shares the exact tags, the new query will completely remove "mytag="foo"": "cpu{pyroscope_app="myapp2"}".
* The "pyroscope_app/service_name" tag is present in the app selector.
* There is no filtering mechanism similar to OG Pyroscope.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants