Add a limits config option to allow for dropping of the cluster label #1726

cstyan · 2019-10-09T23:32:13Z

Adds an optional flag per user to drop the cluster label, in addition to dropping the replica label, when ingesting HA samples. Should only be used if that user has their own set of more than two labels that they use to uniquely identify Prometheus replicas.

There's a bit of a refactor here as well:

I noticed that in the config the ordering of the values was cluster label then replica label, but in RegisterFlags the order is replica then cluster. I moved them around in RegisterFlags to match the order in the struct.
distributor.Push was getting pretty long, I moved the internals of the validation loop into a function validateSeries. This also had the side effect of making that portion of the code testable. I've only added tests for the label removal bits. I assume the actual validation functions ValidateLabels and ValidateSamples are tested already.
renamed removeReplicaLabel -> removeLabel, it wasn't doing anything replica label specific anyways

Signed-off-by: Callum Styan callumstyan@gmail.com
cc @tomwilkie @gouthamve

bboreham · 2019-10-17T07:46:01Z

I find it easier to review PRs like this when you split the pure-refactoring part from the behaviour-changing part, into separate commits. Otherwise I have to figure out your intention on every line and that takes longer.

bboreham

I believe the code works, but I found it confusing. Probably I don't understand the overall design.

Given that "ha-tracker.drop-cluster-label" doesn't seem to be tied to HA, could the feature be done as a general label-removal mechanism? This would give us a portion of #771.

bboreham · 2019-10-17T07:41:39Z

docs/arguments.md

@@ -144,11 +144,13 @@ prefix these flags with `distributor.ha-tracker.`

 ### HA Tracker

-HA tracking has two of it's own flags:
+HA tracking has three of it's own flags:


Suggested change

HA tracking has three of it's own flags:

HA tracking has three of its own flags:

pkg/distributor/distributor.go

pkg/util/validation/limits.go

bboreham · 2019-10-17T08:05:42Z

pkg/distributor/distributor.go

-// Remove the replica label from a slice of LabelPairs if it exists.
-func removeReplicaLabel(replica string, labels *[]client.LabelAdapter) {
+// Remove the label labelname from a slice of LabelPairs if it exists.
+func removeLabel(labelName string, labels *[]client.LabelAdapter) {


This function is very similar to labelPairs.removeBlanks(), so they could be merged.
This one preserves order, which I don't think is necessary.
It might be a good idea to comment why this code does it.

I'll have a look at the labelsPairs function in the morning.

labelPairs is nested within the ingester package. If we're going to merge the two functions should that exist in a new package under util?

And the reason for preserving order is only that I've generally seen that be the case within Prometheus itself. For example within remote write, when processing a sample through external labelse and relabel rules, we preserve the sorted order of the labels.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

testability. Signed-off-by: Callum Styan <callumstyan@gmail.com>

in Distributor. Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan · 2019-10-18T03:42:25Z

@bboreham thanks for the review! I've split out the refactoring into another commit. I also changed the validateSeries function to no longer require the error pointer argument in addition to returning an error.

bboreham · 2019-10-18T11:21:59Z

Given that it isn't tied to HA, could the feature be done as a general label-removal mechanism? This would give us a portion of #771.

cstyan · 2019-10-18T20:48:51Z

I moved the cluster label dropping inside the check for removeReplica, which is tied to HA. I can move that back out and turn the label removal into a more general non-HA dependent thing, maybe in another PR? if you'd like.

bboreham · 2019-10-24T15:01:43Z

Given that it doesn't cost me anything, yes I would prefer a generic "for tenant X, everywhere you see label Y, drop it".

nbellowe · 2019-11-27T03:32:38Z

@cstyan Any updates with this PR? Its much appreciated, we're waiting on this to enable HA writing to cortex.

bboreham

getting close

docs/arguments.md

pkg/util/validation/limits.go

specific. Signed-off-by: Callum Styan <callumstyan@gmail.com>

bboreham

lgtm

gouthamve · 2019-12-03T22:45:10Z

pkg/distributor/distributor.go

@@ -280,6 +281,48 @@ func (d *Distributor) checkSample(ctx context.Context, userID, cluster, replica
 	return true, nil
 }

+// Validates a single series from a write request. Will remove HA labels if necessary.
+// Takes a pointer for a partial error so that we can get partial errors, errors during validation


Can you update the comment to remove the reference to the pointer?

gouthamve · 2019-12-03T22:47:24Z

pkg/util/validation/limits.go

-// HAClusterLabel returns the cluster label to look for when deciding whether to accept a sample from a Prometheus HA replica.
-func (o *Overrides) HAClusterLabel(userID string) string {
-	return o.overridesManager.GetLimits(userID).(*Limits).HAClusterLabel
+// DropLabels returns whether the cluster label should be dropped when ingesting HA samples for the user.


Returns the list of labels to be dropped?

gouthamve · 2019-12-03T22:48:57Z

pkg/distributor/distributor.go

+	if removeReplica {
+		removeLabel(d.limits.HAReplicaLabel(userID), &ts.Labels)
+	}
+	for _, s := range d.limits.DropLabels(userID) {


s is a weird variable name in this instance, maybe call it label/labelName?

pkg/distributor/distributor.go

empty labels after dropping labels Signed-off-by: Callum Styan <callumstyan@gmail.com>

nbellowe · 2019-12-05T05:26:15Z

Thank you @cstyan!

Add changelog item for #1726

bboreham reviewed Oct 17, 2019

View reviewed changes

cstyan added 3 commits October 17, 2019 19:33

Rename removeReplicaLabel to more generic removeLabel.

83318ba

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Refactor Push; move some logic into another function validateSeries for

0c18eb2

testability. Signed-off-by: Callum Styan <callumstyan@gmail.com>

Add a per user option to remove cluster label when ingesting HA samples

e8e42d9

in Distributor. Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan mentioned this pull request Nov 4, 2019

Add ability to drop cluster label after dedupe #1724

Closed

cstyan requested a review from bboreham November 21, 2019 23:14

bboreham reviewed Nov 28, 2019

View reviewed changes

docs/arguments.md Outdated Show resolved Hide resolved

pkg/util/validation/limits.go Show resolved Hide resolved

Make label dropping generic instead of cluster label/HA tracker

82969da

specific. Signed-off-by: Callum Styan <callumstyan@gmail.com>

bboreham approved these changes Nov 29, 2019

View reviewed changes

gouthamve reviewed Dec 3, 2019

View reviewed changes

review comments; fix some comments, more descriptive var name, check for

9954be4

empty labels after dropping labels Signed-off-by: Callum Styan <callumstyan@gmail.com>

gouthamve approved these changes Dec 3, 2019

View reviewed changes

gouthamve merged commit 08ddf88 into cortexproject:master Dec 3, 2019

cstyan mentioned this pull request Dec 3, 2019

Add changelog item for #1726 #1875

Merged

bboreham added a commit that referenced this pull request Dec 5, 2019

Merge pull request #1875 from cstyan/label-drop-changelog

498476e

Add changelog item for #1726

pracucci mentioned this pull request Dec 6, 2019

Fixed per-series sharding token generation in the distributor #1888

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a limits config option to allow for dropping of the cluster label #1726

Add a limits config option to allow for dropping of the cluster label #1726

cstyan commented Oct 9, 2019

bboreham commented Oct 17, 2019

bboreham left a comment

bboreham Oct 17, 2019

bboreham Oct 17, 2019

cstyan Oct 18, 2019

cstyan Nov 28, 2019

cstyan commented Oct 18, 2019

bboreham commented Oct 18, 2019

cstyan commented Oct 18, 2019

bboreham commented Oct 24, 2019

nbellowe commented Nov 27, 2019

bboreham left a comment

bboreham left a comment

gouthamve Dec 3, 2019

gouthamve Dec 3, 2019

gouthamve Dec 3, 2019

nbellowe commented Dec 5, 2019

	HA tracking has three of it's own flags:
	HA tracking has three of its own flags:

Add a limits config option to allow for dropping of the cluster label #1726

Add a limits config option to allow for dropping of the cluster label #1726

Conversation

cstyan commented Oct 9, 2019

bboreham commented Oct 17, 2019

bboreham left a comment

Choose a reason for hiding this comment

bboreham Oct 17, 2019

Choose a reason for hiding this comment

bboreham Oct 17, 2019

Choose a reason for hiding this comment

cstyan Oct 18, 2019

Choose a reason for hiding this comment

cstyan Nov 28, 2019

Choose a reason for hiding this comment

cstyan commented Oct 18, 2019

bboreham commented Oct 18, 2019

cstyan commented Oct 18, 2019

bboreham commented Oct 24, 2019

nbellowe commented Nov 27, 2019

bboreham left a comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

gouthamve Dec 3, 2019

Choose a reason for hiding this comment

gouthamve Dec 3, 2019

Choose a reason for hiding this comment

gouthamve Dec 3, 2019

Choose a reason for hiding this comment

nbellowe commented Dec 5, 2019