Page MenuHomePhabricator

Cumin feature idea: Prometheus backend
Open, Needs TriagePublic

Description

Yesterday we needed to roll-restart a few hhvm on api appservers. Since the list of servers affected could be also inferred from certain metrics, this gave me the idea to have a Prometheus backend for Cumin. Namely the backend would yield a list of hostnames extracted from the result of a Prometheus query.

A simpler way, and perhaps more decoupled, would be to have Cumin accept a list of hostnames in input and have a separate script/component generate said list from Prometheus query results.

Event Timeline

Thanks for the proposal. It seems to me a nice to have backend, I don't see any conceptual problem with its addition to cumin's backends. For example also other non-configuration backends like Icinga in our case might also be useful sometimes, etc.

A simpler way, and perhaps more decoupled, would be to have Cumin accept a list of hostnames in input and have a separate script/component generate said list from Prometheus query results.

This can already be achieved as long as you can gather the FQDNs from anything external to cumin and pass them into cumin as a comma-separated list, either using the direct backend D{hostA.domain,hostB.domain,...} or also with the PuppetDB backend (with some limitation on the query size when using API v3 as we're currently doing) that is our default: hostA.domain,hostB.domain,....

hi! i've been looking into this again and I think i might start looking at making a backend for Prometheus myself. It seems the first step would be to design a grammar for the Prometheus queries that wouldn't conflict with the existing selectors.

It seems like the P: namespace is already taken by the Puppet backend, what could I possibly use instead? T: maybe?

From there on, I think it's fairly smooth sailing: you'd craft a Prometheus query that would return a list of hosts. In my case, one query I'd like to look at is something like:

count(node_reboot_required>0) by (alias)

...which returns something like this:

{alias="cdn-backend-sunet-01.torproject.org"}	1
{alias="hetzner-hel1-02.torproject.org"}	1
{alias="onionoo-backend-01.torproject.org"}	1

A similar query would be with apt_upgrades_pending>0 but the possibilities are basically endless from this point...

The backend would probably need some logic to guess the hostname - you'd probably need to tell it about that alias somehow, but that seems like something that's easy to handle. You also need to pass it the prometheus API end point and possibly authentication details, but those are likely embedded in a single URL.

What do you think?

hi! i've been looking into this again and I think i might start looking at making a backend for Prometheus myself. It seems the first step would be to design a grammar for the Prometheus queries that wouldn't conflict with the existing selectors.

@TheAnarcat, first of all thanks a lot for looking into this, patches are always welcome if you want to contribute!

It seems like the P: namespace is already taken by the Puppet backend, what could I possibly use instead? T: maybe?

I don't have strong preferences, T might be ok, also H might be a possible choice I guess.

From there on, I think it's fairly smooth sailing: you'd craft a Prometheus query that would return a list of hosts. In my case, one query I'd like to look at is something like:

count(node_reboot_required>0) by (alias)

...which returns something like this:

{alias="cdn-backend-sunet-01.torproject.org"}	1
{alias="hetzner-hel1-02.torproject.org"}	1
{alias="onionoo-backend-01.torproject.org"}	1

A similar query would be with apt_upgrades_pending>0 but the possibilities are basically endless from this point...

Are you suggesting to keep the grammar super basic and let the user write any prometheus query?
Because the other approach is to have a grammar that could abstract and represent prometheus capabilities, but I'm worried that it would be very hard to get it right and without limiting a lot its usage.
On the other side having to write prometheus queries is not that straightforward but people could also save the most common ones in cumin aliases.
I'm not a prometheus expert and would like also @fgiunchedi and @CDanis feedbacks on the best practices to get a list of hosts from prometheus queries.

Cumin's backends are basically divided into two groups, those that have an embedded capability of performing aggregation (and, or, not, etc..) and those that don't.
In this case given the vast complexity that a prometheus query can represent I'm tempted to say that the backend should not have aggregation capabilities and if a user needs them they can always use the general grammar to do that (e.g. (T:query1 and not T:query2) or T:query3).

The backend would probably need some logic to guess the hostname - you'd probably need to tell it about that alias somehow, but that seems like something that's easy to handle. You also need to pass it the prometheus API end point and possibly authentication details, but those are likely embedded in a single URL.

For the alias I guess some configuration in cumin might be enough to specify which field to look for if that's customizable.
For the endpoint and auth details it goes all into cumin configuration.

What do you think?

Another aspect to take into account are the dependencies. So far cumin has been built and deployed @WMF via debian package, that means that required dependencies must be available as debian package, ideally in debian upstream, but we could also import stuff in our APT repo, but the dependency should be already packageable.

I'm available to discuss more in-depth details and do a draft-structure of the backend before the development too if you intend to proceed.

Are you suggesting to keep the grammar super basic and let the user write any prometheus query?

Correct.

Because the other approach is to have a grammar that could abstract and represent prometheus capabilities, but I'm worried that it would be very hard to get it right and without limiting a lot its usage.

I don't know if that could make a lot of sense. Prometheus already has its own funky query language grammar, reproducing (a part of) it in *another* grammar would seem like a waste of time to me.

On the other side having to write prometheus queries is not that straightforward but people could also save the most common ones in cumin aliases.

I didn't think of Cumin aliases, but once you get familiar with Prometheus, writing those queries isn't that big of a deal. They're a few clicks away behind Grafana graphs and you get kind of "native" in speaking them.

In this case given the vast complexity that a prometheus query can represent I'm tempted to say that the backend should not have aggregation capabilities and if a user needs them they can always use the general grammar to do that (e.g. (T:query1 and not T:query2) or T:query3).

I agree. Prometheus already has its own boolean logic kind of thing, so we shouldn't have that in the backend grammar.

For the alias I guess some configuration in cumin might be enough to specify which field to look for if that's customizable.

Could *that* be in the grammar? Not sure how it would look like, but maybe, for example, we could "parse" the "by (alias)" thing above? Just an idea... Not sure it would actually work or if it would work at all. The problem is, if we *don't* add it to the grammar, it might not *always* make sense to have the "alias" in the configuration.

After all, "alias" is just one label, and there can be many. The way this works (for example) in Grafana is that the "label" is extracted from a given query using a grafana-specific function. For example, this would extract the alias label from the node_exporter_build_info query:

label_values(node_exporter_build_info, classes)

Or, to reuse our earlier example, I think *this* would also work:

label_values(count(node_reboot_required>0) by (alias), alias)

label_values is the grafana-custom specific thing. The different functions grafana offers there are documented here.

My point is: there are many ways to do this. In my configuration, for example, I could even extract a *Puppet class* from a prometheus query. For example, I could extract the classes that need a reboot in my configuration with this:

label_values(count(node_reboot_required>0) by (classes), classes)

Then it would be pretty awesome if I could throw that thing in Puppet, *within* cumin... Not sure it would fly though...

Another aspect to take into account are the dependencies. So far cumin has been built and deployed @WMF via debian package, that means that required dependencies must be available as debian package, ideally in debian upstream, but we could also import stuff in our APT repo, but the dependency should be already packageable.

Prometheus is fairly well packaged in Debian. While we could also just talk to the API by crafting HTTP requests by hand (or with the requests package), there's a python client library that is available in Debian all the way back to stretch, so that should not be a problem.

I'm available to discuss more in-depth details and do a draft-structure of the backend before the development too if you intend to proceed.

That's great. I'm not sure I'll have time to move forward in the short term, but it's great to start seeing how it would work in practice. I'd definitely welcome some "scaffolding" or some template to get started with a backend...

Thanks for the feedback!

@TheAnarcat thanks indeed for taking the time to look into this!

Are you suggesting to keep the grammar super basic and let the user write any prometheus query?

Correct.

I agree, not having to inspect the Prometheus query if we can possibly avoid it seems best.

On a more detailed level: there will be at least one conflict in syntax I can think of right now, { and } can appear in both Cumin and Prometheus. I'm assuming the latter will need escaping (?)

Because the other approach is to have a grammar that could abstract and represent prometheus capabilities, but I'm worried that it would be very hard to get it right and without limiting a lot its usage.

I don't know if that could make a lot of sense. Prometheus already has its own funky query language grammar, reproducing (a part of) it in *another* grammar would seem like a waste of time to me.

On the other side having to write prometheus queries is not that straightforward but people could also save the most common ones in cumin aliases.

I didn't think of Cumin aliases, but once you get familiar with Prometheus, writing those queries isn't that big of a deal. They're a few clicks away behind Grafana graphs and you get kind of "native" in speaking them.

In this case given the vast complexity that a prometheus query can represent I'm tempted to say that the backend should not have aggregation capabilities and if a user needs them they can always use the general grammar to do that (e.g. (T:query1 and not T:query2) or T:query3).

I agree. Prometheus already has its own boolean logic kind of thing, so we shouldn't have that in the backend grammar.

Same here FWIW

For the alias I guess some configuration in cumin might be enough to specify which field to look for if that's customizable.

Could *that* be in the grammar? Not sure how it would look like, but maybe, for example, we could "parse" the "by (alias)" thing above? Just an idea... Not sure it would actually work or if it would work at all. The problem is, if we *don't* add it to the grammar, it might not *always* make sense to have the "alias" in the configuration.

After all, "alias" is just one label, and there can be many. The way this works (for example) in Grafana is that the "label" is extracted from a given query using a grafana-specific function. For example, this would extract the alias label from the node_exporter_build_info query:

label_values(node_exporter_build_info, classes)

Or, to reuse our earlier example, I think *this* would also work:

label_values(count(node_reboot_required>0) by (alias), alias)

label_values is the grafana-custom specific thing. The different functions grafana offers there are documented here.

I think a possible sidestep of this issue, at least in a first iteration, would be to ask the user to write queries that return only one label and use that label's values. If the values need massaging (e.g. to strip port numbers) then one solution might be to use label_replace in the query itself.

Another aspect to take into account are the dependencies. So far cumin has been built and deployed @WMF via debian package, that means that required dependencies must be available as debian package, ideally in debian upstream, but we could also import stuff in our APT repo, but the dependency should be already packageable.

Prometheus is fairly well packaged in Debian. While we could also just talk to the API by crafting HTTP requests by hand (or with the requests package), there's a python client library that is available in Debian all the way back to stretch, so that should not be a problem.

FWIW last I checked the python client is mostly to expose data, and doesn't deal with the HTTP API, IOW talking directly to the api would be the way to go.

Other things to consider off top of my head: there might be multiple Prometheus instances one would like to query (e.g. there's a Prometheus deployed in each datacenter in WMF's deployment).

HTH!

On a more detailed level: there will be at least one conflict in syntax I can think of right now, { and } can appear in both Cumin and Prometheus. I'm assuming the latter will need escaping (?)

Ugh. Yeah. And double-escaping too, because we need to escape the shell first. That's rather inconvenient.

I think a possible sidestep of this issue, at least in a first iteration, would be to ask the user to write queries that return only one label and use that label's values. If the values need massaging (e.g. to strip port numbers) then one solution might be to use label_replace in the query itself.

Agreed, let's start with that.

FWIW last I checked the python client is mostly to expose data, and doesn't deal with the HTTP API, IOW talking directly to the api would be the way to go.

Ah yes, that thing is for writing exporters, not talking to the server, my bad. So I guess we're need to do HTTP requests ourselves... We already depend on requests so that should be easy enough.

(I also found this code but it hardly seems worth the extra dependency...)

Other things to consider off top of my head: there might be multiple Prometheus instances one would like to query (e.g. there's a Prometheus deployed in each datacenter in WMF's deployment).

Not sure how that would work, but I would start by assuming there's only one endpoint per configuration and go from there.

On a more detailed level: there will be at least one conflict in syntax I can think of right now, { and } can appear in both Cumin and Prometheus. I'm assuming the latter will need escaping (?)

Ugh. Yeah. And double-escaping too, because we need to escape the shell first. That's rather inconvenient.

Why double? Curly braces don't need escape in the shell.
So, this might be a bit tricky. The current global grammar (P{puppetdb query} and A:aliasname...) allow for the use of curly braces in the specific grammars as long as they are inside quoted strings. Technically the implementation allows for { also outside quoted string as long as it's not prefix by one of the valid grammar prefixes like P{. But the closing bracket } is allowed only in quoted strings.
See https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/cumin/+/master/cumin/grammar.py#93 for more details.
I'll need to think a bit about it and go deeper into pyparsing capabilities for this.

(I also found this code but it hardly seems worth the extra dependency...)

Agree that doesn't seem worth it.

Other things to consider off top of my head: there might be multiple Prometheus instances one would like to query (e.g. there's a Prometheus deployed in each datacenter in WMF's deployment).

Not sure how that would work, but I would start by assuming there's only one endpoint per configuration and go from there.

I think we should include this from the start, because is the way prometheus scale, multiple instances, additional instance(s) for aggregation, etc. so I guess it might be common to support multiple instances.
On the configuration side it's easy, just allow for a list of endpoints (label: config). On the grammar side we need to define a way to address a specific instance by label. It would be nice to have this optional in the grammar, so that we can set a default label in the config and set that default automatically when there is only one endpoint, so to avoid the burdeen of writing its label each time.

Why double? Curly braces don't need escape in the shell.

No? Here {foo,bar} means foo bar, and is equivalent to * if the content of the current directory is foo bar.

I'll need to think a bit about it and go deeper into pyparsing capabilities for this.

I don't follow all of this, but it sounds painful. :p

I think we should include this from the start, because is the way prometheus scale, multiple instances, additional instance(s) for aggregation, etc. so I guess it might be common to support multiple instances.
On the configuration side it's easy, just allow for a list of endpoints (label: config). On the grammar side we need to define a way to address a specific instance by label. It would be nice to have this optional in the grammar, so that we can set a default label in the config and set that default automatically when there is only one endpoint, so to avoid the burdeen of writing its label each time.

So what would that actually look like? Maybe a trailing from keyword? With a configuration like:

main: https://prometheus.example.com/api...

You could do a query like:

T:"count(node_reboot_required>0) by (alias) from main"

where from main is implicit if not provided and there's only one endpoint configured... Is that something that would work? Or would it be outside the T: string?

[agreed on the rest]

Other things to consider off top of my head: there might be multiple Prometheus instances one would like to query (e.g. there's a Prometheus deployed in each datacenter in WMF's deployment).

Not sure how that would work, but I would start by assuming there's only one endpoint per configuration and go from there.

I think we should include this from the start, because is the way prometheus scale, multiple instances, additional instance(s) for aggregation, etc. so I guess it might be common to support multiple instances.
On the configuration side it's easy, just allow for a list of endpoints (label: config). On the grammar side we need to define a way to address a specific instance by label. It would be nice to have this optional in the grammar, so that we can set a default label in the config and set that default automatically when there is only one endpoint, so to avoid the burdeen of writing its label each time.

A thought off top of my head, as a generic mechanism/idea of "multiple instances" it could be attached to the backend keyword itself. e.g. T@label: to select a specific instance/label. I agree the default instance/label should be optional, also because it is easy to imagine use cases where there's just one Prometheus (Thanos, Prometheus as a service, etc). WDYT?

Why double? Curly braces don't need escape in the shell.

No? Here {foo,bar} means foo bar, and is equivalent to * if the content of the current directory is foo bar.

Sorry I always write cumin queries in quotes either single or double, that's why I was not getting what you meant.

You could do a query like:

T:"count(node_reboot_required>0) by (alias) from main"

where from main is implicit if not provided and there's only one endpoint configured... Is that something that would work? Or would it be outside the T: string?

With something like this it would be contained within the backend grammar and easy to handle inside that class.

[agreed on the rest]

A thought off top of my head, as a generic mechanism/idea of "multiple instances" it could be attached to the backend keyword itself. e.g. T@label: to select a specific instance/label. I agree the default instance/label should be optional, also because it is easy to imagine use cases where there's just one Prometheus (Thanos, Prometheus as a service, etc). WDYT?

@fgiunchedi the idea it's interesting to add it to the backend keyword as a general expansion of that logic, but it would require a bit of a larger refactor to integrate it as currently the logic to instantiate a backend is based on the keyword to pick the right class and then that class is instantiated with the configuration and then the execute() method is called with the query string.
To add this generalization we'd need to pick the class based on the keyword but then pass and additional argument based on the label. The other thing to think of is if this might affect any other existing grammar, but at first sight it seems it could be contained easily.

I don't have a strong opinion, we could have take advantage of the general approach if we had one in the past for the openstack backend fwiw.