Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Query: allow multiple deduplication label. #1362

Merged
merged 17 commits into from
Sep 16, 2019
Merged

Query: allow multiple deduplication label. #1362

merged 17 commits into from
Sep 16, 2019

Conversation

krasi-georgiev
Copy link
Contributor

@krasi-georgiev krasi-georgiev commented Jul 31, 2019 •
edited
Loading

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.

@krasi-georgiev
allow multiple deduplication label.
bf4cdf3 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
changelog
0837d37 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
docs
6ded7be 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
bug in the replicate labels stripping
afac811 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
extended the TestDedupSeriesSet with multi replica labels.
01d227d 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
docs update
a35f320 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
added replicaLabels to api
d8d83e0 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev
updated docs for the API replica labels.
707eca9 
Signed-off-by: Krasi Georgiev <[email protected]>
@krasi-georgiev krasi-georgiev marked this pull request as ready for review August 2, 2019 12:42
@krasi-georgiev krasi-georgiev changed the title [WIP] Query: allow multiple deduplication label. Query: allow multiple deduplication label. Aug 2, 2019
@krasi-georgiev
Copy link
Contributor Author

ping @bwplotka @brancz @squat

povilasv
povilasv approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@povilasv povilasv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

brancz
brancz reviewed Aug 9, 2019
View reviewed changes
Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkg/query/api/v1.go Outdated Show resolved Hide resolved
pkg/query/iter.go Outdated
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth to comment this indeed.

bwplotka
bwplotka reviewed Aug 27, 2019
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for detailed tests and doc improvements.

Some suggestions and inconsistencies, otherwise LGTM

pkg/query/api/v1.go Outdated Show resolved Hide resolved
pkg/query/api/v1.go Outdated Show resolved Hide resolved
pkg/query/iter.go Outdated
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth to comment this indeed.

pkg/query/iter.go Outdated Show resolved Hide resolved
pkg/query/querier.go Outdated Show resolved Hide resolved
pkg/query/querier_test.go Show resolved Hide resolved
bwplotka
bwplotka reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I think this make sense.

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

docs/components/query.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/query/api/v1.go Outdated Show resolved Hide resolved
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to put this below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@krasi-georgiev krasi-georgiev Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that might work nicely 👍

pkg/query/querier_test.go Show resolved Hide resolved
@krasi-georgiev
added api replica tests
5c4ae8c 
Signed-off-by: Krasi Georgiev <[email protected]>
@bwplotka
Copy link
Member

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

bwplotka
bwplotka approved these changes Sep 16, 2019
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Thanks!

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bwplotka bwplotka merged commit f4757f5 into thanos-io:master Sep 16, 2019
@krasi-georgiev
Copy link
Contributor Author

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.

wbh1 pushed a commit to wbh1/thanos that referenced this pull request Sep 17, 2019
@krasi-georgiev @wbh1
Query: allow multiple deduplication label. (thanos-io#1362)
d289537 
Signed-off-by: Krasi Georgiev <[email protected]>
@amalucelli amalucelli mentioned this pull request Sep 26, 2019
Closed
@jojohappy jojohappy mentioned this pull request Sep 27, 2019
Merged
@wiardvanrij wiardvanrij mentioned this pull request Jun 12, 2020
Merged
3 tasks
@krasi-georgiev krasi-georgiev deleted the multi-dedup-labels branch September 1, 2020 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brancz brancz brancz left review comments

@povilasv povilasv povilasv approved these changes

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to use multiple dedup labels
4 participants
@krasi-georgiev @bwplotka @brancz @povilasv