Query: allow multiple deduplication label. #1362

krasi-georgiev · 2019-07-31T14:28:11Z

Closes: #1174

Changes

Can start the querier with multi replica labels like:

--query.replica-label=prometheus_replica --query.replica-label=service

querier api supports replicaLabels url param which will overwrite the --query.replica-label cli flag to allow dynamic deduplication at query time.

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2019-08-02T14:16:17Z

ping @bwplotka @brancz @squat

povilasv

Nice work 🥇 don't really have anything to comment on :D

Would be great if somebody else would also take a look.

brancz

Generally this looks like the right direction, just confirming two things.

pkg/query/api/v1.go

brancz · 2019-08-09T12:26:48Z

pkg/query/iter.go

@@ -325,10 +325,10 @@ func (s *dedupSeriesSet) Next() bool {
 // replica label if it exists
 func (s *dedupSeriesSet) peekLset() labels.Labels {
 	lset := s.peek.Labels()
-	if lset[len(lset)-1].Name != s.replicaLabel {
+	if _, ok := s.replicaLabels[lset[len(lset)-1].Name]; !ok {


I would have expected that we need to verify each replica label here. I feel like I'm missing something about this logic though, can you explain what's happening here?

If the last label is not in the replicaLabels list than no replica labels are set.
Replica labels are always at the end of the list so if the last one is not present in the s.replicaLabels map this guarantees that no replica labels need to be stripped.

If the last labels in the full list is a replica label than strip all replica labels by removing len(s.replicaLabels) labels from the full labels list.

Worth to comment this indeed.

bwplotka

Nice! Thanks for detailed tests and doc improvements.

Some suggestions and inconsistencies, otherwise LGTM

pkg/query/api/v1.go

bwplotka · 2019-08-27T20:06:34Z

pkg/query/iter.go

@@ -325,10 +325,10 @@ func (s *dedupSeriesSet) Next() bool {
 // replica label if it exists
 func (s *dedupSeriesSet) peekLset() labels.Labels {
 	lset := s.peek.Labels()
-	if lset[len(lset)-1].Name != s.replicaLabel {
+	if _, ok := s.replicaLabels[lset[len(lset)-1].Name]; !ok {


Worth to comment this indeed.

pkg/query/iter.go

pkg/query/querier.go

pkg/query/querier_test.go

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka

Awesome! I think this make sense.

There is a bug (type) but otherwise LGTM! (: Thanks and sorry for delay.

docs/components/query.md

go.mod

pkg/query/api/v1.go

bwplotka · 2019-09-11T16:17:27Z

pkg/query/api/v1.go

@@ -212,6 +230,7 @@ func (api *API) parsePartialResponseParam(r *http.Request) (enablePartialRespons
 	const partialResponseParam = "partial_response"
 	enablePartialResponse = api.enablePartialResponse

+	// Overwrite the cli flag when provided as a query parameter.


Did you mean to put this below?

Anyway I would be fine with just what we have in doc, so maybe we can drop this comment?

No this is the right place and it is for the partialResponseParam
I prefer to keep the comment here as I was a bit confused why this is done this way when I first saw it.

Why you put it here then and not for parseDownsamplingParamMillis and parseReplicaLabelsParam? I don't get it ):

bwplotka · 2019-09-11T16:48:52Z

pkg/query/iter.go

+	// Check how many replica labels are present so that these are removed.
+	var totalToRemove int
+	for index := 0; index < len(s.replicaLabels); index++ {
+		if _, ok := s.replicaLabels[lset[len(lset)-index-1].Name]; ok {


Nice, that might work nicely 👍

pkg/query/querier_test.go

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka · 2019-09-16T15:48:53Z

Looks like netlify OOMs (: https://app.netlify.com/sites/thanos-io/deploys/5d7f620fb4a56e00093ad620

bwplotka

LGTM!

Thanks!

bwplotka · 2019-09-13T13:59:46Z

pkg/query/api/v1.go

@@ -212,6 +230,7 @@ func (api *API) parsePartialResponseParam(r *http.Request) (enablePartialRespons
 	const partialResponseParam = "partial_response"
 	enablePartialResponse = api.enablePartialResponse

+	// Overwrite the cli flag when provided as a query parameter.


Why you put it here then and not for parseDownsamplingParamMillis and parseReplicaLabelsParam? I don't get it ):

krasi-georgiev · 2019-09-16T18:57:53Z

Why you put it here then and not for parseDownsamplingParamMillis and parseReplicaLabelsParam? I don't get it ):

I see what you mean now , yes that would be better to be as a comment to the function itself.

If I don't forget will open a PR.

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev added 8 commits July 31, 2019 17:18

allow multiple deduplication label.

bf4cdf3

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

changelog

0837d37

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

docs

6ded7be

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bug in the replicate labels stripping

afac811

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

extended the TestDedupSeriesSet with multi replica labels.

01d227d

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

docs update

a35f320

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

added replicaLabels to api

d8d83e0

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

updated docs for the API replica labels.

707eca9

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the multi-dedup-labels branch from fd3d893 to 707eca9 Compare August 2, 2019 12:05

krasi-georgiev marked this pull request as ready for review August 2, 2019 12:42

krasi-georgiev changed the title ~~[WIP] Query: allow multiple deduplication label.~~ Query: allow multiple deduplication label. Aug 2, 2019

povilasv approved these changes Aug 6, 2019

View reviewed changes

bwplotka added this to To do in M1 Aug 8, 2019

bwplotka moved this from To do to Review in progress in M1 Aug 8, 2019

bwplotka moved this from Review in progress to In progress in M1 Aug 8, 2019

brancz reviewed Aug 9, 2019

View reviewed changes

bwplotka reviewed Aug 27, 2019

View reviewed changes

krasi-georgiev added 4 commits August 28, 2019 11:56

Merge remote-tracking branch 'upstream/master' into multi-dedup-labels

bcc6d9d

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

missing dedup label from some series

fccf8c8

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

nice review changes

ae7faf1

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into multi-dedup-labels

35ac58b

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the multi-dedup-labels branch from 42c98d9 to 35ac58b Compare August 28, 2019 15:24

bwplotka reviewed Sep 11, 2019

View reviewed changes

krasi-georgiev added 3 commits September 12, 2019 14:42

nits

841b889

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

replicaLabels typo

b292897

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into multi-dedup-labels

b85ce29

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the multi-dedup-labels branch from ab5488b to bf7e0f5 Compare September 13, 2019 08:44

added api replica tests

5c4ae8c

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the multi-dedup-labels branch from bf7e0f5 to 5c4ae8c Compare September 13, 2019 08:49

krasi-georgiev force-pushed the multi-dedup-labels branch from 34f3aed to b22f7b8 Compare September 16, 2019 10:19

use tsdb storage package for the proxy

b8cd889

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the multi-dedup-labels branch from b22f7b8 to b8cd889 Compare September 16, 2019 10:20

bwplotka approved these changes Sep 16, 2019

View reviewed changes

bwplotka merged commit f4757f5 into thanos-io:master Sep 16, 2019

M1 automation moved this from In progress to Done Sep 16, 2019

wbh1 pushed a commit to wbh1/thanos that referenced this pull request Sep 17, 2019

Query: allow multiple deduplication label. (thanos-io#1362)

d289537

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

amalucelli mentioned this pull request Sep 26, 2019

query: multiple 'query.replica-label' flag #1571

Closed

jojohappy mentioned this pull request Sep 27, 2019

Fixed CHANGELOG #1578

Merged

wiardvanrij mentioned this pull request Jun 12, 2020

Feature/multiple replica labels [Thanos] banzaicloud/banzai-charts#1099

Merged

3 tasks

krasi-georgiev deleted the multi-dedup-labels branch September 1, 2020 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query: allow multiple deduplication label. #1362

Query: allow multiple deduplication label. #1362

krasi-georgiev commented Jul 31, 2019 •

edited

Loading

krasi-georgiev commented Aug 2, 2019

povilasv left a comment

brancz left a comment

brancz Aug 9, 2019

krasi-georgiev Aug 9, 2019

bwplotka Aug 27, 2019

bwplotka left a comment

bwplotka Aug 27, 2019

bwplotka left a comment

bwplotka Sep 11, 2019

bwplotka Sep 11, 2019

krasi-georgiev Sep 12, 2019 •

edited

Loading

bwplotka Sep 13, 2019

bwplotka Sep 11, 2019

bwplotka commented Sep 16, 2019

bwplotka left a comment

bwplotka Sep 13, 2019

krasi-georgiev commented Sep 16, 2019

Query: allow multiple deduplication label. #1362

Query: allow multiple deduplication label. #1362

Conversation

krasi-georgiev commented Jul 31, 2019 • edited Loading

Changes

krasi-georgiev commented Aug 2, 2019

povilasv left a comment

Choose a reason for hiding this comment

brancz 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

bwplotka left a comment

Choose a reason for hiding this 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

krasi-georgiev Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Sep 16, 2019

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krasi-georgiev commented Sep 16, 2019

krasi-georgiev commented Jul 31, 2019 •

edited

Loading

krasi-georgiev Sep 12, 2019 •

edited

Loading