receive: Enable exemplars ingestion and querying #4292

onprem · 2021-06-01T05:08:51Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Enable exemplar ingestion in Receive
Add exemplars API implementation for TSDB and MultiTSDB
Expose exemplars gRPC API from Receive

Verification

Local testing, modified some existing tests for ingestion. Need to add E2E tests.

wiardvanrij

I've done a review but it's not on the technical part but more 'usability' part :) Really looking forward to this feature 👍

pkg/exemplars/multitsdb.go

wiardvanrij · 2021-06-01T19:28:41Z

pkg/receive/writer.go

@@ -104,6 +135,18 @@ func (r *Writer) Write(ctx context.Context, tenantID string, wreq *prompb.WriteR
 		level.Warn(r.logger).Log("msg", "Error on ingesting samples that are too old or are too far into the future", "numDropped", numOutOfBounds)
 		errs.Add(errors.Wrapf(storage.ErrOutOfBounds, "add %d samples", numOutOfBounds))
 	}
+	if numExemplarsOutOfOrder > 0 {
+		level.Warn(r.logger).Log("msg", "Error on ingesting out-of-order exemplars", "numDropped", numExemplarsOutOfOrder)


It's a level warn, but the logger msg says it's an error. I would make the message more generic and let log level handle the prefix, like: "warn: exemplars out of order" or something like that.

I was following what we do for samples. I'd like other's opinion here as well to either switch to level.Error, remove Error from message, or keep it as it.

I think the message was there to indicate there is an error, but it's not fatal maybe?

I think we are not too strict here, so it's fine to say error if it's only a warning about the not serious error.

If we handle the returned error on upper levels (this could be logging as well), we don't need to log/handle it twice. I see that you've just followed the existing structure so it seems ok for now.

hm.. what I don't like is duplicate error handling. We both add error and warn error... Wonder if we could just handle that on caller side instead. 🤔

Yeah, this is a bit icky but as Kemal noted I was just following the existing structure we use for sample. I think I can try refactoring this in a follow up PR if that's ok.

pkg/receive/writer.go

cmd/thanos/receive.go

pkg/receive/multitsdb_test.go

pkg/exemplars/multitsdb.go

pkg/exemplars/tsdb.go

yeya24

I don't see the code of forwarding exemplars. I assume only timeserieses are forwarded. Do you want to include the exemplars part in this pr?

pkg/receive/handler.go

pkg/exemplars/tsdb.go

pkg/receive/multitsdb.go

pkg/exemplars/tsdb.go

yeya24

Really awesome work! Thanks for your patience.

yeya24

Nice! LGTM

kakkoyun

LGTM 🚀

kakkoyun · 2021-06-11T06:58:07Z

pkg/receive/writer.go

@@ -104,6 +135,18 @@ func (r *Writer) Write(ctx context.Context, tenantID string, wreq *prompb.WriteR
 		level.Warn(r.logger).Log("msg", "Error on ingesting samples that are too old or are too far into the future", "numDropped", numOutOfBounds)
 		errs.Add(errors.Wrapf(storage.ErrOutOfBounds, "add %d samples", numOutOfBounds))
 	}
+	if numExemplarsOutOfOrder > 0 {
+		level.Warn(r.logger).Log("msg", "Error on ingesting out-of-order exemplars", "numDropped", numExemplarsOutOfOrder)


If we handle the returned error on upper levels (this could be logging as well), we don't need to log/handle it twice. I see that you've just followed the existing structure so it seems ok for now.

bwplotka

This is epic!!! LGTM 💪🏽

From when @onprem you are full-pledge backend engineer with so much skill for high quality work like this? 😱

bwplotka · 2021-06-11T07:03:27Z

pkg/receive/writer.go

@@ -104,6 +135,18 @@ func (r *Writer) Write(ctx context.Context, tenantID string, wreq *prompb.WriteR
 		level.Warn(r.logger).Log("msg", "Error on ingesting samples that are too old or are too far into the future", "numDropped", numOutOfBounds)
 		errs.Add(errors.Wrapf(storage.ErrOutOfBounds, "add %d samples", numOutOfBounds))
 	}
+	if numExemplarsOutOfOrder > 0 {
+		level.Warn(r.logger).Log("msg", "Error on ingesting out-of-order exemplars", "numDropped", numExemplarsOutOfOrder)


hm.. what I don't like is duplicate error handling. We both add error and warn error... Wonder if we could just handle that on caller side instead. 🤔

kakkoyun · 2021-06-11T10:06:50Z

Let's rebase and merge this

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

bwplotka

Solid work 😱

LGTM!

onprem force-pushed the ingest-exemplars branch from e7bbd4c to 8b71925 Compare June 1, 2021 05:16

wiardvanrij reviewed Jun 1, 2021

View reviewed changes

sonatype-lift bot reviewed Jun 1, 2021

View reviewed changes

pkg/receive/multitsdb_test.go Outdated Show resolved Hide resolved

yeya24 reviewed Jun 1, 2021

View reviewed changes

pkg/exemplars/multitsdb.go Show resolved Hide resolved

pkg/exemplars/tsdb.go Show resolved Hide resolved

onprem marked this pull request as ready for review June 2, 2021 12:14

yeya24 reviewed Jun 3, 2021

View reviewed changes

pkg/receive/handler.go Show resolved Hide resolved

yeya24 reviewed Jun 5, 2021

View reviewed changes

pkg/exemplars/tsdb.go Outdated Show resolved Hide resolved

pkg/exemplars/tsdb.go Show resolved Hide resolved

yeya24 reviewed Jun 5, 2021

View reviewed changes

pkg/receive/multitsdb.go Outdated Show resolved Hide resolved

pkg/exemplars/tsdb.go Show resolved Hide resolved

yeya24 reviewed Jun 5, 2021

View reviewed changes

yeya24 previously approved these changes Jun 7, 2021

View reviewed changes

onprem dismissed yeya24’s stale review via 56f141d June 9, 2021 20:31

onprem force-pushed the ingest-exemplars branch from 56f141d to 7b4cb37 Compare June 10, 2021 12:17

kakkoyun previously approved these changes Jun 11, 2021

View reviewed changes

kakkoyun requested review from bwplotka and yeya24 June 11, 2021 06:59

bwplotka previously approved these changes Jun 11, 2021

View reviewed changes

onprem added 10 commits June 17, 2021 00:57

receive: Add support for ingesting exemplars

4a0b8e5

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

receive: Expose exemplars api from Receiver

aa5b09f

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Updated docs

5d766c8

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Fix exemplars API for tsdb returning null always

2d703f3

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Add tests for exemplars in receive

e09ec91

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Fix linting issues

d717022

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Skip querying exemplars if external labels doesn't match

6d67a6f

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Add more details in tsdb.max-exemplars flag description

307cca7

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

exemplars: proxy: return error if matchers only contain external labels

1b15fb2

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

Parse matchers only once for MultiTSDB exemplars queries

88d9eec

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

onprem dismissed stale reviews from bwplotka and kakkoyun via f47c075 June 16, 2021 19:28

onprem force-pushed the ingest-exemplars branch from 7b4cb37 to f47c075 Compare June 16, 2021 19:28

Add documentation and CHANGELOG entry

f47c075

Signed-off-by: Prem Saraswat <prmsrswt@gmail.com>

yeya24 enabled auto-merge (squash) June 17, 2021 02:37

bwplotka approved these changes Jun 17, 2021

View reviewed changes

yeya24 merged commit 13ab756 into thanos-io:main Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

receive: Enable exemplars ingestion and querying #4292

receive: Enable exemplars ingestion and querying #4292

onprem commented Jun 1, 2021 •

edited

Loading

wiardvanrij left a comment

wiardvanrij Jun 1, 2021

onprem Jun 1, 2021 •

edited

Loading

bwplotka Jun 11, 2021

kakkoyun Jun 11, 2021

bwplotka Jun 11, 2021

onprem Jun 16, 2021

yeya24 left a comment

yeya24 left a comment

yeya24 left a comment

kakkoyun left a comment

kakkoyun Jun 11, 2021

bwplotka left a comment

bwplotka Jun 11, 2021

kakkoyun commented Jun 11, 2021

bwplotka left a comment

receive: Enable exemplars ingestion and querying #4292

receive: Enable exemplars ingestion and querying #4292

Conversation

onprem commented Jun 1, 2021 • edited Loading

Changes

Verification

wiardvanrij left a comment

Choose a reason for hiding this comment

wiardvanrij Jun 1, 2021

Choose a reason for hiding this comment

onprem Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

bwplotka Jun 11, 2021

Choose a reason for hiding this comment

kakkoyun Jun 11, 2021

Choose a reason for hiding this comment

bwplotka Jun 11, 2021

Choose a reason for hiding this comment

onprem Jun 16, 2021

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

kakkoyun Jun 11, 2021

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Jun 11, 2021

Choose a reason for hiding this comment

kakkoyun commented Jun 11, 2021

bwplotka left a comment

Choose a reason for hiding this comment

onprem commented Jun 1, 2021 •

edited

Loading

onprem Jun 1, 2021 •

edited

Loading