[FEATURE] adding otlp endpoint #7996

nicolastakashi · 2024-12-16T21:45:30Z

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

Changes

Verification

pkg/receive/handler_otlp.go

+
+	tenant, err := tenancy.GetTenantFromHTTP(r, h.options.TenantHeader, h.options.DefaultTenantID, h.options.TenantField)
+	if err != nil {
+		level.Error(h.logger).Log("msg", "error getting tenant from HTTP", "err", err)


pkg/receive/handler_otlp.go

+
+	defer writeGate.Done()
+	if err != nil {
+		level.Error(tLogger).Log("err", err, "msg", "internal server error")


pkg/receive/handler_otlp.go

+		case errBadReplica:
+			responseStatusCode = http.StatusBadRequest
+		default:
+			level.Error(tLogger).Log("err", err, "msg", "internal server error")


pkg/receive/handler_otlp.go

saswatamcode

Thanks for the efforts here! Looks good already. 🙂

I did recommend copying the translator struct over to use thanos protos natively instead of doing a full conversion, but it could be a maintenance burden.

Let's wait for other maintainers to have a look as well.

Couple of comments

pkg/receive/handler_otlp.go

pkg/receive/otlptranslator/metrics_to_prw.go

matej-g

Thanks for starting this work @nicolastakashi ❤️, I'm adding a couple more suggestions

pkg/receive/handler_otlp.go

matej-g · 2024-12-19T09:43:49Z

pkg/receive/otlptranslator/context.go

+// Copyright (c) The Thanos Authors.
+// Licensed under the Apache License 2.0.
+
+// Provenance-includes-location: https://github.com/prometheus/prometheus/blob/318d6bc4bfe2d81af88697940740a378b503335a/storage/remote/otlptranslator/prometheusremotewrite/context.go#L18


Just for my understanding, where are these provenance directives from? What does it mean? That the code is copied from Prometheus repo?

I copy the code from prometheus and prometheus copied from OTel 😅

test/e2e/e2ethanos/services.go

saswatamcode

Thanks! ❤️
This already looks quite good. @nicolastakashi we can probably mark it ready for review, and get others to have a look as well?

cmd/thanos/receive.go

pkg/receive/handler.go

nicolastakashi · 2024-12-20T13:59:14Z

pkg/receive/handler.go

@@ -275,6 +277,18 @@ func NewHandler(logger log.Logger, o *Options) *Handler {
 		),
 	)

+	h.router.Post(


@saswatamcode just FYI use this endpoint naming will require users to use the metrics_endpoint instead of endpoint property, maybe worth document it or writing a blog post later about Thanos and OTLP wdyt?

You mean on otel-collector? I think that would be fine, we can document

pkg/receive/otlptranslator/histograms.go

pedro-stanaka

LGTM - Thanks for working on this, this makes the project much more relevant in the whole Observability space.

That said, I am not a huge fan of the whole copying files over, but I am okay with it in name of optimization. The code for the handler looks good, I just wanted to see more core reuse in general (but not a problem to solve on this PR). We already have duplication because of Capn Proto ingestion and now also because of OTLP.

Once again, amazing work Nicolas! 💪

matej-g

Thanks again @nicolastakashi 🙌 , this is looking better with every pass.

I'm adding couple more suggestions to improve the parameters, I hope that's OK and doesn't add to much effort.

Two more things:

I think it would be good to have a separate documentation section to explain the OTLP endpoint and mainly to explain the behavior of resource attributes and how the parameters affect the ingested metrics / labels.
With regards to the translator code, I'm personally also leaning towards just importing instead of copying and all of the code, and then seeing if we need to copy the code over for further optimization. But it's not a strong position, if people see value in including this optimization from the get-go, I'm not opposed.

cmd/thanos/receive.go

matej-g

Couple more additions to the previous changes related to the parameters, otherwise looks good on my side!

cmd/thanos/receive.go

pkg/receive/handler_otlp.go

saswatamcode

Apart from pending comments + conflict, I think it looks good now!

I think there few things to note/remember as TODOs,

otlptranslator duplication to use zlabels. I think having it here works, but would cause some maintenance burden overall, we can choose to remove later if that were to be the case (or even upstream zlabels)
We are also duplicating a lot of handler code. I had to kind of do the same initially in Support remote write 2.0 on receive #8033 for remote write 2.0. But I think once this is merged, I'll update my PR to make things around handling/replication more generic for rw1, 2.0, otlp and capnproto!
Having a blog post + more docs about this feature once it lands, maybe even with some reference arch :)

GiedriusS · 2025-01-08T15:11:56Z

pkg/receive/handler_otlp.go

+		}
+	}
+
+	req, err := remote.DecodeOTLPWriteRequest(r)


This will be called lots of times - https://github.com/prometheus/prometheus/blob/1ea9b72997a116b4a7f7a22635c16d71dc5ab440/storage/remote/codec.go#L873 maybe this function could accept a []byte so that we could pool this buffer?

Yes, makes sense to me.
Do you think we should change upstream before we merge it?
Do you prefer copy this function and maintain it on thanos also?

Signed-off-by: Nicolas Takashi <[email protected]>

Co-authored-by: Saswata Mukherjee <[email protected]> Signed-off-by: Nicolas Takashi <[email protected]>

Signed-off-by: Nicolas Takashi <[email protected]>

Co-authored-by: Matej Gera <[email protected]> Signed-off-by: Nicolas Takashi <[email protected]>

Signed-off-by: Nicolas Takashi <[email protected]>

pull-request-size bot added the size/L label Dec 16, 2024

github-advanced-security bot found potential problems Dec 16, 2024

View reviewed changes

saswatamcode reviewed Dec 18, 2024

View reviewed changes

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

saswatamcode reviewed Dec 18, 2024

View reviewed changes

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

nicolastakashi requested a review from saswatamcode December 18, 2024 21:14

pull-request-size bot added size/XXL and removed size/L labels Dec 18, 2024

saswatamcode reviewed Dec 19, 2024

View reviewed changes

pkg/receive/handler_otlp.go Show resolved Hide resolved

pkg/receive/otlptranslator/metrics_to_prw.go Outdated Show resolved Hide resolved

matej-g reviewed Dec 19, 2024

View reviewed changes

nicolastakashi requested review from saswatamcode and matej-g December 20, 2024 10:22

saswatamcode reviewed Dec 20, 2024

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

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

nicolastakashi commented Dec 20, 2024

View reviewed changes

nicolastakashi marked this pull request as ready for review December 20, 2024 14:06

pedro-stanaka reviewed Dec 20, 2024

View reviewed changes

pkg/receive/otlptranslator/histograms.go Show resolved Hide resolved

pedro-stanaka approved these changes Dec 20, 2024

View reviewed changes

matej-g reviewed Dec 23, 2024

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

cmd/thanos/receive.go Outdated Show resolved Hide resolved

nicolastakashi requested a review from matej-g January 6, 2025 16:06

matej-g reviewed Jan 7, 2025

View reviewed changes

cmd/thanos/receive.go Outdated Show resolved Hide resolved

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

saswatamcode previously approved these changes Jan 8, 2025

View reviewed changes

nicolastakashi dismissed saswatamcode’s stale review via b7a038e January 8, 2025 10:56

GiedriusS reviewed Jan 8, 2025

View reviewed changes

jmichalek132 mentioned this pull request Jan 9, 2025

Support for OTLP metrics #6932

Open

nicolastakashi force-pushed the chore/adding-otlp-endpoint branch from 5ad37d2 to 14179f7 Compare January 9, 2025 21:00

nicolastakashi added 6 commits January 9, 2025 21:01

[FEATURE] adding otlp endpoint

6bbcb43

Signed-off-by: Nicolas Takashi <[email protected]>

[FEATURE] adding otlp endpoint

eeb6db9

Signed-off-by: Nicolas Takashi <[email protected]>

[FEATURE] adding otlp endpoint

0011737

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] e2e tests for otlp receiver

1728aa2

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] adding otlp flags

46f5e63

Signed-off-by: Nicolas Takashi <[email protected]>

[DOC] updating docs

3ed655a

Signed-off-by: Nicolas Takashi <[email protected]>

nicolastakashi and others added 21 commits January 9, 2025 21:02

[CHORE] copying otlptranslator

882a46b

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] copying otlptranslator tests

e68e4a9

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] copying otlptranslator tests

f8e9ee2

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] copying otlptranslator tests

3c2f5ec

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] lint issues

94df40f

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] lint issues

170464c

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] lint issues

9feaf8a

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] using multi errors

e1a91cc

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] span naming convention

af2f7b1

Signed-off-by: Nicolas Takashi <[email protected]>

[TEST] adding handler otlp unit test

fe9ceaf

Signed-off-by: Nicolas Takashi <[email protected]>

[TEST] upgrade collector version

f4b7c1a

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] golang lint

57ffaba

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] adding allow size bytes limit gate

866ff14

Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] unit test otlp endpoint

d8e8f12

Signed-off-by: Nicolas Takashi <[email protected]>

Apply suggestions from code review

49c0ff3

Co-authored-by: Saswata Mukherjee <[email protected]> Signed-off-by: Nicolas Takashi <[email protected]>

[FIX] unit test otlp endpoint

da67c11

Signed-off-by: Nicolas Takashi <[email protected]>

[DOC] updating docs

2423347

Signed-off-by: Nicolas Takashi <[email protected]>

[CHORE] applying pr comments

a76888f

Signed-off-by: Nicolas Takashi <[email protected]>

Update pkg/receive/handler_otlp.go

4d3d699

Co-authored-by: Matej Gera <[email protected]> Signed-off-by: Nicolas Takashi <[email protected]>

Update cmd/thanos/receive.go

f1b4659

Co-authored-by: Matej Gera <[email protected]> Signed-off-by: Nicolas Takashi <[email protected]>

[DOCS] updating

eda86c3

Signed-off-by: Nicolas Takashi <[email protected]>

nicolastakashi force-pushed the chore/adding-otlp-endpoint branch from 14179f7 to eda86c3 Compare January 9, 2025 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] adding otlp endpoint #7996

[FEATURE] adding otlp endpoint #7996

nicolastakashi commented Dec 16, 2024

saswatamcode left a comment

matej-g left a comment

matej-g Dec 19, 2024

nicolastakashi Dec 19, 2024

saswatamcode left a comment

nicolastakashi Dec 20, 2024

saswatamcode Jan 8, 2025

pedro-stanaka left a comment

matej-g left a comment

matej-g left a comment

saswatamcode left a comment •

edited

Loading

GiedriusS Jan 8, 2025

nicolastakashi Jan 9, 2025

[FEATURE] adding otlp endpoint #7996

Are you sure you want to change the base?

[FEATURE] adding otlp endpoint #7996

Conversation

nicolastakashi commented Dec 16, 2024

Changes

Verification

saswatamcode left a comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

matej-g Dec 19, 2024

Choose a reason for hiding this comment

nicolastakashi Dec 19, 2024

Choose a reason for hiding this comment

saswatamcode left a comment

Choose a reason for hiding this comment

nicolastakashi Dec 20, 2024

Choose a reason for hiding this comment

saswatamcode Jan 8, 2025

Choose a reason for hiding this comment

pedro-stanaka left a comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

saswatamcode left a comment • edited Loading

Choose a reason for hiding this comment

GiedriusS Jan 8, 2025

Choose a reason for hiding this comment

nicolastakashi Jan 9, 2025

Choose a reason for hiding this comment

saswatamcode left a comment •

edited

Loading