Add OTel tracing bucket #61

kakkoyun · 2023-07-06T10:05:41Z

Refactor client.NewBucket. This changes the behaviour of client.NewBucket. Now it returns, uninstrumented and untraced bucket.
Add OpenTelemetry TracingBucket.

Now you can combine objstore.WrapWithMetrics and tracing/{opentelemetry,opentracing}.WrapWithTraces to have old behavior.

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

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

Changes

Add OTel Tracing bucket
Introduce tracing package
Refactor client.NewBucket
Introduce client.NewInstrumentedBucket
Add examples

Verification

brancz

I'm happy with this. Are you going to take care of fixing Thanos to make sure it doesn't loose instrumentation?

client/factory.go

README.md

kakkoyun · 2023-07-06T11:17:56Z

I'm happy with this. Are you going to take care of fixing Thanos to make sure it doesn't lose instrumentation?

I'll make sure to do that. Maybe we can use opentracing tracing for the instrumented bucket? WDYT?

brancz · 2023-07-06T11:53:29Z

I think just replacing the spots where Thanos just called NewBucket, after this merges replace that with NewTracingBucket(NewInstrumentedBucket(NewBucket(...))). That would be sufficient.

kakkoyun · 2023-07-06T12:10:32Z

I think just replacing the spots where Thanos just called NewBucket, after this merges replace that with NewTracingBucket(NewInstrumentedBucket(NewBucket(...))). That would be sufficient.

thanos-io/thanos#6507

bwplotka

Nice! Some comments, otherwise LGTM 👍🏽

tracing/tracing.go

client/factory.go

bwplotka · 2023-07-06T13:23:20Z

client/factory.go

+	return objstore.NewPrefixedBucket(bucket, bucketConf.Prefix), nil
+}
+
+// NewInstrumentedBucket initializes and returns new object storage clients.


This is not true - it only wraps with metrics. Should we rather remove NewInstrumentedBucket and keep objstore.BucketWithMetrics?

👍 Fine by me, less is more :)

Not addressed.

bwplotka · 2023-07-06T13:26:09Z

client/factory.go

+
+// NewInstrumentedBucket initializes and returns new object storage clients.
+func NewInstrumentedBucket(reg prometheus.Registerer, bkt objstore.Bucket) objstore.InstrumentedBucket {
+	return objstore.BucketWithMetrics(bkt.Name(), bkt, reg)


Renamed this:

Suggested change

return objstore.BucketWithMetrics(bkt.Name(), bkt, reg)

objstore.InstrumentedBucket(bkt.Name(), bkt, reg)

so we have "InstrumentedBucket", "otel.TracingBucket", "opentracing.TracingBucket"? 🤔

You can see it in action thanos-io/thanos#6507

Do you have any other suggestion?

What do you mean?

Anyway, BucketWithMetrics is now WrapWithMetrics not InstrumentedBucket as I suggested. You changed tracing one with TracedBucket. It's improved, but the problem is still that it's a little bit inconsistent with TracedBucket naming. Either let's call this one InstrumentedBucket or if you want to keep WrapWithMetrics, we could changed TracedBucket to WrapWithTracing? Small nit, just let be clear with what suggestion you agree and which not (:

client/testconf/fake-gcs.conf.yml

tracing/opentelemetry/opentelemetry.go

Refactor client.NewBucket Add client.NewInstrumentedBucket Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

brancz

lgtm

client/factory.go

kakkoyun · 2023-07-11T14:51:42Z

Ping @bwplotka, is it ok to merge now?

bwplotka

Thanks! Still some unresolved comments, otherwise good to go!

client/factory.go

bwplotka · 2023-07-12T12:17:28Z

client/factory.go

+	return objstore.NewPrefixedBucket(bucket, bucketConf.Prefix), nil
+}
+
+// NewInstrumentedBucket initializes and returns new object storage clients.


Not addressed.

bwplotka · 2023-07-12T12:18:22Z

client/factory.go

+
+// NewInstrumentedBucket initializes and returns new object storage clients.
+func NewInstrumentedBucket(reg prometheus.Registerer, bkt objstore.Bucket) objstore.InstrumentedBucket {
+	return objstore.BucketWithMetrics(bkt.Name(), bkt, reg)


What do you mean?

Anyway, BucketWithMetrics is now WrapWithMetrics not InstrumentedBucket as I suggested. You changed tracing one with TracedBucket. It's improved, but the problem is still that it's a little bit inconsistent with TracedBucket naming. Either let's call this one InstrumentedBucket or if you want to keep WrapWithMetrics, we could changed TracedBucket to WrapWithTracing? Small nit, just let be clear with what suggestion you agree and which not (:

client/testconf/fake-gcs.conf.yml

bwplotka · 2023-07-12T12:19:53Z

tracing/opentelemetry/opentelemetry.go

+// Copyright (c) The Thanos Authors.
+// Licensed under the Apache License 2.0.
+
+package opentelemetry


Can be simplified to otel if we want:

Suggested change

package opentelemetry

package otel

Let's keep naming consistent with already existing parts (opentracing and opentelemetry).

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

kakkoyun force-pushed the add_otel_tracing_bucket branch from 43dfc20 to 90f9ddf Compare July 6, 2023 10:07

kakkoyun requested review from yeya24, brancz and bwplotka July 6, 2023 10:07

kakkoyun mentioned this pull request Jul 6, 2023

debuginfo: Add traces parca-dev/parca#3396

Merged

brancz reviewed Jul 6, 2023

View reviewed changes

client/factory.go Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

kakkoyun force-pushed the add_otel_tracing_bucket branch 2 times, most recently from 860ea66 to e16e78f Compare July 6, 2023 11:24

kakkoyun force-pushed the add_otel_tracing_bucket branch from e16e78f to b3eeaf3 Compare July 6, 2023 11:58

kakkoyun mentioned this pull request Jul 6, 2023

Upgrade objstore thanos-io/thanos#6507

Merged

kakkoyun mentioned this pull request Jul 6, 2023

Upgrade Thanos objstore polarsignals/frostdb#469

Merged

bwplotka reviewed Jul 6, 2023

View reviewed changes

Add OTel tracing bucket

7b1439a

Refactor client.NewBucket Add client.NewInstrumentedBucket Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

kakkoyun force-pushed the add_otel_tracing_bucket branch from b3eeaf3 to 7b1439a Compare July 6, 2023 14:04

kakkoyun requested a review from bwplotka July 6, 2023 14:04

Update changelog

22a1c74

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

brancz approved these changes Jul 6, 2023

View reviewed changes

client/factory.go Outdated Show resolved Hide resolved

Merge branch 'main' into add_otel_tracing_bucket

64d5e43

bwplotka approved these changes Jul 12, 2023

View reviewed changes

Address review comments

a3234c4

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

kakkoyun requested review from bwplotka and brancz July 12, 2023 15:45

bwplotka approved these changes Jul 13, 2023

View reviewed changes

bwplotka merged commit eb01c83 into main Jul 13, 2023
7 checks passed

kakkoyun deleted the add_otel_tracing_bucket branch July 13, 2023 07:43

shantanualsi mentioned this pull request Aug 28, 2023

Upgrade Thanos objstore, dskit and other modules grafana/loki#10366

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OTel tracing bucket #61

Add OTel tracing bucket #61

kakkoyun commented Jul 6, 2023 •

edited

Loading

brancz left a comment

kakkoyun commented Jul 6, 2023

brancz commented Jul 6, 2023

kakkoyun commented Jul 6, 2023

bwplotka left a comment

bwplotka Jul 6, 2023

kakkoyun Jul 6, 2023

bwplotka Jul 12, 2023

bwplotka Jul 6, 2023

kakkoyun Jul 6, 2023

bwplotka Jul 12, 2023 •

edited

Loading

brancz left a comment

kakkoyun commented Jul 11, 2023

bwplotka left a comment

bwplotka Jul 12, 2023

bwplotka Jul 12, 2023 •

edited

Loading

bwplotka Jul 12, 2023

kakkoyun Jul 12, 2023

	return objstore.BucketWithMetrics(bkt.Name(), bkt, reg)
	objstore.InstrumentedBucket(bkt.Name(), bkt, reg)

Add OTel tracing bucket #61

Add OTel tracing bucket #61

Conversation

kakkoyun commented Jul 6, 2023 • edited Loading

Changes

Verification

brancz left a comment

Choose a reason for hiding this comment

kakkoyun commented Jul 6, 2023

brancz commented Jul 6, 2023

kakkoyun commented Jul 6, 2023

bwplotka left a 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

bwplotka Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

brancz left a comment

Choose a reason for hiding this comment

kakkoyun commented Jul 11, 2023

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakkoyun commented Jul 6, 2023 •

edited

Loading

bwplotka Jul 12, 2023 •

edited

Loading

bwplotka Jul 12, 2023 •

edited

Loading