Extend PrometheusClient interface to pass label matchers as arguments about codemirror-promql HOT 10 CLOSED

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 :)

from codemirror-promql.

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.

from codemirror-promql.

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.

from codemirror-promql.

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 ?

from codemirror-promql.

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.

from codemirror-promql.

Sounds great, and looking forward to more contributions :)

from codemirror-promql.

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)

from codemirror-promql.

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.

from codemirror-promql.

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.

from codemirror-promql.

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

from codemirror-promql.