[query] Add a limit exceeded header if too many series are returned #1838
Conversation
| @@ -200,6 +209,10 @@ func (h *renderHandler) serveHTTP( | |||
| SortApplied: true, | |||
| } | |||
|
|
|||
| if !exhaustive { | |||
| w.Header().Set(handler.LimitHeader, "true") | |||
There was a problem hiding this comment.
nit: Should we make this a constant? i.e. w.Header().Set(handler.LimitHeader, handler.LimitHeaderSeriesLimitApplied)?
In the future if we limit the results due to a datacenter/remote region being down, we can concat these together perhaps.
ie.
w.Header().Set(handler.LimitHeader, handler.LimitHeaderSeriesLimitApplied)
// in handler pkg
const LimitHeaderSeriesLimitApplied = "max_fetch_series_limit_applied"Then in future if we have another type:
w.Header().Set(handler.LimitHeader,
handler.LimitHeaderValue(handler.LimitHeaderSeriesLimitApplied,
handler.LimitHeaderRegionUnavailableLimitApplied))
// in handler pkg
func LimitHeaderValues(str ...string) string {
return strings.Join(str..., ",")
}There was a problem hiding this comment.
The future is now!
robskillington
changed the title
|
Adding a note here, if #1938 lands first then need to search for "pull/1838" in the source code and explicitly add the limit exceed header if any warnings. |
# Conflicts: # src/query/generated/proto/rpcpb/query.pb.go # src/query/storage/fanout/storage.go # src/query/storage/m3/m3_mock.go
Codecov Report
@@ Coverage Diff @@
## master #1838 +/- ##
=========================================
+ Coverage 58.6% 63.4% +4.8%
=========================================
Files 1087 1114 +27
Lines 99076 105919 +6843
=========================================
+ Hits 58073 67254 +9181
+ Misses 36900 34338 -2562
- Partials 4103 4327 +224
Continue to review full report at Codecov.
|
| @@ -40,6 +40,11 @@ import ( | |||
| opentracinglog "github.com/opentracing/opentracing-go/log" | |||
| ) | |||
|
|
|||
| type readResult struct { | |||
There was a problem hiding this comment.
Nice, good pattern
src/query/block/lazy_test.go
Outdated
| @@ -1,5 +1,5 @@ | |||
| // Copyright (c) 2019 Uber Technologies, Inc. | |||
| // | |||
| //c | |||
src/query/block/meta.go
Outdated
|
|
||
| // NewResultMetadata creates a new result metadata. | ||
| // NB: capacity for warning is initialized to 1, since it is unlikely that more | ||
| // than one warning will exist. |
There was a problem hiding this comment.
Maybe worth even just leaving it nil? Only because I assume most queries won't cause a warning, and it will unfortunately almost definitely be a heap allocation (since it's a slice and pointer type of which the life time is hard to reason about, since not contained to use in a single method).
src/query/block/meta.go
Outdated
|
|
||
| // CombineMetadata combines two result metadatas. | ||
| func (m ResultMetadata) CombineMetadata(other ResultMetadata) ResultMetadata { | ||
| combinedWarnings := make([]Warning, 0, len(m.Warnings)+len(other.Warnings)) |
There was a problem hiding this comment.
Maybe only allocate if len(m.Warnings)+len(other.Warnings) is greater than zero?
| // Update constituent blocks with combined resultMetadata if it has been | ||
| // changed. | ||
| if !resultMeta.IsDefault() { | ||
| bl = block.NewLazyBlock(bl, lazyOpts) |
There was a problem hiding this comment.
Hah, this is kind of cool. Nice.
| mu.Unlock() | ||
| results[i] = targetSeries |
There was a problem hiding this comment.
Should the results[i] = targetSeries remain inside the mu.Lock() and mu.Unlock()? I guess it's inserting into unique index so doesn't necessarily need to be...
There was a problem hiding this comment.
I think it's the latter, but moved it into the lock just to be safe; doubt there are any perf implications here
| } | ||
|
|
||
| if len(r.seenIters) == 0 { | ||
| return encoding.EmptySeriesIterators, nil | ||
| return result, nil |
There was a problem hiding this comment.
Does result.SeriesIterators = encoding.EmptySeriesIterators need to be set here before returning?
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/golang/mock/gomock" |
There was a problem hiding this comment.
Reminder to group gomock with the testify/assert.
src/query/storage/m3/storage.go
Outdated
| @@ -351,8 +381,8 @@ func (s *m3storage) SearchSeries( | |||
| return nil, err | |||
| } | |||
|
|
|||
| metrics := make(models.Metrics, len(tagResult)) | |||
| for i, result := range tagResult { | |||
| metrics := make(models.Metrics, len(tagResult.Tags)) | |||
There was a problem hiding this comment.
Make this into a length zero with append calls while here nmaybe?
src/query/tsdb/remote/codecs.go
Outdated
| } | ||
|
|
||
| return rpc.FanoutOption_FORCE_ENABLED, | ||
| fmt.Errorf("Unknown fanout option for proto encoding: %v\n", opt) |
There was a problem hiding this comment.
Hm seems this one could be return 0, fmt.Errorf("unknown ...") as pinged about elsewhere
What this PR does / why we need it:
Adds a limit exceeded header to requests that go over the m3db limit
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: