Skip to content
This repository has been archived by the owner on Nov 29, 2024. It is now read-only.

Add tracing with opentelemetry go sarama wrapper #179

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Add tracing with opentelemetry go sarama wrapper #179

merged 1 commit into from
Jul 18, 2022

Conversation

melchiormoulin
Copy link
Contributor

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 scholzj requested a review from ppatierno May 24, 2022 16:29
scholzj
scholzj reviewed May 24, 2022
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Member

scholzj commented May 24, 2022

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
Copy link
Contributor Author

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

@devguyio
Copy link
Member

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
Copy link
Member

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
Copy link
Member

devguyio commented May 25, 2022 •
edited
Loading

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
SamBarker reviewed May 25, 2022
View reviewed changes
Copy link

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
@melchiormoulin
Copy link
Contributor Author

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
Copy link
Contributor Author

is all good on this PR ?

@melchiormoulin melchiormoulin force-pushed the add_otlp branch 4 times, most recently from 32a5f09 to e5a1e42 Compare May 31, 2022 15:04
SamBarker
SamBarker reviewed Jun 1, 2022
View reviewed changes
Copy link

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the right track for me.

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

cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
internal/config/canary_config.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
SamBarker
SamBarker reviewed Jun 9, 2022
View reviewed changes
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
SamBarker
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
Copy link
Contributor Author

is all good ?

ppatierno
ppatierno reviewed Jun 23, 2022
View reviewed changes
cmd/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
internal/services/consumer.go Outdated Show resolved Hide resolved
internal/services/producer.go Show resolved Hide resolved
k-wall
k-wall reviewed Jun 27, 2022
View reviewed changes
cmd/main.go Outdated Show resolved Hide resolved
k-wall
k-wall reviewed Jun 27, 2022
View reviewed changes
cmd/main.go Outdated Show resolved Hide resolved
ppatierno
ppatierno reviewed Jun 29, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
@ppatierno
Copy link
Member

ppatierno commented Jun 29, 2022 •
edited
Loading

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

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

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

@ppatierno
Copy link
Member

@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
Copy link
Contributor Author

melchiormoulin commented Jun 29, 2022 •
edited
Loading

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

@melchiormoulin
Copy link
Contributor Author

i added some tests

@ppatierno ppatierno added this to the 0.5.0 milestone Jun 30, 2022
@melchiormoulin
Copy link
Contributor Author

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

@melchiormoulin melchiormoulin force-pushed the add_otlp branch 3 times, most recently from 4131487 to 56cc874 Compare July 3, 2022 21:29
scholzj
scholzj approved these changes Jul 4, 2022
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the PR.

pavolloffay
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
ppatierno
ppatierno reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 Outdated Show resolved Hide resolved
internal/security/auth_test.go Outdated Show resolved Hide resolved
@melchiormoulin
Add tracing telemetry
fbdea82 
Opentelemetry tracing with Jaeger or OTEL exporter configured by EXPORTER_TYPE_TRACING env variable

Signed-off-by: Melchior Moulin <melchior.moulin@gmail.com>
@melchiormoulin
Copy link
Contributor Author

@ppatierno seems good now

ppatierno
ppatierno approved these changes Jul 18, 2022
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ppatierno ppatierno merged commit d7cc85d into strimzi:main Jul 18, 2022
@ppatierno ppatierno mentioned this pull request Jul 18, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@k-wall k-wall k-wall left review comments

@pavolloffay pavolloffay pavolloffay left review comments

@SamBarker SamBarker SamBarker left review comments

@ppatierno ppatierno ppatierno approved these changes

@scholzj scholzj scholzj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
7 participants
@melchiormoulin @scholzj @devguyio @ppatierno @SamBarker @pavolloffay @k-wall