Skip to content

feat: add OTEL instrumentation for authentication and handlers#25296

Merged
agaudreault merged 5 commits intoargoproj:masterfrom
devopsjedi:otel-instrumentation
Feb 10, 2026
Merged

feat: add OTEL instrumentation for authentication and handlers#25296
agaudreault merged 5 commits intoargoproj:masterfrom
devopsjedi:otel-instrumentation

Conversation

@devopsjedi
Copy link
Contributor

@devopsjedi devopsjedi commented Nov 15, 2025 •
edited
Loading

Follow up to #23727, improving observability for the OIDC authentication flow:

  • Add OTEL tracer initialization in server, session, oidc packages
  • Add span creation to all methods for tracing authentication flow with SSO
  • Wrap HTTP handlers with otelhttp.NewHandler to capture the root span
  • Add error status and attributes to spans'
  • Capture cache read/write timing

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Title of the PR
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@devopsjedi devopsjedi requested a review from a team as a code owner November 15, 2025 01:27
@bunnyshell
Copy link

bunnyshell bot commented Nov 15, 2025 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@devopsjedi devopsjedi force-pushed the otel-instrumentation branch 3 times, most recently from dcc37c2 to 9221027 Compare November 15, 2025 01:41
@codecov
Copy link

codecov bot commented Nov 15, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 73.55372% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.52%. Comparing base (3bf3d8a) to head (faa209d).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
util/oidc/oidc.go 71.95% 22 Missing and 1 partial ⚠️
util/oidc/provider.go 64.28% 5 Missing ⚠️
server/server.go 76.47% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #25296      +/-   ##
==========================================
+ Coverage   62.49%   62.52%   +0.02%     
==========================================
  Files         351      351              
  Lines       49602    49697      +95     
==========================================
+ Hits        31001    31072      +71     
- Misses      15631    15655      +24     
  Partials     2970     2970

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@afzal442
Copy link
Member

afzal442 commented Nov 18, 2025

Could you share the snapshot of metrics on a visualization tool?

@devopsjedi
Copy link
Contributor Author

devopsjedi commented Nov 18, 2025

Could you share the snapshot of metrics on a visualization tool?

Here are some examples of the new spans:

server.ArgoCDServer.Authenticate

This is a new span to capture an authentication flow:
image

session.SessionService/GetUserInfo

Captured by gRPC middleware, showing new oidc spans
image

cluster.SettingsService/Get

Captured by gRPC middleware, showing new oidc spans
image

server.ClientApp/HandleCallback

This was captured by the new OTEL middleware handler:
image

@devopsjedi
Copy link
Contributor Author

devopsjedi commented Nov 18, 2025

Could you share the snapshot of metrics on a visualization tool?

Capturing cache read/write timing. We should also consider extending the cache interface in the future to accept a context so it could add its own spans.

image image

@afzal442
Copy link
Member

afzal442 commented Nov 19, 2025

I thought it would be Jaeger, since I worked on OTEL instrumentation part. 😄 Thanks for sharing those.

@agaudreault agaudreault self-assigned this Dec 14, 2025
@devopsjedi devopsjedi force-pushed the otel-instrumentation branch from 562fd91 to c6045b3 Compare January 5, 2026 18:01
@devopsjedi
feat: add OTEL instrumentation for authentication and handlers
6549613 
* Add OTEL tracer initialization in server, session, oidc packages
* Add span creation to all methods for tracing authentication flow with SSO
* Wrap HTTP handlers with otelhttp.NewHandler to capture the root span
* Add error status and attributes to spans'
* Capture cache read/write timing

Signed-off-by: Mike Cutsail <mcutsail15@apple.com>
@devopsjedi devopsjedi force-pushed the otel-instrumentation branch from c6045b3 to 6549613 Compare January 5, 2026 18:21
agaudreault
agaudreault requested changes Feb 3, 2026
View reviewed changes
Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

LGTM. Few small things

server/server.go Outdated
// OIDC tokens will be verified but will not be refreshed here.
claims, newToken, err := server.sessionMgr.VerifyToken(ctx, tokenString)
if err != nil {
span.SetAttributes(attribute.String("token", tokenString))
Copy link
Member

@agaudreault agaudreault Feb 3, 2026

Choose a reason for hiding this comment

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

I dont think we should add the token here.

Suggested change
span.SetAttributes(attribute.String("token", tokenString))

util/oidc/oidc.go Outdated
// GetTokenSourceFromCache creates an oauth2 TokenSource from a cached oidc token. The TokenSource will be configured
// with an early expiration based on the refreshTokenThreshold.
func (a *ClientApp) GetTokenSourceFromCache(ctx context.Context, oidcTokenCache *OidcTokenCache) (oauth2.TokenSource, error) {
spanCtx, span := tracer.Start(ctx, "oidc.ClientApp.GetTokenSourceFromCache")
Copy link
Member

@agaudreault agaudreault Feb 3, 2026

Choose a reason for hiding this comment

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

Afaik, it is good practice to just overwrite the ctx.

Suggested change
spanCtx, span := tracer.Start(ctx, "oidc.ClientApp.GetTokenSourceFromCache")
ctx, span = tracer.Start(ctx, "oidc.ClientApp.GetTokenSourceFromCache")

util/oidc/oidc.go Outdated
Comment on lines +217 to +221
span.SetAttributes(
attribute.String("network", network),
attribute.String("addr", addr),
)
defer span.End()
Copy link
Member

@agaudreault agaudreault Feb 3, 2026

Choose a reason for hiding this comment

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

defer as close as possible

Suggested change
span.SetAttributes(
attribute.String("network", network),
attribute.String("addr", addr),
)
defer span.End()
defer span.End()
span.SetAttributes(
attribute.String("network", network),
attribute.String("addr", addr),
)

util/oidc/oidc.go Outdated
defer span.End()

ctx = gooidc.ClientContext(ctx, a.client)
if span.IsRecording() {
Copy link
Member

@agaudreault agaudreault Feb 3, 2026

Choose a reason for hiding this comment

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

I dont think there is any performance need to use IsRecording func here. Correct me if I am wrong, but this is in case expensive operation needs to be done to set the attributes.

go.mod Outdated
sigs.k8s.io/yaml v1.6.0
)

require (
Copy link
Member

@agaudreault agaudreault Feb 3, 2026

Choose a reason for hiding this comment

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

Move to the common require section

@agaudreault
Merge branch 'master' into otel-instrumentation
cdb47d5 
Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
@agaudreault agaudreault enabled auto-merge (squash) February 10, 2026 12:48
@agaudreault agaudreault merged commit 2793097 into argoproj:master Feb 10, 2026
26 checks passed
ppapapetrou76 pushed a commit to ppapapetrou76/argo-cd that referenced this pull request Feb 12, 2026
@devopsjedi @agaudreault @ppapapetrou76
feat: add OTEL instrumentation for authentication and handlers (argop…
f56472d 
…roj#25296)

Signed-off-by: Mike Cutsail <mcutsail15@apple.com>
Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
devopsjedi added a commit to devopsjedi/argo-cd that referenced this pull request Mar 7, 2026
@devopsjedi @agaudreault
feat: add OTEL instrumentation for authentication and handlers (argop…
af04642 
…roj#25296)

Signed-off-by: Mike Cutsail <mcutsail15@apple.com>
Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agaudreault agaudreault agaudreault approved these changes

Assignees

@agaudreault agaudreault

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@devopsjedi @afzal442 @agaudreault