Tracing: Migrate Google Cloud (Stackdriver) client to OpenTelemetry #4838

matej-g · 2021-11-07T21:22:16Z

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

Changes

This is first in the series of PRs to start migrating our tracing from the OpenTracing packages to OpenTelemetry. This PR in particular moves Google Cloud provider to OpenTelemetry. In details:

Migrate Stackdriver to OpenTelemetry, rename to Google Cloud, while keeping the configuration backwards compatible
Introduce utilities (bridge tracer and sampler), which will be required to build the OpenTelemetry tracer(s) while keeping the current behavior and features (e.g. forcing tracing by HTTP header)
Adjust HTTP middleware
Update documentation

Partially addresses #1972.

Verification

Tests have been updated; the change has been also verified manually by sending traces to Google Cloud.

- Add method to create bridge tracer - Implement a sampler which enables us to force tracing Signed-off-by: Matej Gera <matejgera@gmail.com>

- Rename to Google Cloud, keep it backwards compatible - Refactor and move to OTEL exporter - Adjust factory to use bridge tracer for this provider Signed-off-by: Matej Gera <matejgera@gmail.com>

- to ensure force tracing / populate trace ID header works for the bridge tracer as well Signed-off-by: Matej Gera <matejgera@gmail.com>

Signed-off-by: Matej Gera <matejgera@gmail.com>

yeya24 · 2021-11-13T22:25:36Z

pkg/tracing/google_cloud/google_cloud.go

+	}
+
+	if len(os.Args) > 1 {
+		attr = append(attr, attribute.String("binary_cmd", os.Args[1]))


Why do we need to have binary Infos to attributes? Maybe remove this or at least make this configurable?

Opentelemetry specification says lots of semantic_conventions , we should use it as possible. like process.executable.name. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/process.md#process

Honestly, this was purely to keep the behavior, attribute name etc. the same as it is now, as we wanted to make the change as 'backwards compatible' as possible. But yes, OTEL has more detailed guidance on semantic conventions and I'm definitely planning to follow them as we move the rest of the instrumentation to OTEL. However, if the feeling is that this can be changed already here, I'm happy to follow through.

matej-g · 2021-11-29T11:11:40Z

ping @yeya24 @hanjm if you have interest and capacity to review by any chance, I'd be grateful 🙏

hanjm · 2021-11-30T01:01:18Z

💪 LGTM

hanjm · 2021-12-11T13:05:03Z

ping @yeya24

pkg/tracing/google_cloud/google_cloud.go

yeya24 · 2021-12-12T00:18:06Z

pkg/tracing/google_cloud/google_cloud.go

+
+	resource, err := resource.New(ctx, resource.WithAttributes(collectAttributes(serviceName)...))
+	if err != nil {
+		level.Warn(logger).Log("msg", "detecting resources for tracing provider failed", "err", err)


If there is an error, should we return it or do we need to deal with it in line 67 as well?

If there's an error, we should still receive valid (although possibly empty) resource. I think best we can do is log warning and move on (or we can possibly change level to error). I'll add a comment as well.

yeya24 · 2021-12-20T18:23:21Z

Would like to move this forward 😄

…ver-exporter

Signed-off-by: Matej Gera <matejgera@gmail.com>

matej-g · 2021-12-21T17:03:17Z

Hey @yeya24 thanks for the ping. I updated the PR and also now updated OTEL to version 1.3.0.

However, after further testing, I'm noticing the traces get 'broken' between components 😞. For example when executing query -> receiver, it shows up as two distinct traces for each component - it gets 'split' on the store gRPC operation between client and server; I'm not sure exactly why, whether some changes are required to the middleware as well. I'll need more time to sort it out.

hanjm · 2021-12-22T01:11:23Z

@matej-g are you do inject span context to header in grpc/http client? like this https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L94

- Currently, bridge tracer supports only HTTP headers as a carrier.However, our instrumentation e.g. for gRPC uses metatada.MD as a carrier instead, breaking the propagatin. This fix works around it by 'converting' the carrier to HTTP header. See code docs for details. Signed-off-by: Matej Gera <matejgera@gmail.com>

…ver-exporter

Signed-off-by: Matej Gera <matejgera@gmail.com>

matej-g · 2022-01-28T15:32:10Z

Apologies for the long delay on this one, but I needed to find a bit of time to pin this down.

@hanjm I could not at first understand why the data was not being propagated, as you point out, but in the end I discovered that the OpenTelemetry's BridgeTracer supports only HTTP headers as a carrier when propagating. Since in the gRPC middleware we are using the gRPC metadata as a carrier, the trace injection was always failing due to unsupported carrier. I created a wrapper for the bridge tracer to work around this. I have tested is as well on Google Cloud and now the traces are looking as expected:

cc @yeya24

matej-g · 2022-02-24T08:54:20Z

Gentle ping @hanjm @yeya24, if you'd get a chance to re-review, thank you 🙏

hanjm · 2022-02-24T14:42:18Z

Overall LGTM, Thanks you.

hanjm · 2021-12-11T13:04:41Z

pkg/tracing/google_cloud/google_cloud.go

+	}
+
+	if len(os.Args) > 1 {
+		attr = append(attr, attribute.String("binary_cmd", os.Args[1]))


hanjm · 2022-02-24T14:36:57Z

go.mod

+	go.opentelemetry.io/otel v1.3.0
+	go.opentelemetry.io/otel/bridge/opentracing v1.3.0
+	go.opentelemetry.io/otel/sdk v1.3.0
+	go.opentelemetry.io/otel/trace v1.3.0


1.3.1 has released.

Thanks, I'll update to latest version!

pkg/tracing/client/factory.go

yeya24 · 2022-03-05T21:06:52Z

We should be good to go after fixing @hanjm 's comments 👍

Signed-off-by: Matej Gera <matejgera@gmail.com>

matej-g · 2022-03-17T09:13:22Z

Thanks @hanjm @yeya24, hopefully now this is good to go!

yeya24

LGTM

matej-g added 5 commits November 7, 2021 21:37

Add migration utilities

1671d8d

- Add method to create bridge tracer - Implement a sampler which enables us to force tracing Signed-off-by: Matej Gera <matejgera@gmail.com>

Migrate and rename Stackdriver to Google Cloud

7f296bc

- Rename to Google Cloud, keep it backwards compatible - Refactor and move to OTEL exporter - Adjust factory to use bridge tracer for this provider Signed-off-by: Matej Gera <matejgera@gmail.com>

Adjust HTTP middleware

6e30e79

- to ensure force tracing / populate trace ID header works for the bridge tracer as well Signed-off-by: Matej Gera <matejgera@gmail.com>

Update documentation

fa5d04f

Signed-off-by: Matej Gera <matejgera@gmail.com>

Copyright file headers

1c56643

Signed-off-by: Matej Gera <matejgera@gmail.com>

matej-g mentioned this pull request Nov 7, 2021

Move tracing to OpenTelemetry #1972

Open

Fix doc formatting

4a9d838

Signed-off-by: Matej Gera <matejgera@gmail.com>

yeya24 reviewed Nov 13, 2021

View reviewed changes

yeya24 reviewed Dec 12, 2021

View reviewed changes

matej-g added 2 commits December 21, 2021 15:56

Merge remote-tracking branch 'origin/main' into otel-migrate-stackdri…

8f7b63b

…ver-exporter

Upgrade OTEL to 1.3.0

99b2d44

Signed-off-by: Matej Gera <matejgera@gmail.com>

pull-request-size bot added the size/XL label Dec 21, 2021

matej-g added 3 commits January 28, 2022 11:58

Merge remote-tracking branch 'origin/main' into otel-migrate-stackdri…

0e33409

…ver-exporter

Remove leftover go.mod replace

f754549

Signed-off-by: Matej Gera <matejgera@gmail.com>

hanjm previously approved these changes Feb 24, 2022

View reviewed changes

Update version; fix constants naming

b8346a3

Signed-off-by: Matej Gera <matejgera@gmail.com>

matej-g dismissed hanjm’s stale review via b8346a3 March 17, 2022 09:02

matej-g requested review from yeya24 and hanjm March 17, 2022 09:13

yeya24 enabled auto-merge (squash) March 17, 2022 20:47

yeya24 approved these changes Mar 17, 2022

View reviewed changes

yeya24 merged commit 72d63e3 into thanos-io:main Mar 17, 2022

matej-g mentioned this pull request Mar 18, 2022

Tracing: Remove forgotten debug line #5245

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracing: Migrate Google Cloud (Stackdriver) client to OpenTelemetry #4838

Tracing: Migrate Google Cloud (Stackdriver) client to OpenTelemetry #4838

matej-g commented Nov 7, 2021 •

edited

yeya24 Nov 13, 2021

hanjm Nov 14, 2021

matej-g Nov 16, 2021 •

edited

hanjm Dec 11, 2021

matej-g commented Nov 29, 2021

hanjm commented Nov 30, 2021

hanjm commented Dec 11, 2021

yeya24 Dec 12, 2021

matej-g Dec 21, 2021

yeya24 commented Dec 20, 2021

matej-g commented Dec 21, 2021

hanjm commented Dec 22, 2021

matej-g commented Jan 28, 2022 •

edited

matej-g commented Feb 24, 2022

hanjm commented Feb 24, 2022

hanjm Dec 11, 2021

hanjm Feb 24, 2022

matej-g Mar 14, 2022

yeya24 commented Mar 5, 2022

matej-g commented Mar 17, 2022

yeya24 left a comment

Tracing: Migrate Google Cloud (Stackdriver) client to OpenTelemetry #4838

Tracing: Migrate Google Cloud (Stackdriver) client to OpenTelemetry #4838

Conversation

matej-g commented Nov 7, 2021 • edited

Changes

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g Nov 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g commented Nov 29, 2021

hanjm commented Nov 30, 2021

hanjm commented Dec 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 commented Dec 20, 2021

matej-g commented Dec 21, 2021

hanjm commented Dec 22, 2021

matej-g commented Jan 28, 2022 • edited

matej-g commented Feb 24, 2022

hanjm commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 commented Mar 5, 2022

matej-g commented Mar 17, 2022

yeya24 left a comment

Choose a reason for hiding this comment

matej-g commented Nov 7, 2021 •

edited

matej-g Nov 16, 2021 •

edited

matej-g commented Jan 28, 2022 •

edited