API client doesn't support POST method for Query/QueryRange

[jacksontj]

POST support was added to query/query_range API endpoints in prometheus with prometheus/prometheus#3322. This client currently only sends GET requests, which means that it isn't won't work for large query string values. As the API stands today:

Query(ctx context.Context, query string, ts time.Time) (model.Value, error)
QueryRange(ctx context.Context, query string, r Range) (model.Value, error)

There isn't really a place to put it, so I see a couple options:

1. add a NewPOSTAPI method to return an httpAPI instance with a flag to use POST for both query and query_range (this would be done by setting an option in the httpAPI struct-- to be added).
2. add QueryPost and QueryRangePost to the API interface
3. add METHOD argument to Query and QueryRange

I'm not sure what the compatibility guarantees are on this client interface, so I'm not sure how we want to proceed. I'm leaning towards 1 or 2 (probably in that order)

cc @juliusv

[juliusv]

What about a fourth option: the user intent is already clear (query a given PromQL expression with a given time range), so can we just either determine automatically whether we need GET or POST (from some maximum length that GET args can have?), or alternatively always use POST?

[juliusv]

(the one thing against always using POST is that POST support was only introduced <1y ago, so probably some auto-detection would be best that just goes to POST for long queries)

[jacksontj]

I think that's a great idea. How about I add an option to the client Config which is MaxHeaderSize, if that is set it'll switch to POST for queries that would be larger. That way if you don't define the max size in the config, it will always do GET. This also means the interface can remain the same.

[juliusv]

Is it the HTTP headers in total that are size-limited, or the URL (that contains GET parameters) specifically?

[jacksontj]

the HTTP headers themselves. I looked into adding it there, but the issue is that the client.Config struct isn't exposed to the API client since it takes a Client (which is an interface). So this config option would need to be at the API client level, or we'd have to add a Config() method to the Client interface to access it.

[juliusv]

I want to understand this better. prometheus/prometheus#3072 was about URL size limits, not about HTTP headers in general. Browsers have URL size limits (like https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers), but here already the client is not a browser anymore, but a Go HTTP client. So we only ever need to switch to POST in case there are also size limitations in either the Go HTTP client (used in this client lib) or in the Go HTTP server (used by the Prometheus server that handles the requests generated here). I just tried it out and I only seem to be hitting a limit (that causes a connection reset by peer) around the 1MB URL size, which I don't expect any PromQL query to ever hit. So maybe we actually never need to use POST here?

Of course there might also be HTTP proxies in the middle, but not sure what the common URL size limits are on those...

[jacksontj]

The issue reported was about browser limits, but those don't apply too much to this client interface since we are talking from go. In this case the main limitation is the server itself. Go's server struct has an option called MaxHeaderBytes which determines the max total size of headers it will allow. By default this is 1MB -- so as you point out this might be fine to punt on for now, because a >1MB header likely means a MONSTROUS promql query ;)

Assuming this is still something we want, I think having an option for setting a limit and then having the client work around that limit would be good. That way if you do have proxies in the middle we can swap to using POST etc.

[juliusv]

I'm just unsure about the situation where an older server doesn't support POST yet and the client lib still switches to it because the query size is too large. It would result in a Method not Allowed HTTP status code where previously queries were working, and that could be more confusing than getting a more topical HTTP response code. Instead of having a user worry about configuring max header sizes, how about letting them specify the minimal Prometheus version they are targeting and always use POST if the version supports it, and always GET otherwise?

[beorn7]

Thanks, @juliusv for chiming in. Assigning this to @krasi-georgiev as per https://github.com/prometheus/client_golang/blob/master/MAINTAINERS.md for shepherding.

[jacksontj]

I'm just unsure about the situation where an older server doesn't support POST yet and the client lib still switches to it because the query size is too large.

This is why I'd leave the default config option for MaxHeaderBytes to 0, and default to GET. This would mean the client caller would need to specifically set a limit that would tell us to swap to POST requests. I think that'll be less of a maintenance headache as we don't have anysort of prom version discovery stuff built into this client, nor do I think we really want to :)

[juliusv]

I don't think it'd be unreasonable to make the client lib explicitly aware of the version of the Prometheus server on the other side. The API user will have to know the Prometheus version in any case to make use of the new POST features.

It's a question of how high- or low-level we want this control knob to be, and whether we'll want to do more future version-dependent switches. From the user's perspective, which explanation would be more straightforward: how MaxHeaderBytes interplays with different Prometheus versions and query sizes and how it will still lead to misleading error messages if set incorrectly, or just explaining how setting the Prometheus version >X enables support for larger queries?

But honestly, I would love to hear someone else's opinion on this too.

[jacksontj]

I prefer the more direct control (in this case the MaxHeaderBytes). The main reason being we don't know how they'll use the client. For example, if they where to talk to prom behind some proxy they'd need to set their own header limit which is independent of the prom version. I'm also fine to get some other opinions in here, but to me it seems that we at lest want this option. IMO if we're going down the route of differing the client based on version we may want to create separate interfaces for that -- as APIs come and go across prom versions (instead of a single API interface, have the various sub-versions).

[krasi-georgiev]

what about using compatbility matrix , similar to the k8s client
https://github.com/kubernetes/client-go#compatibility-matrix

[krasi-georgiev]

so nothing special in code , just always use post and in the readme mention that client version 0.9.1 and above works with prometheus version ....

[jacksontj]

That would work assuming the maintainers are okay with managing N versions for support, otherwise this client will immediately kick off anyone <2.2 (and similarly for other changes in the future).

That is really a question of what the support and compatibility guarantees are on this library.

[juliusv]

I don't think anyone here is up for managing client library versions to that degree and so I think just dropping support for older 2.x Proms in newer versions of this library is definitely not a good idea.

The case about proxy servers requiring a smaller MaxHeaderBytes is a good one. Ok, but now I set it to a particular value and I have an older Prom and get back a Method not Allowed error on larger queries, while smaller queries work just fine. That is confusing and unexpected. So I think yes, we might need to control the MaxHeaderBytes, but just unconditionally switching to POST when a query is larger than allowed doesn't seem right either, as it requires the right Prometheus version to work. If we knew that the right Prometheus version was on the other end, we could just always use POST to begin with, which solves any header size issues also for proxy servers (and otherwise POST won't help anyway, even with proxies). So I still think letting the library user specify the version on a client is a better way to go.

[krasi-georgiev]

I don't think anyone here is up for managing client library versions to that degree and so I think just dropping support for older 2.x Proms in newer versions of this library is definitely not a good idea.

I see what you mean. Basically if there is a bug in the client lib and the user is still using a Prom version that doesn't support POST we would need to backport the fix.

I agree that few more if statements based on provided Prom version to the client would require least maintenance.

@jacksontj I am happy to review a PR if you have the time.

[jacksontj]

now I set it to a particular value and I have an older Prom and get back a Method not Allowed error on larger queries, while smaller queries work just fine. That is confusing and unexpected

My thinking would be that the default of this off stays GETs forever, and we document that setting this option will only work for prom v2.x and up. So by you the client setting that option you are saying you are at least that version. This seems like the least maintenance to me, especially since the difference goes away after prom 1.x support is dropped.

If we want to add switching based on the prom version, the idea is that we'd unconditionally do GET/POST based soely on the version?

[brian-brazil]

How about using POST and falling back to GET if we get that the method isn't supported?

[krasi-georgiev]

that is a good idea as well.
I am ok with both ideas.

[juliusv]

POST first and GET as fallback sounds good to me too, slight preference from my side to do it unconditionally, then we don't even need the MaxHeaderBytes option.

[jranson]

Any progress on this issue?

[beorn7]

Assigning to @krasi-georgiev for tracking.

[krasi-georgiev]

Happy to review a PR , but don't think I will have time to do the actual implementation so contributions are welcome.

[krasi-georgiev]

@jacksontj Any chance for a PR? I think we have a pretty good plan for action here so I can review it quickly.