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

Supporting VictoriaMetric's Extended PromQL #253

Open
AeroNotix opened this issue Dec 4, 2019 · 38 comments
Open

Supporting VictoriaMetric's Extended PromQL #253

AeroNotix opened this issue Dec 4, 2019 · 38 comments

Comments

@AeroNotix
Copy link

There are some nice convenience functions here: https://github.com/VictoriaMetrics/VictoriaMetrics/wiki/ExtendedPromQL which make writing queries, alerts etc much simpler.

When I try to use them with promxy, I get errors such as:

Error executing query: invalid parameter 'query': parse error at char 3: unknown function with name "ru" for a query such as: ru(kubelet_volume_stats_available_bytes, kubelet_volume_stats_capacity_bytes).

I understand you are using the underlying prometheus libraries for a lot of the query layer and it's likely that layer which is bubbling this error - but would promxy consider adding support for VM's extended PromQL?

@jacksontj
Copy link
Owner

I'd definitely consider it, I actually have an issue upstream (VictoriaMetrics/VictoriaMetrics#56) to move the extended promql stuff into a more re-usable library. At this point I don't have the driving use case to spend my time rearchitecting that library to work with promql's types, but I'm definitely for it being in there eventually, PRs are definitely welcome!

@AeroNotix
Copy link
Author

Cool, worthwhile changing the labels to feature or similar?

@jacksontj
Copy link
Owner

Done :)

@valyala
Copy link

valyala commented Dec 9, 2019

FYI, we already have a PR that moves extended PromQL code into a separate library - VictoriaMetrics/VictoriaMetrics#256

@valyala
Copy link

valyala commented Dec 25, 2019

FYI, PromQL parser from VictoriaMetrics has been extracted into a standalone library - see docs.

@jacksontj
Copy link
Owner

From a quick look it'll require some more work to be supported in promxy, as promxy actually uses a forked version of promql with support for NodeReplacer (a mechanism to rewrite the query tree post-parse pre-execution). I have patches to add that functionality on prometheus that I maintain, presumably we could add something similar here.

@valyala
Copy link

valyala commented Dec 26, 2019

This sounds good!

@valyala
Copy link

valyala commented Jun 20, 2020

FYI, the MetricsQL parser has been moved to a separate repository with its own release schedule - https://github.com/VictoriaMetrics/metricsql . This should significantly simplify its' integration into third-party projects such as Promxy.

@jeromegn
Copy link

I'd be very interested in this.

@jacksontj if you can point me at the right bits of code, I'm sure i could come up with a PR.

My company would be happy to sponsor this issue if you'd prefer that.

@jacksontj
Copy link
Owner

The place that I think we'd want to implement this is to implement the Engine interface (https://github.com/jacksontj/promxy/blob/master/cmd/promxy/main.go#L188) and then add an option to swap between those.

TBH I haven't spent a lot of time looking into it so I'm not sure exactly how difficult it'll be. I'd be more than happy to review a PR and if you are interested in other options we can discuss those as well :)

@sdryga
Copy link

sdryga commented Aug 5, 2020

I use Promxy in conjunction with VictoriaMetrics. I really lack the support of MetricsQL in Promxy. I'll be glad to see this in the next release. Hope it'll be implemented soon! )

@davidsome
Copy link

davidsome commented Dec 21, 2020

@jacksontj I use Promxy in conjunction with VictoriaMetrics too. I really lack the support of MetricsQL in Promxy. I'll be glad to see this in the next release. Hope it'll be implemented soon! )

@dnull88
Copy link

dnull88 commented Mar 24, 2021

+1 for supporting of MetricsQL in Promxy

@IrekFasikhov
Copy link

+1 for supporting of MetricsQL in promxy

@IrekFasikhov
Copy link

@jacksontj Hi, Thomas. Need some help for the implementation?

@ihard
Copy link

ihard commented Jul 21, 2021

+1 very demanded functionality

@jc16180
Copy link

jc16180 commented Jul 27, 2021

+1 Would love to see this implemented. Only thing stopping me from switching to promxy. 🙂

@anvpetrov
Copy link

+1 for supporting of MetricsQL in promxy

1 similar comment
@autokilla47
Copy link

+1 for supporting of MetricsQL in promxy

@valdis932
Copy link

valdis932 commented Aug 10, 2021

+1

@AeroNotix
Copy link
Author

yikes, lads, +1 city in here.

Pull out the editor and get to work. Sheesh.

@AeroNotix
Copy link
Author

#253 (comment) @jacksontj where are these patches?

@G-Asura
Copy link

G-Asura commented Aug 17, 2021

+1 for supporting of MetricsQL in promxy

1 similar comment
@sc7565
Copy link

sc7565 commented Aug 25, 2021

+1 for supporting of MetricsQL in promxy

@tiagosanto737
Copy link

+1 for supporting MetricsQL in promxy. This is extremely important for us to be able to use promxy in our production environment that has multiple victoriametrics clusters. We have 1000s of dashboards built using promQL and metricsQL and can not change them.

@hagen1778
Copy link

Does anyone work on this? If no, I'd like to take it)

@V1pr
Copy link

V1pr commented Jan 4, 2023

Does anyone work on this? If no, I'd like to take it)

@valyala were you able to make any progress? :)

@psalaberria002
Copy link

Related, but not the same. With the 0.79.0 release we are getting the following error when querying absent_over_time against a Victoria Metrics backend.

expanding series: remote server http://vmselect.victoriametrics:8481/select/1/prometheus/api/v1/read returned HTTP status 400 Bad Request: remoteAddr: "10.24.11.33:51258"; requestURI: /select/1/prometheus/api/v1/read; unsupported path requested: "/select/1/prometheus/api/v1/read"

I understand VM doesn't support remote read. Any workaround?

@oOHenry
Copy link

oOHenry commented Apr 13, 2023

I understand VM doesn't support remote read. Any workaround?

set remote_read: false in promxy config ;)

@mohammadkhavari
Copy link

Hey, Is it still in progress? @valyala @jacksontj

@sergeyshevch
Copy link

Hi! We're very interested in this feature and I will try to make PR within the next few days. @jacksontj Can you please guide me to the right entry point for such modifications?

Is this still relevant? (3 years after) #253 (comment)

@sergeyshevch
Copy link

After codebase research, I've found the following points.

  • We need to update the downstream Prometheus code to accept the QueryEngine interface in the WebOptions struct and in internal packages. (I will create PR for this)
  • We need to implement QueryEngine for VictoriaMetrics. As I see we mostly need to bypass some checks of promql.Parser and use MetricsQL validation instead.
  • The MetricsQL package has huge differences from the PromQL parser so it's hard to implement the query engine

It will be great if VictoriaMetrics has some QueryEngine implementation on their own like the Thanos project.
https://github.com/thanos-io/promql-engine

@isodude
Copy link

isodude commented Dec 7, 2023

Does it not make sense to just support MetricsQL out of the box, but only if downstream targets support it?
I.e. adding a QueryLanguage variable to the ServerGroup model. If the query is to be sent to that one, MetricsQL is used to parse.

@jacksontj
Copy link
Owner

@isodude to be clear here; it would be nice to add support, but as @sergeyshevch mentioned there are a few issues with doing so.

In addition to the list @sergeyshevch mentioned (specifically adding support for a different engine in Options). We'd also need to add support for a NodeReplacer type interface -- as this is the main "secret sauce" that makes promxy performance what it is. This is what requires me to maintain a fork of prometheus (as it doesn't support this either).

@dvnscr
Copy link

dvnscr commented Oct 28, 2024

Is there any hope for this to happen ?

@isodude
Copy link

isodude commented Dec 27, 2024

@dvnscr I implemented with VictoriaMetrics cluster setup instead and the performance is much better since it's using a binary protocol after vmselect. I don't know if the effort of coaxing support into this project is worth it. There will be a lot of considerations to be made, since it differs in a lot of small details and is moving a bit faster.

It's actually breaking API-wise for /query by allowing '&step=' as a parameter.

@dvnscr
Copy link

dvnscr commented Dec 27, 2024

@isodude Yeah, thanks, i am thinking about switching Single Victoria variant to Cluster stack on the single node per region for now. It will become future proof if i need to scale it horizontaly too 👍🏻

@jacksontj
Copy link
Owner

the performance is much better since it's using a binary protocol after vmselect

I am actually curious about this -- what protocol is VM using for their internal transfer? If they have a different API to "do the same" we could wrap that into a compatible interface with relative ease (if thats a notable perf gain).

I don't know if the effort of coaxing support into this project is worth it.

This seems to be the current thinking so far -- because the engine API for prometheus and VM are wildly different. I am not opposed to someone tackling this issue; but I don't run VM personally or professionally so I don't have a vested interest in making the time to add this myself. So definitely open to adding this; but this seems like a lot of effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests