-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
store: add ability to limit max num of samples / concurrent queries #798
Merged
GiedriusS
merged 57 commits into
thanos-io:master
from
GiedriusS:feature/store_sample_limit
Mar 23, 2019
Merged
Changes from 50 commits
Commits
Show all changes
57 commits
Select commit
Hold shift + click to select a range
1bc1f59
store: add ability to limit max samples / conc. queries
e87f763
store/bucket: account for the RawChunk case
1ab1dc6
store/bucket_e2e_test: adjust sample limit size
d7c3ade
store/bucket: add metric thanos_bucket_store_queries_limited_total
12db24a
store/bucket: register queriesLimited metric
9d0b8a7
store: make changes according to the review comments
9727072
docs/store: update
d9c733a
store: gating naming changes, add span/extra metric
c4ce735
store: improve error messages
30eef19
store/limiter: improve error messages
194394d
store/gate: time -> seconds
2e51c2e
store/bucket_e2e_test: narrow down the first query
58a14fa
store/bucket: check for negative maxConcurrent
4d1b7ed
cmd/store: clarify help message
3ae3733
Merge remote-tracking branch 'origin/master' into smpl
b149f74
pkg/store: hook thanos_bucket_store_queries_limited into Limiter
e79c56d
store/bucket_test: fix NewBucketStore call
1d07515
docs: update again
38a093b
store/gate: spelling fix
7b13f7e
store/gate: spelling fix #2
4540394
Merge remote-tracking branch 'origin/master' into feature/store_sampl…
GiedriusS 3e532fe
store/bucket: remove pointless newline
GiedriusS cff979c
store/gate: generalize gate timing
e7ea64b
store/gate: convert the g.gateTiming metric into a histogram
23c7368
store/bucket: change comment wording
da575e4
store/bucket: remove type from maxSamplesPerChunk
e390846
store/bucket: rename metric into thanos_bucket_store_queries_dropped
24e8e1f
thanos/store: clarify help message
4012eca
store/gate: rename metric to thanos_bucket_store_queries_in_flight
e7be55d
store/gate: fix MustRegister() call
3e8150d
docs: update
5ec5ce9
store/bucket: clarify the name of the span
810a131
store/bucket: inline calculation into the function call
541f180
Merge remote-tracking branch 'origin' into fork_store_sample_limit
ae8e425
CHANGELOG: add item about this
de8a234
store/gate: reduce number of buckets
07b4658
store/bucket: rename metric to thanos_bucket_store_queries_dropped_total
61d6ecd
store/bucket: move defer out of code block
70b115d
store/gate: generalize gate for different kinds of subsystems
36f1153
store/limiter: remove non-nil check
9b74bbe
CHANGELOG: fixes
82bdb3c
store/limiter: convert failedCounter to non-ptr
4d8420f
store/limiter: remove invalid comment
590b9a6
*: update according to review comments
3f40bac
CHANGELOG: update
f4734e5
*: fix according to review
d6c1534
*: fix according to review
1147acd
*: make docs
1d0fad3
CHANGELOG: clean up
ef4a51e
CHANGELOG: update
GiedriusS 48141fd
Merge remote-tracking branch 'origin' into feature/store_sample_limit
c9a7d83
*: queries_in_flight_total -> queries_in_flight
GiedriusS d71f1d8
Merge branch 'master' into smpl_limit
280a8ca
store/bucket: do not wraper samplesLimiter error
11c4b18
store/bucket: err -> errors.Wrap
31a8346
store: make store.grpc.series-max-concurrency 20 by default
GiedriusS 6e98dfd
CHANGELOG: add warning about new limit
GiedriusS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import ( | |
"github.com/improbable-eng/thanos/pkg/block/metadata" | ||
"github.com/improbable-eng/thanos/pkg/compact/downsample" | ||
"github.com/improbable-eng/thanos/pkg/component" | ||
"github.com/improbable-eng/thanos/pkg/extprom" | ||
"github.com/improbable-eng/thanos/pkg/objstore" | ||
"github.com/improbable-eng/thanos/pkg/pool" | ||
"github.com/improbable-eng/thanos/pkg/runutil" | ||
|
@@ -42,6 +43,14 @@ import ( | |
"google.golang.org/grpc/status" | ||
) | ||
|
||
// maxSamplesPerChunk is approximately the max number of samples that we may have in any given chunk. This is needed | ||
// for precalculating the number of samples that we may have to retrieve and decode for any given query | ||
// without downloading them. Please take a look at https://github.com/prometheus/tsdb/pull/397 to know | ||
// where this number comes from. Long story short: TSDB is made in such a way, and it is made in such a way | ||
// because you barely get any improvements in compression when the number of samples is beyond this. | ||
// Take a look at Figure 6 in this whitepaper http://www.vldb.org/pvldb/vol8/p1816-teller.pdf. | ||
const maxSamplesPerChunk = 120 | ||
|
||
type bucketStoreMetrics struct { | ||
blocksLoaded prometheus.Gauge | ||
blockLoads prometheus.Counter | ||
|
@@ -57,6 +66,8 @@ type bucketStoreMetrics struct { | |
seriesMergeDuration prometheus.Histogram | ||
resultSeriesCount prometheus.Summary | ||
chunkSizeBytes prometheus.Histogram | ||
queriesDropped prometheus.Counter | ||
queriesLimit prometheus.Gauge | ||
} | ||
|
||
func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics { | ||
|
@@ -132,6 +143,15 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics { | |
}, | ||
}) | ||
|
||
m.queriesDropped = prometheus.NewCounter(prometheus.CounterOpts{ | ||
Name: "thanos_bucket_store_queries_dropped_total", | ||
Help: "Number of queries that were dropped due to the sample limit.", | ||
}) | ||
m.queriesLimit = prometheus.NewGauge(prometheus.GaugeOpts{ | ||
Name: "thanos_bucket_store_queries_concurrent_max", | ||
Help: "Number of maximum concurrent queries.", | ||
}) | ||
|
||
if reg != nil { | ||
reg.MustRegister( | ||
m.blockLoads, | ||
|
@@ -148,6 +168,8 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics { | |
m.seriesMergeDuration, | ||
m.resultSeriesCount, | ||
m.chunkSizeBytes, | ||
m.queriesDropped, | ||
m.queriesLimit, | ||
) | ||
} | ||
return &m | ||
|
@@ -173,7 +195,12 @@ type BucketStore struct { | |
// Number of goroutines to use when syncing blocks from object storage. | ||
blockSyncConcurrency int | ||
|
||
partitioner partitioner | ||
// Query gate which limits the maximum amount of concurrent queries. | ||
GiedriusS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
queryGate *Gate | ||
|
||
// samplesLimiter limits the number of samples per each Series() call. | ||
samplesLimiter *Limiter | ||
partitioner partitioner | ||
} | ||
|
||
// NewBucketStore creates a new bucket backed store that implements the store API against | ||
|
@@ -185,12 +212,19 @@ func NewBucketStore( | |
dir string, | ||
indexCacheSizeBytes uint64, | ||
maxChunkPoolBytes uint64, | ||
maxSampleCount uint64, | ||
maxConcurrent int, | ||
debugLogging bool, | ||
blockSyncConcurrency int, | ||
) (*BucketStore, error) { | ||
if logger == nil { | ||
logger = log.NewNopLogger() | ||
} | ||
|
||
if maxConcurrent < 0 { | ||
return nil, errors.Errorf("max concurrency value cannot be lower than 0 (got %v)", maxConcurrent) | ||
} | ||
|
||
indexCache, err := newIndexCache(reg, indexCacheSizeBytes) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "create index cache") | ||
|
@@ -202,6 +236,7 @@ func NewBucketStore( | |
|
||
const maxGapSize = 512 * 1024 | ||
|
||
metrics := newBucketStoreMetrics(reg) | ||
s := &BucketStore{ | ||
logger: logger, | ||
bucket: bucket, | ||
|
@@ -212,14 +247,18 @@ func NewBucketStore( | |
blockSets: map[uint64]*bucketBlockSet{}, | ||
debugLogging: debugLogging, | ||
blockSyncConcurrency: blockSyncConcurrency, | ||
queryGate: NewGate(maxConcurrent, extprom.NewSubsystem(reg, "thanos_bucket_store")), | ||
samplesLimiter: NewLimiter(maxSampleCount, metrics.queriesDropped), | ||
partitioner: gapBasedPartitioner{maxGapSize: maxGapSize}, | ||
} | ||
s.metrics = newBucketStoreMetrics(reg) | ||
s.metrics = metrics | ||
|
||
if err := os.MkdirAll(dir, 0777); err != nil { | ||
return nil, errors.Wrap(err, "create dir") | ||
} | ||
|
||
s.metrics.queriesLimit.Set(float64(maxConcurrent)) | ||
|
||
return s, nil | ||
} | ||
|
||
|
@@ -472,14 +511,15 @@ func (s *bucketSeriesSet) Err() error { | |
return s.err | ||
} | ||
|
||
func (s *BucketStore) blockSeries( | ||
func blockSeries( | ||
ctx context.Context, | ||
ulid ulid.ULID, | ||
extLset map[string]string, | ||
indexr *bucketIndexReader, | ||
chunkr *bucketChunkReader, | ||
matchers []labels.Matcher, | ||
req *storepb.SeriesRequest, | ||
samplesLimiter *Limiter, | ||
) (storepb.SeriesSet, *queryStats, error) { | ||
ps, err := indexr.ExpandedPostings(matchers) | ||
if err != nil { | ||
|
@@ -557,7 +597,7 @@ func (s *BucketStore) blockSeries( | |
} | ||
|
||
// Preload all chunks that were marked in the previous stage. | ||
if err := chunkr.preload(); err != nil { | ||
if err := chunkr.preload(samplesLimiter); err != nil { | ||
return nil, nil, errors.Wrap(err, "preload chunks") | ||
} | ||
|
||
|
@@ -661,10 +701,17 @@ func debugFoundBlockSetOverview(logger log.Logger, mint, maxt int64, lset labels | |
} | ||
|
||
// Series implements the storepb.StoreServer interface. | ||
// TODO(bwplotka): It buffers all chunks in memory and only then streams to client. | ||
// 1. Either count chunk sizes and error out too big query. | ||
// 2. Stream posting -> series -> chunk all together. | ||
func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storepb.Store_SeriesServer) error { | ||
{ | ||
span, _ := tracing.StartSpan(srv.Context(), "store_query_gate_ismyturn") | ||
err := s.queryGate.IsMyTurn(srv.Context()) | ||
span.Finish() | ||
if err != nil { | ||
return errors.Wrapf(err, "failed to wait for turn") | ||
} | ||
} | ||
defer s.queryGate.Done() | ||
|
||
matchers, err := translateMatchers(req.Matchers) | ||
if err != nil { | ||
return status.Error(codes.InvalidArgument, err.Error()) | ||
|
@@ -703,13 +750,14 @@ func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storepb.Store_Serie | |
defer runutil.CloseWithLogOnErr(s.logger, chunkr, "series block") | ||
|
||
g.Add(func() error { | ||
part, pstats, err := s.blockSeries(ctx, | ||
part, pstats, err := blockSeries(ctx, | ||
b.meta.ULID, | ||
b.meta.Thanos.Labels, | ||
indexr, | ||
chunkr, | ||
blockMatchers, | ||
req, | ||
s.samplesLimiter, | ||
) | ||
if err != nil { | ||
return errors.Wrapf(err, "fetch series for block %s", b.meta.ULID) | ||
|
@@ -1580,11 +1628,21 @@ func (r *bucketChunkReader) addPreload(id uint64) error { | |
} | ||
|
||
// preload all added chunk IDs. Must be called before the first call to Chunk is made. | ||
func (r *bucketChunkReader) preload() error { | ||
func (r *bucketChunkReader) preload(samplesLimiter *Limiter) error { | ||
const maxChunkSize = 16000 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could perhaps move this to the top along with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No necessarily as it only is used here |
||
|
||
var g run.Group | ||
|
||
numChunks := uint64(0) | ||
for _, offsets := range r.preloads { | ||
for range offsets { | ||
numChunks++ | ||
} | ||
} | ||
if err := samplesLimiter.Check(numChunks * maxSamplesPerChunk); err != nil { | ||
return errors.Wrapf(err, "exceeded samples limit") | ||
GiedriusS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
for seq, offsets := range r.preloads { | ||
sort.Slice(offsets, func(i, j int) bool { | ||
return offsets[i] < offsets[j] | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
package store | ||
|
||
import ( | ||
"context" | ||
"time" | ||
|
||
"github.com/improbable-eng/thanos/pkg/extprom" | ||
"github.com/prometheus/client_golang/prometheus" | ||
"github.com/prometheus/prometheus/pkg/gate" | ||
) | ||
|
||
// Gate wraps the Prometheus gate with extra metrics. | ||
type Gate struct { | ||
g *gate.Gate | ||
inflightQueries prometheus.Gauge | ||
gateTiming prometheus.Histogram | ||
} | ||
|
||
// NewGate returns a new query gate. | ||
func NewGate(maxConcurrent int, reg *extprom.SubsystemRegisterer) *Gate { | ||
g := &Gate{ | ||
g: gate.New(maxConcurrent), | ||
} | ||
g.inflightQueries = prometheus.NewGauge(prometheus.GaugeOpts{ | ||
Name: "queries_in_flight_total", | ||
Help: "Total number of queries that are currently in flight.", | ||
Subsystem: reg.Subsystem(), | ||
}) | ||
g.gateTiming = prometheus.NewHistogram(prometheus.HistogramOpts{ | ||
Name: "gate_duration_seconds", | ||
Help: "How many seconds it took for queries to wait at the gate.", | ||
Buckets: []float64{ | ||
0.01, 0.05, 0.1, 0.25, 0.6, 1, 2, 3.5, 5, 10, | ||
}, | ||
Subsystem: reg.Subsystem(), | ||
}) | ||
|
||
if r := reg.Registerer(); r != nil { | ||
r.MustRegister(g.inflightQueries, g.gateTiming) | ||
} | ||
|
||
return g | ||
} | ||
|
||
// IsMyTurn iniates a new query and waits until it's our turn to fulfill a query request. | ||
func (g *Gate) IsMyTurn(ctx context.Context) error { | ||
start := time.Now() | ||
defer func() { | ||
g.gateTiming.Observe(float64(time.Now().Sub(start))) | ||
}() | ||
|
||
if err := g.g.Start(ctx); err != nil { | ||
return err | ||
GiedriusS marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
g.inflightQueries.Inc() | ||
return nil | ||
} | ||
|
||
// Done finishes a query. | ||
func (g *Gate) Done() { | ||
g.inflightQueries.Dec() | ||
g.g.Done() | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone could tell that's too verbose, but I actually like this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 someone will definitely start wondering why 120 is here just like I have at the beginning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is fine 👍