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

Prometheus: Add native histogram functions #86002

Merged
merged 10 commits into from
Apr 15, 2024

Conversation

bohandley
Copy link
Contributor

@bohandley bohandley commented Apr 11, 2024

Fixes #76111

Be aware!!!
These functions will not parse correctly when switching from code editor. This won't work until the @prometheus-io/lezer-promql is updated here.

This adds functions for Prometheus native histograms. It provides auto complete for the following functions:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@bohandley bohandley added this to the 11.1.x milestone Apr 11, 2024
@bohandley bohandley self-assigned this Apr 11, 2024
@bohandley bohandley requested a review from a team as a code owner April 11, 2024 20:38
Copy link
Contributor

@itsmylife itsmylife left a comment

Choose a reason for hiding this comment

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

LGTM
Builder -> Code is working
Code -> Builder is not
But with lezer-promql update it will work.

Comment on lines 293 to 314
it('parses a native histogram function correctly', () => {
expect(
buildVisualQueryFromString('histogram_count(rate(counters_logins{app="backend"}[$__rate_interval]))')
).toEqual({
errors: [],
query: {
metric: 'counters_logins',
labels: [{ label: 'app', op: '=', value: 'backend' }],
operations: [
{
id: 'rate',
params: ['$__rate_interval'],
},
{
id: 'histogram_quantile',
},
],
},
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

This will always fail because lezer-promqlv0.37.0-rc.1 has no idea about new histogram functions. So you can skip it and I'll enable it in my PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried your changes on my PR and it works!

Screen.Recording.2024-04-15.at.19.09.03.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course there is no graph available because the metric I used is not native histogram. I picked it just for the sake of the demo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I wrote it just to have a test ready for when we do get it in. I will remove it and you can add it to your PR. Go Lezer parser update!!! Great job Ismail!!! 🏆

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say don't remove it. But just mark it to skip. You can define it as
xit('parses a native... so it will be ignored
^^^___________________________________

@bohandley bohandley force-pushed the bohandley/add-native-histogram-functions branch from 2f6e666 to d13a5b7 Compare April 15, 2024 19:07
@bohandley bohandley merged commit 7c15627 into main Apr 15, 2024
18 checks passed
@bohandley bohandley deleted the bohandley/add-native-histogram-functions branch April 15, 2024 19:26
xlson added a commit that referenced this pull request Apr 16, 2024
* main: (482 commits)
  I18N: Split loadTranslations i18next plugin into seperate file (#86217)
  Grafana UI: `StackingEditor` and `addHideFrom` - Replace `HorizontalGroup` with `Stack` (#86192)
  Dashboard: Migration -  Add public dashboard tag in the nav bar (#86204)
  mssql: decouple sqleng (#86130)
  Docs: Fix typos (#85651)
  Playlists: Enable kubernetesPlaylists by default in OSS (#86259)
  DashboardScene: Fixes saving dashboard with angular panels  (#86098)
  DashboardScene: Fix empty row repeat issue (#86095)
  Prometheus: Use the frontend package in Prometheus and remove feature toggle (#86080)
  docs: add annotations play link (#86206)
  Chore: add missing modowner (#86234)
  docs: add thresholds play link (#86212)
  docs: added logs panel play link; added dedup options, reformatted fig (#86209)
  docs: query data/relative time range override play link (#86213)
  Docs: added bar gauge play link (#86205)
  Docs: add template var Play link; cleanup existing (#86074)
  Prometheus: Add native histogram functions (#86002)
  K8s: add ID token to requests outbound to new query endpoint (#86214)
  Prometheus: Respect dashboard queries when querying ad hoc filter labels (#85674)
  Chore: Use `handleReducedMotion` to smooth the animation in `LoadingBar` (#85808)
  ...
@kevinwcyu kevinwcyu modified the milestones: 11.1.x, 11.1.0 Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new PromQL native histogram functions
3 participants