Add tracing with opentelemetry go sarama wrapper #179

melchiormoulin · 2022-05-24T16:19:58Z

Adding tracing with opentelemetry sarama wrapper

Why ?
Add more telemetry data for debugging a particular message, more convenient than logs.

How ?
By putting this env variable ACTIVATE_TRACING=true

Here is just an example of a console tracing.
If this PR is accepted i can handle a more generic approach than the console exporter.

scholzj

Stupid question ... what does console tracing mean? Always when i used tracing in the past I was sending the traces for example to Jaeger.

melchiormoulin · 2022-05-24T16:40:56Z

the end goal usually is to use jaeger yes.
What i meant by console tracing is to have the trace in the log like here https://opentelemetry.io/docs/instrumentation/go/getting-started/#creating-a-console-exporter

scholzj · 2022-05-24T22:08:52Z

Thanks for the explanation. I'm not sure how useful really the tracing into the file is on Kubernetes because it brings issues with storage etc. with it. So I wonder if we should target a full support to be able to ship the logs to Jaeger.

@ppatierno WDYT?

melchiormoulin · 2022-05-25T07:39:08Z

i making the change to target JAEGER if you are ok to integrate the tracing feature

devguyio · 2022-05-25T07:53:26Z

Adding tracing is definitely valuable, however I think it would be great if the tracing backend is configurable , maybe a ConfigMap? Here's how it's done in Knative to support Zipkin and Jaeger:
https://knative.dev/docs/eventing/accessing-traces/#configuring-tracing

ppatierno · 2022-05-25T07:54:12Z

I don't think that tracing to the console is something really useful, also bringing problems related to storage already mentioned by Jakub.
I would be more for tracing with systems like Jaeger. Anyway, if user is not using Jaeger but something different like Zipkin or Prometheus (as mentioned in the link you provided), how this is going to work? Should we make it configurable and pluggable?

devguyio · 2022-05-25T07:57:01Z

I don't think that tracing to the console is something really useful, also bringing problems related to storage already mentioned by Jakub. I would be more for tracing with systems like Jaeger. Anyway, if user is not using Jaeger but something different like Zipkin or Prometheus (as mentioned in the link you provided), how this is going to work? Should we make it configurable and pluggable?

Jaeger provides a zipkin compatible API, so typically it's a good strategy (as a first step) to target zipkin and it'd work for both

SamBarker

I certainly think tracing is a good idea for the canary.

Console tracing works for me as a proof of concept, but would need something else to actually go forward. I think the config-map approach suggested else where is a promising way to go.

cmd/main.go

melchiormoulin · 2022-05-25T15:01:32Z

i've just done the jaeger exporter
For the future we can use the env variable OTEL_TRACES_EXPORTER : open-telemetry/opentelemetry-go#2310
The configuration of this exporter is with the default OTEL env variables

melchiormoulin · 2022-05-31T08:41:37Z

is all good on this PR ?

SamBarker

On the right track for me.

My only concern is how best to handle the exporter selection.

cmd/main.go

internal/config/canary_config.go

cmd/main.go

README.md

cmd/main.go

internal/config/canary_config.go

melchiormoulin · 2022-06-20T16:08:24Z

is all good ?

cmd/main.go

README.md

cmd/main.go

internal/services/consumer.go

internal/services/producer.go

cmd/main.go

internal/config/canary_config.go

README.md

ppatierno · 2022-06-29T10:24:18Z

@melchiormoulin could you fix the DCO and conflicts please? So that we can have another pass. Thanks!

melchiormoulin · 2022-06-29T16:56:55Z

i can't easily do the DCO with the merge, can you help me ?

ppatierno · 2022-06-29T16:58:53Z

@melchiormoulin I think we are going to make a 0.4.0 release out for the latest important bug fixes merged today. The release candidate could be out tomorrow. This PR still needs another pass from all reviewers and I was wondering if you thought about adding some tests. I think it will make 0.5.0 but it doesn't mean that it will be so late after 0.4.0.

melchiormoulin · 2022-06-29T17:02:46Z

yes i can add some test but i wanted to be sure all people are ok with the current implementation.

melchiormoulin · 2022-06-29T17:35:01Z

i added some tests

melchiormoulin · 2022-06-30T09:44:23Z

can you just help me for dco ? the git rebase HEAD~16 --signoff command totally mess up.

scholzj

LGTM. Thanks for the PR.

cmd/main.go

ppatierno

@melchiormoulin sorry for the delay! I left a couple of more comments but then we should be ok on my side.
You should fix DCO and check why tests are failing.

internal/security/auth.go

internal/security/auth_test.go

Opentelemetry tracing with Jaeger or OTEL exporter configured by EXPORTER_TYPE_TRACING env variable Signed-off-by: Melchior Moulin <[email protected]>

melchiormoulin · 2022-07-18T12:42:04Z

@ppatierno seems good now

ppatierno

@melchiormoulin thank you very much! Sorry for this PR taking so long!

scholzj requested a review from ppatierno May 24, 2022 16:29

scholzj reviewed May 24, 2022

View reviewed changes

SamBarker reviewed May 25, 2022

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

cmd/main.go Show resolved Hide resolved

melchiormoulin force-pushed the add_otlp branch from 62de362 to 12b94ca Compare May 25, 2022 14:54

melchiormoulin force-pushed the add_otlp branch from bd39772 to 46b2ebc Compare May 30, 2022 13:09

melchiormoulin force-pushed the add_otlp branch 4 times, most recently from 32a5f09 to e5a1e42 Compare May 31, 2022 15:04

SamBarker reviewed Jun 1, 2022

View reviewed changes

SamBarker reviewed Jun 9, 2022

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

cmd/main.go Show resolved Hide resolved

SamBarker reviewed Jun 13, 2022

View reviewed changes

cmd/main.go Show resolved Hide resolved

internal/config/canary_config.go Outdated Show resolved Hide resolved

melchiormoulin force-pushed the add_otlp branch from 2a5a217 to dc38e72 Compare June 14, 2022 08:27

ppatierno reviewed Jun 23, 2022

View reviewed changes

k-wall reviewed Jun 27, 2022

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

k-wall reviewed Jun 27, 2022

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

k-wall reviewed Jun 27, 2022

View reviewed changes

internal/config/canary_config.go Outdated Show resolved Hide resolved

melchiormoulin force-pushed the add_otlp branch from 3e41628 to 4e84099 Compare June 28, 2022 12:02

ppatierno reviewed Jun 29, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

melchiormoulin force-pushed the add_otlp branch from 29ccb52 to b8e5bac Compare June 29, 2022 16:08

melchiormoulin force-pushed the add_otlp branch 5 times, most recently from 3183f82 to 56cc874 Compare June 29, 2022 16:55

ppatierno added this to the 0.5.0 milestone Jun 30, 2022

melchiormoulin force-pushed the add_otlp branch 3 times, most recently from 4131487 to 56cc874 Compare July 3, 2022 21:29

scholzj approved these changes Jul 4, 2022

View reviewed changes

pavolloffay reviewed Jul 5, 2022

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

cmd/main.go Outdated Show resolved Hide resolved

cmd/main.go Outdated Show resolved Hide resolved

cmd/main.go Outdated Show resolved Hide resolved

melchiormoulin force-pushed the add_otlp branch from 32e3576 to 3171aa0 Compare July 6, 2022 07:39

ppatierno reviewed Jul 14, 2022

View reviewed changes

internal/security/auth.go Outdated Show resolved Hide resolved

internal/security/auth_test.go Outdated Show resolved Hide resolved

Add tracing telemetry

fbdea82

Opentelemetry tracing with Jaeger or OTEL exporter configured by EXPORTER_TYPE_TRACING env variable Signed-off-by: Melchior Moulin <[email protected]>

melchiormoulin force-pushed the add_otlp branch from 3171aa0 to fbdea82 Compare July 18, 2022 12:36

ppatierno approved these changes Jul 18, 2022

View reviewed changes

ppatierno merged commit d7cc85d into strimzi:main Jul 18, 2022

ppatierno mentioned this pull request Jul 18, 2022

Add OpenTelemetry support #197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracing with opentelemetry go sarama wrapper #179

Add tracing with opentelemetry go sarama wrapper #179

melchiormoulin commented May 24, 2022

scholzj left a comment

melchiormoulin commented May 24, 2022

scholzj commented May 24, 2022

melchiormoulin commented May 25, 2022

devguyio commented May 25, 2022

ppatierno commented May 25, 2022

devguyio commented May 25, 2022 •

edited

Loading

SamBarker left a comment

melchiormoulin commented May 25, 2022

melchiormoulin commented May 31, 2022

SamBarker left a comment

melchiormoulin commented Jun 20, 2022

ppatierno commented Jun 29, 2022 •

edited

Loading

melchiormoulin commented Jun 29, 2022

ppatierno commented Jun 29, 2022

melchiormoulin commented Jun 29, 2022 •

edited

Loading

melchiormoulin commented Jun 29, 2022

melchiormoulin commented Jun 30, 2022

scholzj left a comment

ppatierno left a comment •

edited

Loading

melchiormoulin commented Jul 18, 2022

ppatierno left a comment

Add tracing with opentelemetry go sarama wrapper #179

Add tracing with opentelemetry go sarama wrapper #179

Conversation

melchiormoulin commented May 24, 2022

scholzj left a comment

Choose a reason for hiding this comment

melchiormoulin commented May 24, 2022

scholzj commented May 24, 2022

melchiormoulin commented May 25, 2022

devguyio commented May 25, 2022

ppatierno commented May 25, 2022

devguyio commented May 25, 2022 • edited Loading

SamBarker left a comment

Choose a reason for hiding this comment

melchiormoulin commented May 25, 2022

melchiormoulin commented May 31, 2022

SamBarker left a comment

Choose a reason for hiding this comment

melchiormoulin commented Jun 20, 2022

ppatierno commented Jun 29, 2022 • edited Loading

melchiormoulin commented Jun 29, 2022

ppatierno commented Jun 29, 2022

melchiormoulin commented Jun 29, 2022 • edited Loading

melchiormoulin commented Jun 29, 2022

melchiormoulin commented Jun 30, 2022

scholzj left a comment

Choose a reason for hiding this comment

ppatierno left a comment • edited Loading

Choose a reason for hiding this comment

melchiormoulin commented Jul 18, 2022

ppatierno left a comment

Choose a reason for hiding this comment

devguyio commented May 25, 2022 •

edited

Loading

ppatierno commented Jun 29, 2022 •

edited

Loading

melchiormoulin commented Jun 29, 2022 •

edited

Loading

ppatierno left a comment •

edited

Loading