Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pyroscope: Template variable support #73572

Merged
merged 14 commits into from
Aug 30, 2023
Merged

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented Aug 21, 2023

Adds template variable support for pyroscope. This includes

  • profile or application type variable
  • label variable
  • label values variable

Variables in dashboard settings
Screenshot 2023-08-23 at 17 29 37

Variable editor
Screenshot 2023-08-23 at 17 29 49

Dashbard with variables
Screenshot 2023-08-23 at 17 30 00

Query editor
Also one change to make this work was to make the profile selector in pyroscope query editor work with custom values:
Screenshot 2023-08-23 at 17 45 56

testing

Use pyroscope or phlare dev env block and create dashboard creating variables similar to those in screenshots.

@aocenas aocenas marked this pull request as ready for review August 23, 2023 15:44
@aocenas aocenas requested review from a team as code owners August 23, 2023 15:44
@aocenas aocenas requested review from L-M-K-B, eledobleefe and Rperry2174 and removed request for a team, L-M-K-B and eledobleefe August 23, 2023 15:44
@aocenas aocenas added add to changelog add to what's new datasource/grafana-pyroscope Grafana pyroscope datasource (previously Phlare) no-backport Skip backport of PR labels Aug 23, 2023
@@ -113,7 +115,7 @@ export class Cascader extends PureComponent<CascaderProps, CascaderState> {
for (const option of searchableOptions) {
const optionPath = option.value || [];

if (optionPath.indexOf(initValue) === optionPath.length - 1) {
if (optionPath[optionPath.length - 1] === initValue) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@tskarhed 2 small fixes here. This could have failed to select option if main and child option have the same value. Seems easier to just test the leaf and compare that.

Also the separator had spaces around when used for custom values vs for other values so unified that.

@@ -0,0 +1,77 @@
import React, { useEffect, useMemo, useState } from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of this was just moved to separate component so it can be reused in variable editor

Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

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

Nice touch to allow variables within the VariableQueryEditor 👍

  • Clicking an item in the cascader updates the value even if it's incomplete. I would expect this for a leaf value such as process_cpu-samples but if I'm just clicking memory or mutex for example, I may only be taking a look at the options, rather than wanting to actually change the profileType. I would have to navigate back to my previous selection after clicking on anything in the cascader, if my intent was not actually to change the value. Is there a specific reason we update the cascader when any option is clicked? If it's so users can see their selection/breadcrumb I wonder could we update the UI when they click an options but not update the actual state until they have finished their selection down to the leaf?

  • Error if I don't select Profile type or Label. Error seems expected as no values to map but still should be handled gracefully.
    Screenshot 2023-08-29 at 09 44 35

  • Other than those few comments, this looks really good!
    Screenshot 2023-08-29 at 09 55 55

@aocenas
Copy link
Member Author

aocenas commented Aug 29, 2023

@joey-grafana

Clicking an item in the cascader updates the value even if it's incomplete.

Fixed.

Error if I don't select Profile type or Label.

Fixed. Seems like other data sources just don't run query if some data is missing so done the same.

@aocenas aocenas merged commit a6ff503 into main Aug 30, 2023
17 checks passed
@aocenas aocenas deleted the aocenas/pyroscope/template-variables branch August 30, 2023 08:48
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog add to what's new area/frontend datasource/grafana-pyroscope Grafana pyroscope datasource (previously Phlare) no-backport Skip backport of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants