feat: support opentelemetry for grpc tracing #7539

yeya24 · 2021-10-24T18:20:29Z

This pr adds support for basic grpc level tracing using otelgrpc to send traces to opentelemetry collector.

Checklist:

yeya24 · 2021-10-24T19:24:30Z

Looks like it is not working due to the grpc version upgrade. Will try to resolve it.

cmd/argocd-repo-server/commands/argocd_repo_server.go

yeya24 · 2021-10-25T08:21:21Z

I can see grpc server tracing is working now. There is still some problems about the protobuf version and once I fix it I will mark the pr as ready.

cmd/util/trace.go

yeya24 · 2022-02-12T05:58:15Z

Close this for now and hopefully we can work back on this once grpc version is newer

leoluz · 2022-04-18T19:13:26Z

Reopening this as we now have updated gRPC runtime library to the latest version (v1.45.0) in ArgoCD.

leoluz

Please revert some of the changes (I added a proper comment in those) before rebasing to avoid dealing with unnecessary conflicts.

cmd/argocd-server/commands/argocd_server.go

cmd/util/trace.go

go.mod

pkg/apiclient/grpcproxy.go

cmd/util/trace.go

cmd/argocd-server/commands/argocd_server.go

leoluz · 2022-04-18T19:56:37Z

cmd/util/trace.go

+)
+
+func InitTracer(serviceName, otlpAddress string) (trace.TracerProvider, func(), error) {
+	ctx := context.Background()


We should probably receive the context from the main server build the resource with it.

Sorry but it seems that this is still not using the context from the server.
This is the context that I was referring to:

argo-cd/cmd/argocd-server/commands/argocd_server.go

Line 168 in 7beb8b6

ctx, cancel := context.WithCancel(ctx)

Sorry I missed that. One question here, why do we need the for loop?

argo-cd/cmd/argocd-server/commands/argocd_server.go

Line 166 in 7beb8b6

for {

, can I move the ctx out of the loop because I don't want to put the init tracer function in the loop

The for-loop is mainly to automatically restart the server if it eventually stops for any reason. You can not move the ctx out of the for loop. Note that argocd.Run(...) is a blocking call. Ideally the initialization of the tracer should be inside the Run() function just like how it is done for the prometheus metric server.

Thanks for the detailed reply. Then I think this is fixed.

Sorry but you can't move the ctx out of the for-loop. A new context need to be build on every iteration and the initTracer call needs to happen somewhere inside this loop.

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

codecov · 2022-04-19T20:33:41Z

Codecov Report

Merging #7539 (26f3cf4) into master (c7ff388) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7539      +/-   ##
==========================================
- Coverage   45.63%   45.50%   -0.14%     
==========================================
  Files         217      219       +2     
  Lines       25696    25895     +199     
==========================================
+ Hits        11727    11783      +56     
- Misses      12337    12473     +136     
- Partials     1632     1639       +7

Impacted Files	Coverage Δ
server/server.go	`54.23% <100.00%> (-1.31%)`	⬇️
applicationset/services/scm_provider/github.go	`63.52% <0.00%> (-17.65%)`	⬇️
util/cert/cert.go	`82.40% <0.00%> (-2.35%)`	⬇️
util/settings/settings.go	`48.10% <0.00%> (ø)`
server/rbacpolicy/rbacpolicy.go	`82.35% <0.00%> (ø)`
cmd/argocd/commands/admin/settings_rbac.go	`23.24% <0.00%> (ø)`
...is/applicationset/v1alpha1/applicationset_types.go	`47.22% <0.00%> (ø)`
server/application/websocket.go	`8.33% <0.00%> (ø)`
server/application/terminal.go	`3.60% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ff388...26f3cf4. Read the comment docs.

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

leoluz · 2022-04-20T19:01:22Z

cmd/argocd-server/commands/argocd_server.go

-				ctx := context.Background()
-				ctx, cancel := context.WithCancel(ctx)


This can't be moved outside this loop.

If this cannot be moved, then the InitTracer call should be inside the loop as well? Because it needs the server context

Correct. A new context needs to be built on every loop iteration and the InitTracer call needs to happen somewhere inside this loop.

This seems a little bit weird to me. But anyway I updated the code.

util/trace/trace.go

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

yeya24 · 2022-04-25T04:26:39Z

Hello @leoluz, is there any action item left for this PR?

cmd/argocd-server/commands/argocd_server.go

leoluz · 2022-04-25T23:51:48Z

Hello @leoluz, is there any action item left for this PR?

@yeya24 Added one more comment.
Otherwise LGTM!

leoluz

LGTM

yeya24 mentioned this pull request Oct 24, 2021

Add OpenTelemetry tracing integration #4972

Closed

yeya24 force-pushed the support-otelgrpc branch 2 times, most recently from 19cc92c to 5d7c1ab Compare October 24, 2021 19:05

wtam2018 reviewed Oct 25, 2021

View reviewed changes

cmd/argocd-repo-server/commands/argocd_repo_server.go Show resolved Hide resolved

yeya24 marked this pull request as draft October 25, 2021 02:48

yeya24 marked this pull request as ready for review October 25, 2021 08:20

yeya24 marked this pull request as draft October 25, 2021 08:26

tobert reviewed Nov 18, 2021

View reviewed changes

cmd/util/trace.go Outdated Show resolved Hide resolved

yeya24 closed this Feb 12, 2022

leoluz mentioned this pull request Mar 18, 2022

feat: add opentracing support #8487

Closed

10 tasks

leoluz reopened this Apr 18, 2022

leoluz requested changes Apr 18, 2022

View reviewed changes

Ben Ye added 2 commits April 19, 2022 11:03

feat: add support for otel grpc tracing

485d6fb

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

chore: rebase code to master

c9c86b8

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

yeya24 force-pushed the support-otelgrpc branch from 5d7c1ab to c9c86b8 Compare April 19, 2022 18:53

feat: add otelgrpc to cmpserver

2012eb2

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

yeya24 marked this pull request as ready for review April 19, 2022 19:10

feat: add util trace package

7beb8b6

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

fix: fix tracer context

03fae39

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

leoluz reviewed Apr 20, 2022

View reviewed changes

util/trace/trace.go Show resolved Hide resolved

chore: move tracer initialization part to the loop

26f3cf4

Signed-off-by: Ben Ye <ben.ye@bytedance.com>

leoluz reviewed Apr 25, 2022

View reviewed changes

cmd/argocd-server/commands/argocd_server.go Show resolved Hide resolved

leoluz approved these changes Apr 26, 2022

View reviewed changes

leoluz merged commit 09e5b60 into argoproj:master Apr 26, 2022

yeya24 deleted the support-otelgrpc branch April 27, 2022 04:39

leoluz added this to the v2.4 milestone Apr 28, 2022

jannfis mentioned this pull request May 2, 2022

fix: Prevent crash on server reload if opentelemetry is not enabled #9270

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support opentelemetry for grpc tracing #7539

feat: support opentelemetry for grpc tracing #7539

yeya24 commented Oct 24, 2021 •

edited

Loading

yeya24 commented Oct 24, 2021

yeya24 commented Oct 25, 2021 •

edited

Loading

yeya24 commented Feb 12, 2022

leoluz commented Apr 18, 2022

leoluz left a comment

leoluz Apr 18, 2022

yeya24 Apr 19, 2022

leoluz Apr 19, 2022

yeya24 Apr 20, 2022

leoluz Apr 20, 2022 •

edited

Loading

yeya24 Apr 20, 2022

leoluz Apr 20, 2022

codecov bot commented Apr 19, 2022 •

edited

Loading

leoluz Apr 20, 2022

yeya24 Apr 20, 2022

leoluz Apr 20, 2022

yeya24 Apr 20, 2022

yeya24 commented Apr 25, 2022

leoluz commented Apr 25, 2022 •

edited

Loading

leoluz left a comment

		ctx := context.Background()
		ctx, cancel := context.WithCancel(ctx)

feat: support opentelemetry for grpc tracing #7539

feat: support opentelemetry for grpc tracing #7539

Conversation

yeya24 commented Oct 24, 2021 • edited Loading

yeya24 commented Oct 24, 2021

yeya24 commented Oct 25, 2021 • edited Loading

yeya24 commented Feb 12, 2022

leoluz commented Apr 18, 2022

leoluz 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

leoluz Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 19, 2022 • edited Loading

Codecov Report

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

yeya24 commented Apr 25, 2022

leoluz commented Apr 25, 2022 • edited Loading

leoluz left a comment

Choose a reason for hiding this comment

yeya24 commented Oct 24, 2021 •

edited

Loading

yeya24 commented Oct 25, 2021 •

edited

Loading

leoluz Apr 20, 2022 •

edited

Loading

codecov bot commented Apr 19, 2022 •

edited

Loading

leoluz commented Apr 25, 2022 •

edited

Loading