querier: adjust requested limit parameter for downstream requests to series endpoint #10652
Conversation
narqo
changed the title
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
narqo
force-pushed
the
vldmr/series-api-limit
branch
from
b17685e to
4d7ffd2
Compare
narqo
changed the title
pkg/distributor/distributor.go
Outdated
| if d.cfg.IngestStorageConfig.Enabled { | ||
| // When ingest storage is enabled, each partition, represented by one replication set is owned by only one ingester. | ||
| // So we use the number of replication sets to count the number of shards. | ||
| shardSize = len(replicationSets) |
There was a problem hiding this comment.
len(replicationSets) includes both ACTIVE and INACTIVE partitions. To be more accurate, I think we should only consider ACTIVE partitions (the shard size on the write path, where series are written, is made only of ACTIVE partitions). It may not be a big deal: if we decide to not address it, then I would at least leave a comment here about it.
pkg/distributor/distributor.go
Outdated
| } else if len(replicationSets) == 1 { | ||
| // We expect to always have exactly 1 replication set when ingest storage is disabled. | ||
| // In classic Mimir the total number of shards (ingestion-tenant-shard-size) is the number of ingesters in the shard across all zones. | ||
| shardSize = len(replicationSets[0].Instances) / d.ingestersRing.ReplicationFactor() |
There was a problem hiding this comment.
Similar comment about read-only instances here. Another case where the number of instances is higher than the real shard size is when the "lookback" threshold triggers (e.g. for 12h after a scale up). The number of instances we lookup is higher than the real shard size. I think what we really want here is the shard size as computed on the write path, and not the read path. It would be more accurate computing the actual shard size by looking at the min between "configured tenant shard size" and "writeable instances/partitions in the ring".
There was a problem hiding this comment.
I've updated the PR. The idea makes sense to me, although I'm not fully sure whether I've picked the right methods in the ring to calculate what you'd suggested. PTAL
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
Signed-off-by: Vladimir Varankin <[email protected]>
narqo
force-pushed
the
vldmr/series-api-limit
branch
from
8ae53eb to
4bfdff4
Compare
What this PR does
This is a follow-up to #10620
Here I'm updating the
MetricsForLabelMatchersto adjust thelimitparameter passed to the downstream ingesters, base on the replication sets. The idea, suggested by @pracucci is to approximate the limit as:Note that for ingest storage, the above formula is equivalent to
L / <partition-shard-size>.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.