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

feat(datasource-plugin): add support for label queries #1877

Merged
merged 6 commits into from
Mar 3, 2023

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Feb 24, 2023

The implementation and query syntax is similar to the Prometheus datasource for convenience: https://grafana.com/docs/grafana/latest/datasources/prometheus/template-variables/#use-query-variables

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 1006.86 KB (0%) 20.2 s (0%) 4 s (+6.72% 🔺) 24.1 s
webapp/public/assets/app.css 19.9 KB (0%) 398 ms (0%) 0 ms (+100% 🔺) 398 ms
webapp/public/assets/styles.css 9.6 KB (0%) 192 ms (0%) 0 ms (+100% 🔺) 192 ms
packages/pyroscope-flamegraph/dist/index.js 632.77 KB (0%) 12.7 s (0%) 2.1 s (+39.19% 🔺) 14.7 s
packages/pyroscope-flamegraph/dist/index.node.js 633.47 KB (0%) 12.7 s (0%) 744 ms (+4.68% 🔺) 13.5 s
packages/pyroscope-flamegraph/dist/index.css 8.11 KB (0%) 163 ms (0%) 0 ms (+100% 🔺) 163 ms

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

Flamegraph.com report

pyroscope-oss.alloc_objects.json
See in flamegraph.com
pyroscope-oss.alloc_space.json
See in flamegraph.com
pyroscope-oss.cpu.json
See in flamegraph.com
pyroscope-oss.goroutines.json
See in flamegraph.com
pyroscope-oss.inuse_objects.json
See in flamegraph.com
pyroscope-oss.inuse_space.json
See in flamegraph.com

Created by Flamegraph.com Github Action

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2023

Flamegraph.com report

pyroscope-oss.frontend.cpu.json
See in flamegraph.com
pyroscope-oss.frontend.inuse_objects.json
See in flamegraph.com
pyroscope-oss.frontend.inuse_space.json
See in flamegraph.com

Created by Flamegraph.com Github Action

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (4129a30) 63.28% compared to head (5e6cfc7) 63.28%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1877   +/-   ##
=======================================
  Coverage   63.28%   63.28%           
=======================================
  Files         175      175           
  Lines        5912     5912           
  Branches     1587     1587           
=======================================
  Hits         3741     3741           
  Misses       2162     2162           
  Partials        9        9           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kolesnikovae kolesnikovae force-pushed the feat(datasource-plugin)/label_queries branch from 5e2445a to 85bdb39 Compare February 24, 2023 18:30
@kolesnikovae kolesnikovae force-pushed the feat(datasource-plugin)/label_queries branch from 85bdb39 to 3b5c43e Compare February 24, 2023 18:40
@kolesnikovae kolesnikovae marked this pull request as ready for review February 24, 2023 18:54
@kolesnikovae kolesnikovae requested a review from eh-am February 24, 2023 18:54
@kolesnikovae
Copy link
Collaborator Author

kolesnikovae commented Feb 24, 2023

@eh-am if you have a chance, please give the PR a look – after our discussion, I made an attempt to implement label queries, and I wonder if it is sufficient for our needs

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

This is pretty sweet!

Screen Shot 2023-02-24 at 20 13 26

Screen Shot 2023-02-24 at 20 21 24

Kapture.2023-02-24.at.20.18.53.mp4

One thing I find necessary though is to be able to choose no tag. In the example above I may want to look at simple.app.cpu{}, it would be nice to have some kind of catch all using .*, but that would require updating the endpoints etc I assume.

One last thing is that we can't forget about updating the README with this new functionality.

packages/pyroscope-datasource-plugin/src/datasource.ts Outdated Show resolved Hide resolved
@eh-am
Copy link
Collaborator

eh-am commented Feb 26, 2023

One thing I find necessary though is to be able to choose no tag. In the example above I may want to look at simple.app.cpu{}, it would be nice to have some kind of catch all using .*, but that would require updating the endpoints etc I assume.

One idea is to in the datasource, if both sides of the = are empty, to just discard it altogether? That way, if you select the 2 variables to be an empty value, it will be translated myapp{=} -> myapp{}

@kolesnikovae
Copy link
Collaborator Author

kolesnikovae commented Feb 27, 2023

One thing I find necessary though is to be able to choose no tag. In the example above I may want to look at simple.app.cpu{}, it would be nice to have some kind of catch all using .*, but that would require updating the endpoints etc I assume.

One idea is to in the datasource, if both sides of the = are empty, to just discard it altogether? That way, if you select the 2 variables to be an empty value, it will be translated myapp{=} -> myapp{}

Yeah, also was thinking about this. But honestly I doubt variable substitution mechanism in Grafana is good/designed for this – I can picture one may want to use multiple labels, use different operators, have a sophisticated logic of label dependencies, etc. In general, I can't even find a good example when to use label_names. I think we should implement a proper query builder instead.

I was playing with various scenarios and discovered that repeating panel or row does not seem to work as expected (both for apps and label values) – when I specify multiple values, our datasource gets an array of values:

image

The variable:
image

The panel:
image

What do you think? Is it what we should handle in our panel plugin / query editor or I did miss something?

@kolesnikovae
Copy link
Collaborator Author

kolesnikovae commented Feb 27, 2023

Ok, I figured out how to make it work with multi-variable choice:
image

@kolesnikovae kolesnikovae force-pushed the feat(datasource-plugin)/label_queries branch from 4abbb6c to c9f95f0 Compare February 27, 2023 15:04
@kolesnikovae kolesnikovae force-pushed the feat(datasource-plugin)/label_queries branch from 41bc990 to 9aa53f4 Compare February 27, 2023 17:34
@kolesnikovae kolesnikovae merged commit e877f85 into main Mar 3, 2023
@kolesnikovae kolesnikovae deleted the feat(datasource-plugin)/label_queries branch March 3, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants