Extend PrometheusClient interface to pass label matchers as arguments

[dsmith3197]

Hi @Nexucis and @juliusv ,

Can we extend the PrometheusClient interface to pass in the label matchers for the associated vector selector? I want to implement a custom PrometheusClient that behaves in the same way as the Grafana M3 plugin. However, the current interface does not pass enough information to make the more refined series queries that the Grafana plugin makes. This is an issue when a metric name has high enough of a cardinality that will cause the series endpoint to fail when only the metric name is given w/o additional label matchers. It is also an issue when a user selects a label matcher that then limits the remaining label matchers that can be chosen. As such, this package as it is now is unusable for my use case. However, with a slight modification, it will support implementing a PrometheusClient with the same functionality as the Grafrana M3 plugin.

A proposed extended interface is as follows:

export interface PrometheusClient {
  labelNames(metricName?: string, matchers?: Matcher[]): Promise<string[]>;

  // labelValues return a list of the value associated to the given labelName.
  // In case a metric is provided, then the list of values is then associated to the couple <MetricName, LabelName>
  labelValues(labelName: string, metricName?: string, matchers?: Matcher[]): Promise<string[]>;

  metricMetadata(): Promise<Record<string, MetricMetadata[]>>;

  series(metricName: string, matchers?: Matcher[]): Promise<Map<string, string>[]>;
}

export interface Context {
  kind: ContextKind;
  metricName?: string;
  labelName?: string;
  matchers?: Matcher[];
}

Here are a few examples where this is necessary.

• Suppose the expression is some_metric{job="my_job", instance="my_instance", |} (where the | is the cursor position, not a character in the expression).

  • In this case, codemirror-promql with send the following API request /api/v1/series?start={}&end={}&matcher[]=some_metric. However, I would instead like to make the following API request: /api/v1/series?start={}&end={}&matcher[]=some_metric{job="my_job", instance="my_instance"} (this is what the Grafana M3 plugin does). In order to so, I propose that the matchers passed as an argument to labelValues(...) is a list of 2 label matchers (i.e., the equivalent of job="my_job and instance="my_instance").

• Suppose the expression is some_metric{job="my_job", instance="|"} (where the | is the cursor position, not a character in the expression, so the instance label matcher currently has an empty string value).

  • In this case, codemirror-promql with send the following API request /api/v1/series?start={}&end={}&matcher[]=some_metric. However, I would instead like to make the following API request: /api/v1/series?start={}&end={}&matcher[]=some_metric{job="my_job"} (this is what the Grafana M3 plugin does). In order to so, I propose that the matchers passed as an argument to labelValues(...) is a list of 2 label matchers (i.e., the equivalent of job="my_job and instance="") and the client can determine which label matcher to omit from the query.

• Finally, I have an additional endpoint I use to search for metric names given a metric prefix. In order to handle this case, could we have metricName be defined when labelValues("__name__") is called. For instance, suppose the expression is some_metric_prefix. Currently, labelValues("__name__") is called in this case. Instead, I wish for labelValues("__name__", "some_metric_prefix") to be called. This one does not require extending the interface at all, so I really hope we can implement this one. This one in particular is blocking me from using this library.

I've implemented a proof of concept of this that works. I believe that this extension of the interface will allow for much more sophisticated clients, so I hope you do consider. Also, I'm willing to implement these changes myself along with tests.

[juliusv]

Hi @dsmith3197, thanks for the great description! Yes, I think the main reason this doesn't exist yet (especially the first two points) is that nobody got around to it yet. Other than that I would say it makes sense.

Just for good measure, a word of caution / something to consider (and you most likely have already): The main downside of this would be that the extension needs to run more queries in total as the user is typing (albeit with progressively smaller results) and/or cache more variations of differently constrained results, making the cache larger and less useful. So I expect this to perform better for some use cases and worse for others. Generally, autocompletion silently firing off potentially expensive queries (as we currently already do) is always a bit of a liability, as we run the risk of overloading the user's Prometheus backend without getting explicit consent to do so. In any case, since the PrometheusClient is swappable, users of this package can then choose to implement this proposed extra level of filtering or not. Btw. do you also plan on building a new cache that deals with the richer metadata situation, or would you just give up on client-side caching for that?

By the way, I really wish we had prometheus/prometheus#6178 already, which would make label value queries directly filterable by label matchers, without needing to go through the more expensive /api/v1/series API.

As for the last point of supplying the already-typed metric name prefix, I don't see an issue with that except that the specific overloading of the metricName argument can be confusing. I wonder if it would be cleaner / more obvious to people what's happening if we added a new method to the interface that specifically only autocompletes metric names? Like metricNames(prefix?: string): Promise<string[]>.

Anyway, overall no objections, would be great to enable this!

The repo is also still pretty new / experimental in general, so it will be great to have more people invested in it, maybe even maintaining it over time :)

[Nexucis]

Hi @dsmith3197,

Thanks for the idea.

@juliusv yeah I think it will be less confusing to add a dedicated method to retrieve the metric instead of using the method labelValues. Just note it will require to change a bit the current completion strategy in order to provide this prefix in this particular case.

Otherall I don't think it makes sense to pass the matcher as is in the method. I would prefer it is built by the client itself. I think it would make more sense to provide a pair of labelName/labelValue to the method and then it will be the responsibility of the client to then generate the correct query.

Also like that it will be easier to ignore the empty value (with no particular regex or weird parsing)
And it will avoid to have to add the matcher to the method labelName which for me is not really needed.
And it will avoid also to expose a bit too much the way the prometheus API is working.

Finally I think it will be a nice optimization when you are not using the caching system provided by the lib. So yeah no issue to implement it.

Tell us if you want some help.

[Nexucis]

Otherall I don't think it makes sense to pass the matcher as is in the method

Looking again at your code you shared, do you actually want to use the class Matcher coming from the package parser ?

[dsmith3197]

Looking again at your code you shared, do you actually want to use the class Matcher coming from the package parser ?

I am considering using it because it contains the information I want to pass to the client. However, the client shouldn't have to understand anything about the parser itself, especially the codes for the =,=~,!=,!~, so it may make sense to define a new type.

[dsmith3197]

Also, thank you for the quick responses! I'm really excited about this library so I'm happy to contribute. I'm going to begin by added a metricNames method to the client, as that is my most immediate blocker.

[juliusv]

Sounds great, and looking forward to more contributions :)

[Nexucis]

I am considering using it because it contains the information I want to pass to the client. However, the client shouldn't have to understand anything about the parser itself, especially the codes for the =,=~,!=,!~, so it may make sense to define a new type.

yeah I think it will be great to reuse it since the logic to create the Matcher already exists there. But it's a bit annoying that the prometheus client depends on this package mmm. Maybe a reorg of the package is needed.

By the way I will have some free time during this week / weekend, so if you want I can help you to implement what is missing. But if you prefer to do it on your side, it's ok too.

Once this issue is closed, I think we will be good to create a new release (v0.11.0 probably)

[Nexucis]

But it's a bit annoying that the prometheus client depends on this package mmm.

Or maybe it's fine, and it is just a bit late for this kind of thoughts hahaha.

[dsmith3197]

hey @Nexucis, sorry I was on vacation for the past week. If you have the free time, I'd love some help. Otherwise, I can try to get this done within the next week or so.

[Nexucis]

no problem @dsmith3197 :). Ok I will try to help then. Let's see if I'm able to provide a PR in a couple of days