Limit the number of samples remote read can return. #4532

tomwilkie · 2018-08-23T17:15:54Z

Tracking down Prometheus OOMs for a client, initially thought it was PromQL (see #4414), but looks like it is remote read requests.

Quick and dirty limit; probably want this plumbed through as a command line argument?

Signed-off-by: Tom Wilkie [email protected]

gouthamve · 2018-08-23T17:17:53Z

This might break some Thanos users. @fabxc @Bplotka

tomwilkie · 2018-08-23T17:19:09Z

Yes, sorry should have said; OOMs seemed to be caused by queries submitted via Thanos.

We'll see if we can remove this limit through a streaming API as discussed at the dev summit, this is just a quick fix.

brian-brazil · 2018-08-23T17:49:29Z

cmd/prometheus/main.go

@@ -164,6 +164,9 @@ func main() {
 	a.Flag("storage.remote.flush-deadline", "How long to wait flushing sample on shutdown or config reload.").
 		Default("1m").PlaceHolder("<duration>").SetValue(&cfg.RemoteFlushDeadline)

+	a.Flag("storage.remote.read-sample-limit", "Maxium number of samples to return via the remote read interface, in a single query.").


brian-brazil · 2018-08-23T17:49:41Z

cmd/prometheus/main.go

@@ -164,6 +164,9 @@ func main() {
 	a.Flag("storage.remote.flush-deadline", "How long to wait flushing sample on shutdown or config reload.").
 		Default("1m").PlaceHolder("<duration>").SetValue(&cfg.RemoteFlushDeadline)

+	a.Flag("storage.remote.read-sample-limit", "Maxium number of samples to return via the remote read interface, in a single query.").
+		Default("20m").IntVar(&cfg.web.RemoteReadLimit)


Usually 0 would mean no limit.

Does m mean milli here?

milesbxf · 2018-08-23T18:00:51Z

cmd/prometheus/main.go

@@ -164,6 +164,9 @@ func main() {
 	a.Flag("storage.remote.flush-deadline", "How long to wait flushing sample on shutdown or config reload.").
 		Default("1m").PlaceHolder("<duration>").SetValue(&cfg.RemoteFlushDeadline)

+	a.Flag("storage.remote.read-sample-limit", "Maxium number of samples to return via the remote read interface, in a single query.").
+		Default("20m").IntVar(&cfg.web.RemoteReadLimit)


Default is "20m", but the flag is an IntVar - running this results in Error parsing commandline arguments: strconv.ParseFloat: parsing "20m": invalid syntax

So.. how to assess what max number of samples Prometheus can handle from remote read API?

tomwilkie · 2018-08-23T18:02:23Z

Yeah, should be 2e7. I can also make 0 mean no limit, but it’s not very safe...

…

On Thu, 23 Aug 2018 at 19:01, Miles Bryant ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cmd/prometheus/main.go <#4532 (comment)> : > @@ -164,6 +164,9 @@ func main() { a.Flag("storage.remote.flush-deadline", "How long to wait flushing sample on shutdown or config reload."). Default("1m").PlaceHolder("<duration>").SetValue(&cfg.RemoteFlushDeadline) + a.Flag("storage.remote.read-sample-limit", "Maxium number of samples to return via the remote read interface, in a single query."). + Default("20m").IntVar(&cfg.web.RemoteReadLimit) Default is "20m", but the flag is an IntVar - running this results in Error parsing commandline arguments: strconv.ParseFloat: parsing "20m": invalid syntax — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4532 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbGhWFcKEjnhTll1a7RFVj92GkhHMT7ks5uTu3-gaJpZM4WJ8eW> .

bwplotka

As long as this has reasonable default and configurable flag, Thanos should be fine.

Incidentally is there any spike for streaming API for remote Read @tomwilkie? Can I help with this?

bwplotka · 2018-08-24T10:55:54Z

cmd/prometheus/main.go

@@ -164,6 +164,9 @@ func main() {
 	a.Flag("storage.remote.flush-deadline", "How long to wait flushing sample on shutdown or config reload.").
 		Default("1m").PlaceHolder("<duration>").SetValue(&cfg.RemoteFlushDeadline)

+	a.Flag("storage.remote.read-sample-limit", "Maxium number of samples to return via the remote read interface, in a single query.").
+		Default("20m").IntVar(&cfg.web.RemoteReadLimit)


So.. how to assess what max number of samples Prometheus can handle from remote read API?

bwplotka · 2018-08-24T10:56:25Z

storage/remote/codec.go

 	resp := &prompb.QueryResult{}
 	for ss.Next() {
 		series := ss.At()
 		iter := series.Iterator()
 		samples := []*prompb.Sample{}

 		for iter.Next() {
+			numSamples += 1
+			if numSamples > sampleLimit {
+				return nil, fmt.Errorf("too many samples")


Can we also wrap the error with what is the limit?

We should probably have some metrics around this too.

And make it return a 400, not a 500...

tomwilkie · 2018-08-24T11:27:03Z

So.. how to assess what max number of samples Prometheus can handle from remote read API?

I've set the default to 20 million based on discussions on the other PR (#4513), but will happily consider making it higher.

tomwilkie · 2018-08-24T11:29:45Z

Incidentally is there any spike for streaming API for remote Read @tomwilkie? Can I help with this?

Yes we discussed at the dev summit making a streaming API to help thanos & other systems. I've just done similar in cortex: cortexproject/cortex#933

Its beyond the scope of this PR - I suggest you start a thread on the -dev mailing list. We discussed streaming the compressed chunks out, instead of samples; previously I think you + I discussed trying to make a common API that can be used in both Cortex and Thanos.

brian-brazil · 2018-08-24T11:30:17Z

We're in a different context here, so a higher value could be fine. I arbitrarily say that we aim to keep it under 1GB by default.

tomwilkie · 2018-08-24T11:35:45Z

Ignoring labels, 60M * 16 bytes = 1GB, so I'll set it to that.

brian-brazil · 2018-08-24T11:37:30Z

Call it 50M to allow for the labels, and requests for many series with few points. It's also more round.

- Return 413 entity too large. - Limit can be set be a flag. Allow 0 to mean no limit. - Include limit in error message. - Set default limit to 50M (* 16 bytes = 800MB). Signed-off-by: Tom Wilkie <[email protected]>

tomwilkie · 2018-08-28T14:19:00Z

Rebased against master; ready for final review I hope!

brian-brazil · 2018-08-28T15:13:51Z

storage/remote/codec.go

+			if sampleLimit > 0 && numSamples > sampleLimit {
+				return nil, HTTPError{
+					msg:    fmt.Sprintf("exceeded sample limit (%d)", sampleLimit),
+					status: http.StatusRequestEntityTooLarge,


This isn't a 413, as it's the server response that's too big. I think this should be a 400.

Agree, 413 indicates the request size too big, 400 would be less confusing

brian-brazil

We might want to add some metrics around this

bwplotka

Typo + some suggestions

bwplotka · 2018-09-05T11:04:08Z

cmd/prometheus/main.go

@@ -177,6 +177,9 @@ func main() {
 	a.Flag("rules.alert.resend-delay", "Minimum amount of time to wait before resending an alert to Alertmanager. Must be lower than resolve_timeout in Alertmanager").
 		Default("1m").SetValue(&cfg.resendDelay)

+	a.Flag("storage.remote.read-sample-limit", "Maximum number of samples to return via the remote read interface, in a single query.  0 means no limit.").


s/query. 0 means no limit/ query. 0 means no limit/ (double space)

Also worth to mention that this is "overall" number of samples. not per series (this could be a follow up question if there is no clarification)

Also, is there anyway we can make it more convenient for user and already map this flag into memory size used? It is easier to limit the memory here and we can calculate/estimate mem into number of samples... The cost is that the calculation is on our part.. but it's still better, than thousands question on IRC how the number of samples here maps into memory (: Any thoughts?

Ignore me, we can only roughly estimate the mem usage with the simple logic, so let's stick to just samples limit.

bwplotka · 2018-09-05T11:20:19Z

storage/remote/codec.go

+			if sampleLimit > 0 && numSamples > sampleLimit {
+				return nil, HTTPError{
+					msg:    fmt.Sprintf("exceeded sample limit (%d)", sampleLimit),
+					status: http.StatusRequestEntityTooLarge,


Agree, 413 indicates the request size too big, 400 would be less confusing

bwplotka · 2018-09-05T11:20:53Z

storage/remote/codec.go

 	resp := &prompb.QueryResult{}
 	for ss.Next() {
 		series := ss.At()
 		iter := series.Iterator()
 		samples := []*prompb.Sample{}

 		for iter.Next() {
+			numSamples += 1


minor nit: ++ instead?

bwplotka · 2018-09-05T11:22:39Z

web/api/v1/api.go

-			http.Error(w, err.Error(), http.StatusInternalServerError)
+			if httpErr, ok := err.(remote.HTTPError); ok {
+				http.Error(w, httpErr.Error(), httpErr.Status())
+			} else {


can we just return in each case? no need for else here and one indent less below

brancz · 2018-09-05T11:37:08Z

Can you actually try a large size message? As far as I know the default maximum size for a proto message is limited to 64Mb (last time I checked they warned of larger sizes as it loosens some security constraints, but not sure of the details).

bwplotka · 2018-09-05T12:01:35Z

To clarify @brancz do you mean "if it's even useful to have this flag because anything more than 50m samples will make protobuf message larger than default proto message (64MB)"?

Good point, not sure of the implications. Also, how you get 64MB? Just for comparison: The grpc grpc/grpc#7927 by default allows max 4MB (: But it might be because protocol limitations itself. Would love to know more about implications.

BTW we are working on streaming as well: https://docs.google.com/document/d/1JqrU3NjM9HoGLSTPYOvR217f5HBKBiJTqikEB9UiJL0/edit?ts=5b8f135a#

@tomwilkie how the current flag would fit into streamed response? That limit would be an overall limit? or per frame?

gouthamve · 2018-09-05T12:06:22Z

We don't use gRPC, we marshal into proto and send it over HTTP.

prometheus/storage/remote/codec.go

Lines 68 to 81 in 14b0449

    
           // EncodeReadResponse writes a remote.Response to a http.ResponseWriter. 
        
           func EncodeReadResponse(resp *prompb.ReadResponse, w http.ResponseWriter) error { 
        
           	data, err := proto.Marshal(resp) 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           	w.Header().Set("Content-Type", "application/x-protobuf") 
        
           	w.Header().Set("Content-Encoding", "snappy") 
        
           	compressed := snappy.Encode(nil, data) 
        
           	_, err = w.Write(compressed) 
        
           	return err 
        
           }

Not sure where the 64MB limit is documented though.

bwplotka · 2018-09-05T12:07:09Z

I know @gouthamve, just putting grpc as an example

Signed-off-by: Tom Wilkie <[email protected]>

tomwilkie · 2018-09-05T12:17:29Z

As far as I know the default maximum size for a proto message is limited to 64Mb

I'm not aware, nor could I find, any limit in the golang proto bindings at https://github.com/golang/protobuf.

To clarify @brancz do you mean "if it's even useful to have this flag because anything more than 50m samples will make protobuf message larger than default proto message (64MB)"?

Yes it is still useful; we build the datastructure before encoding it, and its the building of the datastructure to which this limits applies. Even if there was a 64MB limit, we still see OOMs for large queries to the remote read endpoint, imply its the building of the datastructure causing the OOM.

@tomwilkie how the current flag would fit into streamed response? That limit would be an overall limit? or per frame?

Until an agreed upon approach exists for the streaming, no. I can only speculate that this could apply to a single "frame", but thats just a guess. Lets take this one offline, we can always deprecate the flag if needs be.

brancz · 2018-09-05T12:19:02Z

The thing I was thinking of is this: https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.io.coded_stream#CodedInputStream.SetTotalBytesLimit.details

The golang implementations shouldn't actually be affected by this, but it feels like we should follow the best practice especially for an API that is exposed to be consumed by arbitrary implementations.

gouthamve · 2018-09-05T12:20:18Z

👍

tomwilkie · 2018-09-05T13:00:29Z

The golang implementations shouldn't actually be affected by this, but it feels like we should follow the best practice especially for an API that is exposed to be consumed by arbitrary implementations.

By setting the limit to 64MB by default, you'd be limiting Thanos to fetching ~8k timeseries per query (64MB / 16bytes * 15s scrape interval / 2hr head block). I think that limit would be too low, unless my math is wrong.

brancz · 2018-09-05T13:48:28Z

That's fair. Maybe we should improve this step by step instead and only impose smaller limits if we actually manage to get streaming for example.

brancz

We should just have a test for the too large case, otherwise lgtm.

brancz · 2018-09-05T13:50:14Z

web/api/v1/api_test.go

@@ -861,6 +862,10 @@ func TestReadEndpoint(t *testing.T) {
 	recorder := httptest.NewRecorder()
 	api.remoteRead(recorder, request)

+	if recorder.Code/100 != 2 {


Can we add a test for the too large case as well?

sksingh20 · 2021-05-28T15:53:41Z

Team, I have major issue on grafana dashboard which is increasing step size by itself resulting in substantial loss of data.
grafana/grafana#34775

As per them this is limitation of Prometheus where prometheus is making limit of 11K records.

Could you please confirm if there is any limit enforced for query to return data points and what is that value.

sksingh20 · 2021-05-31T16:48:52Z

@brancz Any thoughts on claim by Grafana teams?

tomwilkie force-pushed the limit-remote-read branch from 0d18599 to 533ee47 Compare August 23, 2018 17:16

tomwilkie force-pushed the limit-remote-read branch 2 times, most recently from d2afd39 to eec2302 Compare August 23, 2018 17:35

brian-brazil reviewed Aug 23, 2018

View reviewed changes

milesbxf reviewed Aug 23, 2018

View reviewed changes

bwplotka approved these changes Aug 24, 2018

View reviewed changes

tomwilkie force-pushed the limit-remote-read branch from e21a12c to ea7e52a Compare August 28, 2018 14:13

Limit the number of samples remote read can return.

14b0449

- Return 413 entity too large. - Limit can be set be a flag. Allow 0 to mean no limit. - Include limit in error message. - Set default limit to 50M (* 16 bytes = 800MB). Signed-off-by: Tom Wilkie <[email protected]>

tomwilkie force-pushed the limit-remote-read branch from ea7e52a to 14b0449 Compare August 28, 2018 14:18

tomwilkie changed the title ~~[WIP] Limit the number of samples remote read can return.~~ Limit the number of samples remote read can return. Aug 28, 2018

brian-brazil reviewed Aug 28, 2018

View reviewed changes

bwplotka requested changes Sep 5, 2018

View reviewed changes

Review feedback.

43f4213

Signed-off-by: Tom Wilkie <[email protected]>

tomwilkie merged commit 457e4bb into master Sep 5, 2018

tomwilkie deleted the limit-remote-read branch September 5, 2018 13:50

brancz reviewed Sep 5, 2018

View reviewed changes

xjewer mentioned this pull request Sep 12, 2018

Query via Thanos causes Prometheus to OOM thanos-io/thanos#455

Closed

gouthamve mentioned this pull request Oct 18, 2018

Limit number of data points returned in a query #4414

Closed

Roasbeef mentioned this pull request Jul 3, 2019

Lndmon fixups lightninglabs/lndmon#3

Merged

Limit the number of samples remote read can return. #4532

Limit the number of samples remote read can return. #4532

Conversation

tomwilkie commented Aug 23, 2018

gouthamve commented Aug 23, 2018

tomwilkie commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Aug 23, 2018 via email

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie commented Aug 24, 2018

tomwilkie commented Aug 24, 2018

brian-brazil commented Aug 24, 2018

tomwilkie commented Aug 24, 2018

brian-brazil commented Aug 24, 2018

tomwilkie commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Sep 5, 2018 • edited Loading

bwplotka commented Sep 5, 2018 • edited Loading

gouthamve commented Sep 5, 2018

bwplotka commented Sep 5, 2018 • edited Loading

tomwilkie commented Sep 5, 2018 • edited Loading

brancz commented Sep 5, 2018

gouthamve commented Sep 5, 2018

tomwilkie commented Sep 5, 2018 • edited Loading

brancz commented Sep 5, 2018

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sksingh20 commented May 28, 2021

sksingh20 commented May 31, 2021

bwplotka Sep 5, 2018 •

edited

Loading

brancz commented Sep 5, 2018 •

edited

Loading

bwplotka commented Sep 5, 2018 •

edited

Loading

bwplotka commented Sep 5, 2018 •

edited

Loading

tomwilkie commented Sep 5, 2018 •

edited

Loading

tomwilkie commented Sep 5, 2018 •

edited

Loading