Allow Cortex to receive WriteRequest with Metadata. #2336

gotjosh · 2020-03-26T17:32:23Z

What this PR does:

Now that Prometheus remote write supports sending of Metadata, this change allows Cortex to receive the metadata and shard it accordingly.

Which issue(s) this PR fixes:

This partially implements the proposal accepted as part of #2311

Checklist

e2e test added
Unit tests for the distributors
TODOs resolved
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

I've decided to split this across two PRs to manage the changes made and make it easy on the reviewer. There are still some lingering questions that I'm hoping to resolve with the help of any maintainer.

Signed-off-by: gotjosh [email protected]

pkg/distributor/distributor.go

pkg/ingester/client/timeseries.go

gotjosh · 2020-03-26T18:09:08Z

pkg/ingester/ingester.go

This is only here temporary, I'd like to some feedback on what I've done before I open a PR with the rest of the change.

pracucci

Good job @gotjosh (and sorry for the delay in the initial review). The overall design LGTM. I'm a bit dubious about the increased complexity of Distributor.Push() but I would like to discuss it in a second pass. My main feedback is that I've the feeling PreallocMetricMetadata is unncessary and just makes things more complicated.

pkg/util/validation/limits.go

pkg/util/validation/validate.go

pkg/distributor/distributor.go

pkg/ingester/client/cortex.proto

pracucci

Very good! I just left few small comments. A part from this, I would be glad to see the Push() logic covered in a unit test if it's not too complicated (given the ingester logic has not been built yet), otherwise you can do it in the next PR (up to you).

pkg/distributor/distributor.go

pracucci · 2020-04-02T15:57:19Z

pkg/distributor/distributor.go

If it's true that a push request can only contain series or metadata, why don't we return error? I would cost us nearly zero effort to make it not a best-effort from the Cortex side.

If it's true that a push request can only contain series or metadata, why don't we return error?

It is for now, but more importantly, it is something that we think will change in the future (given other Prometheus integrations e.g. Stackdriver) require metadata to be propagated with the series. The key takeaways here are a) We don't want to couple that implementation with Cortex and b) We want to prioritise samples over metadata.

I think we can this conversation again when I submit the refactor to send series + samples together to ingesters as the problem will be pushed there, but for now, I would feel more confident to just log the error for now and make sure we ingester the samples. WDYT?

In the code above there's the following comment. I think it's misleading. Either we assume a request can contain both or not. I'm OK if we do, but we should simply reflect that comment just saying A WriteRequest can include samples and metadata.

// A WriteRequest can now include samples and metadata. However, at the moment of writing, // we can only receive samples and no metadata or metadata and no samples.

About whether returning error or not, I'm OK keeping it as is (just logging error). We can always change it in the future to more stricter policy if required.

In the code above there's the following comment. I think it's misleading. Either we assume a request can contain both or not.

It's a bit tricky, but I see your point. I'll change to state:

A WriteRequest can only contain series or metadata but not both. This might change in the future.

pkg/distributor/distributor.go

pracucci

Thanks @gotjosh for your hard work! You did a great job and this first PR LGTM 👏

pstibrany

I haven't finished my review yet, but I don't want to sit on feedback I have so far.

pkg/distributor/distributor.go

pstibrany · 2020-04-03T09:32:57Z

pkg/distributor/distributor.go

This method has changed significantly, and mixes handling of samples and metadata. Would it make sense to split Push into two, to handle these two cases separately? (Push calling pushMetadata and pushSamples or something similar).

I plan to add another change that merges the two. Since ingesters receive a WriteRequest, they can just go together. I just wanted to avoid a big refactor upfront.

Hmm, ok. Push is quite big at the moment and could use some trimming.

pkg/distributor/distributor.go

Now that Prometheus remote write supports sending of Metadata, this change allows Cortex to receive the metadata and shard it acordingly. I've decided to split this across two PRs to manage the changes made and make it easy on the reviewer. Signed-off-by: gotjosh <[email protected]>

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: gotjosh <[email protected]>

Signed-off-by: gotjosh <[email protected]>

- Continue validating the rest of the metadata on error. - Move the subRing initialisationg after the null check. - Supply a "msg" key to the metadata failure log line. - Remove TODO about the rate limits Signed-off-by: gotjosh <[email protected]>

Signed-off-by: gotjosh <[email protected]>

- Removed the checks around metadata/samples iteration. Don't need them anymore. - Changed the comment wrt WriteRequest samples/metadata handling. Signed-off-by: gotjosh <[email protected]>

Signed-off-by: gotjosh <[email protected]>

pkg/util/validation/validate.go

Signed-off-by: gotjosh <[email protected]>

pstibrany · 2020-04-03T16:40:42Z

pkg/distributor/distributor.go

+func (d *Distributor) validateMetadata(m *ingester_client.MetricMetadata, userID string) error {
+	return validation.ValidateMetadata(d.limits, userID, m)
+}
+


I'm not sure this method brings anything at the moment. Should we get rid of it?

As you're going to work on this, let's move it to the next PR.

pstibrany · 2020-04-03T16:48:14Z

pkg/distributor/distributor.go

Hmm, ok. Push is quite big at the moment and could use some trimming.

pstibrany · 2020-04-03T16:52:57Z

pull-request-size bot added size/L and removed size/XL labels 3 hours ago

haha, good bot :)

pstibrany · 2020-04-03T16:53:30Z

@gotjosh Nice job, looks good to me. Can we fix CHANGELOG entry formatting? :)

Signed-off-by: Peter Štibraný <[email protected]>

pull-request-size bot added the size/XL label Mar 26, 2020

gotjosh commented Mar 26, 2020

View reviewed changes

pkg/distributor/distributor.go Outdated Show resolved Hide resolved

gotjosh commented Mar 26, 2020

View reviewed changes

pkg/distributor/distributor.go Outdated Show resolved Hide resolved

gotjosh commented Mar 26, 2020

View reviewed changes

pkg/distributor/distributor.go Outdated Show resolved Hide resolved

gotjosh commented Mar 26, 2020

View reviewed changes

pkg/distributor/distributor.go Outdated Show resolved Hide resolved

gotjosh commented Mar 26, 2020

View reviewed changes

pkg/distributor/distributor.go Outdated Show resolved Hide resolved

gotjosh commented Mar 26, 2020

View reviewed changes

pkg/ingester/client/timeseries.go Outdated Show resolved Hide resolved

gotjosh commented Mar 26, 2020

View reviewed changes

pracucci reviewed Apr 1, 2020

View reviewed changes

gotjosh mentioned this pull request Apr 1, 2020

Return first validation error on sample ingestion #2383

Merged

3 tasks

gotjosh force-pushed the distributors-metadata branch 2 times, most recently from f5a9ac0 to 9743ba9 Compare April 1, 2020 20:09

gotjosh commented Apr 2, 2020

View reviewed changes

pkg/ingester/client/cortex.proto Outdated Show resolved Hide resolved

gotjosh requested a review from pracucci April 2, 2020 09:45

gotjosh force-pushed the distributors-metadata branch from 9743ba9 to 073aa36 Compare April 2, 2020 10:49

pracucci reviewed Apr 2, 2020

View reviewed changes

gotjosh force-pushed the distributors-metadata branch from 9f96443 to df60bc9 Compare April 2, 2020 21:30

gotjosh requested a review from pracucci April 3, 2020 01:34

pracucci approved these changes Apr 3, 2020

View reviewed changes

pstibrany reviewed Apr 3, 2020

View reviewed changes

gotjosh and others added 10 commits April 3, 2020 13:11

Update pkg/util/validation/validate.go

a81e8b2

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: gotjosh <[email protected]>

Apply suggestions from code review

1d9da92

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: gotjosh <[email protected]>

Update pkg/distributor/distributor.go

fb8074a

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: gotjosh <[email protected]>

Update pkg/util/validation/limits.go

ae66828

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: gotjosh <[email protected]>

Address review comments

f16c5f7

Signed-off-by: gotjosh <[email protected]>

Appease the linter.

25c4efd

Signed-off-by: gotjosh <[email protected]>

Save the metadata error.

ad8f4b3

Signed-off-by: gotjosh <[email protected]>

Address review comments.

05188b5

- Continue validating the rest of the metadata on error. - Move the subRing initialisationg after the null check. - Supply a "msg" key to the metadata failure log line. - Remove TODO about the rate limits Signed-off-by: gotjosh <[email protected]>

Add tests to distributor Push including metadata

45b1f53

Signed-off-by: gotjosh <[email protected]>

gotjosh added 2 commits April 3, 2020 13:28

Address review comments

d397fe4

- Removed the checks around metadata/samples iteration. Don't need them anymore. - Changed the comment wrt WriteRequest samples/metadata handling. Signed-off-by: gotjosh <[email protected]>

Minor adjustments

f96a4cd

Signed-off-by: gotjosh <[email protected]>

pracucci reviewed Apr 3, 2020

View reviewed changes

pkg/util/validation/validate.go Outdated Show resolved Hide resolved

Register the DiscardedMetadata metric

ca5e188

Signed-off-by: gotjosh <[email protected]>

gotjosh force-pushed the distributors-metadata branch from 2ea3164 to ca5e188 Compare April 3, 2020 12:47

Fix tests after rebase

d612f76

Signed-off-by: gotjosh <[email protected]>

gotjosh requested a review from pstibrany April 3, 2020 13:27

pull-request-size bot added size/L and removed size/XL labels Apr 3, 2020

pstibrany reviewed Apr 3, 2020

View reviewed changes

pstibrany approved these changes Apr 3, 2020

View reviewed changes

Update CHANGELOG.md

51cf00b

Signed-off-by: Peter Štibraný <[email protected]>

pracucci merged commit 8fe97dc into cortexproject:master Apr 3, 2020

gotjosh mentioned this pull request Apr 6, 2020

Refactor: Unify metadata and samples sends to ingesters #2410

Merged

3 tasks

gotjosh mentioned this pull request Apr 14, 2020

Handle metric metadata in the ingesters #2459

Merged

18 tasks

Allow Cortex to receive WriteRequest with Metadata. #2336

Allow Cortex to receive WriteRequest with Metadata. #2336

Uh oh!

Conversation

gotjosh commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany commented Apr 3, 2020

Uh oh!

pstibrany commented Apr 3, 2020

Uh oh!

Uh oh!

gotjosh commented Mar 26, 2020 •

edited

Loading