Implement federated exemplar API #3846

goku321 · 2021-02-26T18:29:13Z

Signed-off-by: Deepak [email protected]

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

Fixes: #3435

This PR follows the PR #3350 and comment #3345 to implement the Prometheus exemplar API

Changes

Add federated exemplar api to query and sidecar.

Verification

testing in progress

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

goku321 · 2021-02-26T18:45:55Z

@bwplotka @yeya24 I've tried to complete this as much as possible but there are still few gaps. A review would really be helpful to find out all the issues this pr has.

Changes I'm not sure about:

Also, I'll make sure to add required tests.

Thank you for your support and guidance.

yeya24 · 2021-02-26T18:57:25Z

Thanks for the great work! Looks perfect at the first glance. I will do some detailed review next week.

Have you tested this manually with the WIP upstream branch? prometheus/prometheus#6635

goku321 · 2021-02-26T19:20:41Z

Have you tested this manually with the WIP upstream branch? prometheus/prometheus#6635

Not yet 😅 . Will be doing it very soon.

yeya24 · 2021-02-27T06:36:35Z

I fixed some existing problems in this pr and it is working now! Will add some detailed comments tomorrow. Feel free to check my commit here.

If you want to experiment with this, feel free to try this tns app https://github.com/yeya24/tns/tree/thanos/production/docker-compose. I added Thanos sidecar and querier there.

It works smoothly. Thanks @goku321!

yeya24 · 2021-03-01T01:48:42Z

pkg/api/query/v1.go

@@ -152,6 +158,8 @@ func (qapi *QueryAPI) Register(r *route.Router, tracer opentracing.Tracer, logge
 	r.Get("/rules", instr("rules", NewRulesHandler(qapi.ruleGroups, qapi.enableRulePartialResponse)))

 	r.Get("/metadata", instr("metadata", NewMetricMetadataHandler(qapi.metadatas, qapi.enableMetricMetadataPartialResponse)))
+
+	r.Get("/exemplars", instr("exemplars", NewExemplarsHandler(qapi.exemplars, qapi.enableExemplarPartialResponse)))


Let's be consistent with the upstream PR https://github.com/prometheus/prometheus/pull/6635/files#diff-9bcb876da5d17ef1046ab5ea531bbaaf64d753a7beaa2cb11a3fe6dd94e16563R349.

The endpoint is query_exemplars

yeya24 · 2021-03-01T01:49:27Z

pkg/api/query/v1.go

+	}
+
+	return func(r *http.Request) (interface{}, []error, *api.ApiError) {
+		typeParam := r.URL.Query().Get("type")


What's this param for?
Let's be consistent with the upstream pr.
The params we want to have are:

- `query=<string>`: Prometheus expression query string. - `start=<rfc3339 | unix_timestamp>`: Start timestamp. - `end=<rfc3339 | unix_timestamp>`: End timestamp.

yeya24 · 2021-03-01T01:50:27Z

pkg/exemplars/exemplarspb/rpc.proto

+    string instance = 1 [(gogoproto.jsontag) = "instance"];
+    string job = 2 [(gogoproto.jsontag) = "job"];
+    string service = 3 [(gogoproto.jsontag) = "service"];
+}


This seems not used. Let's remove it.

yeya24 · 2021-03-01T01:50:37Z

pkg/exemplars/exemplarspb/rpc.proto

+    double value = 2 [(gogoproto.jsontag) = "value"];
+    google.protobuf.Timestamp ts = 3 [(gogoproto.jsontag) = "timestamp", (gogoproto.stdtime) = true, (gogoproto.nullable) = false ];
+    bool hasts = 4 [(gogoproto.jsontag) = "hasTimestamp"];
+}


Please add a new line.

yeya24 · 2021-03-01T01:51:48Z

pkg/exemplars/exemplarspb/rpc.proto

+message Exemplar {
+    ZLabelSet labels = 1 [(gogoproto.jsontag) = "labels", (gogoproto.nullable) = false];
+    double value = 2 [(gogoproto.jsontag) = "value"];
+    google.protobuf.Timestamp ts = 3 [(gogoproto.jsontag) = "timestamp", (gogoproto.stdtime) = true, (gogoproto.nullable) = false ];


Let's use int64 type here to be consistent with the TS type in prometheus https://github.com/prometheus/prometheus/pull/6635/files#diff-fba4b55f88ed50da9d6d2624ba745f9347cdc87a80bac431a090aee67a49c02fR23

yeya24 · 2021-03-01T01:52:17Z

pkg/promclient/promclient.go

+// NOTE: This method is tested in pkg/store/prometheus_test.go against Prometheus.
+func (c *Client) ExemplarsInGRPC(ctx context.Context, base *url.URL, typeExemplars string) ([]*exemplarspb.ExemplarData, error) {
+	u := *base
+	u.Path = path.Join(u.Path, "/api/v1/exemplars")


Ditto. Let's change the endpoint.

yeya24 · 2021-03-01T01:52:55Z

pkg/promclient/promclient.go

+	// Prometheus does not support PartialResponseStrategy, and probably would never do. Make it Abort by default.
+	for _, g := range m.Data {
+		g.PartialResponseStrategy = storepb.PartialResponseStrategy_ABORT
+	}


I guess we don't need to keep this PartialResponseStrategy field in the proto. Let's remove it for now.

yeya24 · 2021-03-01T01:53:49Z

pkg/exemplars/exemplarspb/rpc.proto

+// message Label {
+//     string name = 1 [(gogoproto.jsontag) = "name"];
+//     string value = 2 [(gogoproto.jsontag) = "value"];
+// }


Let's remove this

yeya24 · 2021-03-01T01:57:54Z

pkg/exemplars/exemplars.go

+
+	warnings []error
+	data     []*exemplarspb.ExemplarData
+}


Let's add two additional methods for it yeya24@1b6abfc#diff-d09c4014e056aee5c67a76aca788de273aecb440edefc8a3b040bc77e27f2c38R39-R55, otherwise it will panic.

yeya24 · 2021-03-03T15:40:19Z

@goku321 Let me know your availability. If it is acceptable I can open a pr to your branch as I have a working version already yeya24@1b6abfc.

goku321 · 2021-03-03T16:59:55Z

Hi @yeya24 , I was a bit caught up, thanks for your patience.

By all means. Please go ahead with the pr. I am glad to have your help on this. I really appreciate your time and effort on fixing this pr.

If it is acceptable I can open a pr to your branch as I have a working version already

yeya24 · 2021-03-03T17:06:06Z

It is not urgent. Anyway, we need to wait for the upstream Prometheus pr first XD

goku321 · 2021-03-03T18:01:40Z

I fixed some existing problems in this pr and it is working now! Will add some detailed comments tomorrow. Feel free to check my commit here.

If you want to experiment with this, feel free to try this tns app https://github.com/yeya24/tns/tree/thanos/production/docker-compose. I added Thanos sidecar and querier there.

It works smoothly. Thanks @goku321!

Sure, I'll try out the tns app to get a better understanding.

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

squat · 2021-03-12T10:48:25Z

pkg/exemplars/proxy.go

+)
+
+// Proxy implements exemplarspb.Exemplars gRPC that fanouts requests to
+// given exemplarspb.Exemplars and de-duplication on the way.


Let's update this comment. Currently, Proxy itself does not deduplicate. Instead, the it seems to be a responsibility of the caller of proxy.Exemplars to deduplication. Alternatively, we can make it so that the Proxy needs to deduplication.

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

pkg/exemplars/exemplarspb/rpc.proto

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

test/e2e/e2ethanos/services.go

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

test/e2e/metadata_api_test.go

test/e2e/e2ethanos/services.go

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

test/e2e/e2ethanos/services.go

pkg/exemplars/exemplars_test.go

pkg/exemplars/exemplarspb/custom.go

yeya24

@goku321 Thanks for the great work! This is super close now, only some nits and testcases are left.

pkg/api/query/v1.go

pkg/exemplars/exemplarspb/custom.go

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

yeya24

Perfect work @goku321. One last thing, can you please update the changelog?
I think this pr is ready to merge. But it would be better to have another 👀 for a final look.

Note: There is no E2E API test for this feature now as we don't have an app instrumented with exemplars. We can move E2E part to another pr.

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

squat

Looks clean to me!

goku321 added 3 commits February 25, 2021 00:09

Add exemplars API

3ffc597

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

Sync with main branch

45a4bac

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

Fix errors in e2e tests

47ef813

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

yeya24 reviewed Mar 1, 2021

View reviewed changes

Sync with main branch

ab00a68

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

squat reviewed Mar 12, 2021

View reviewed changes

goku321 added 2 commits March 18, 2021 21:58

Fix exemplars proto file and query endpoint

74bdd3e

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

Sync with main branch

7b49b71

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

yeya24 reviewed Mar 18, 2021

View reviewed changes

pkg/exemplars/exemplarspb/rpc.proto Outdated Show resolved Hide resolved

goku321 added 2 commits March 19, 2021 23:34

Remove hasts field from Exemplar message

8fdd90c

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

Update comment on exemplars.Proxy struct

d7dffcc

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

goku321 force-pushed the exemplars-api branch from 78274bd to d7dffcc Compare March 19, 2021 18:12

yeya24 reviewed Mar 19, 2021

View reviewed changes

test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved

goku321 added 2 commits March 20, 2021 00:22

Fix e2e test

e510368

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

Format exemplars.go with gofmt

fa8ced6

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

yeya24 reviewed Mar 20, 2021

View reviewed changes

test/e2e/metadata_api_test.go Show resolved Hide resolved

yeya24 reviewed Mar 20, 2021

View reviewed changes

test/e2e/e2ethanos/services.go Outdated Show resolved Hide resolved

Revert to old prometheus image in e2e test

a7b0ecd

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

yeya24 reviewed Mar 20, 2021

View reviewed changes

yeya24 approved these changes Mar 20, 2021

View reviewed changes

yeya24 requested changes Mar 20, 2021

View reviewed changes

pkg/api/query/v1.go Show resolved Hide resolved

yeya24 requested changes Mar 20, 2021

View reviewed changes

pkg/exemplars/exemplarspb/custom.go Outdated Show resolved Hide resolved

goku321 added 2 commits March 22, 2021 22:21

Add more tests for exemplars

422b24f

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

Run go fmt

84975b1

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

goku321 requested a review from yeya24 March 22, 2021 16:56

yeya24 approved these changes Mar 22, 2021

View reviewed changes

goku321 added 2 commits March 22, 2021 23:08

Update changelog

cc7cab4

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

Sync with main branch

6159136

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

goku321 requested a review from squat March 22, 2021 17:43

squat approved these changes Mar 22, 2021

View reviewed changes

yeya24 merged commit 468d0d6 into thanos-io:main Mar 22, 2021

ioaiaaii mentioned this pull request Jul 7, 2021

Thanos Sidecar and Exemplars #4423

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement federated exemplar API #3846

Implement federated exemplar API #3846

goku321 commented Feb 26, 2021 •

edited

Loading

goku321 commented Feb 26, 2021 •

edited

Loading

yeya24 commented Feb 26, 2021

goku321 commented Feb 26, 2021

yeya24 commented Feb 27, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 Mar 1, 2021

yeya24 commented Mar 3, 2021

goku321 commented Mar 3, 2021

yeya24 commented Mar 3, 2021

goku321 commented Mar 3, 2021

squat Mar 12, 2021

yeya24 left a comment

yeya24 left a comment •

edited

Loading

squat left a comment

Implement federated exemplar API #3846

Implement federated exemplar API #3846

Conversation

goku321 commented Feb 26, 2021 • edited Loading

Changes

Verification

goku321 commented Feb 26, 2021 • edited Loading

yeya24 commented Feb 26, 2021

goku321 commented Feb 26, 2021

yeya24 commented Feb 27, 2021

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

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

Choose a reason for hiding this comment

yeya24 commented Mar 3, 2021

goku321 commented Mar 3, 2021

yeya24 commented Mar 3, 2021

goku321 commented Mar 3, 2021

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

yeya24 left a comment • edited Loading

Choose a reason for hiding this comment

squat left a comment

Choose a reason for hiding this comment

goku321 commented Feb 26, 2021 •

edited

Loading

goku321 commented Feb 26, 2021 •

edited

Loading

yeya24 left a comment •

edited

Loading