feat: implement distributed tracing using OpenTelemetry and Gin (#189)#225
feat: implement distributed tracing using OpenTelemetry and Gin (#189)#225prasiddhi-105 wants to merge 5 commits into
Conversation
|
All contributors have signed the CLA. ✅ Thank you! |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR initializes OpenTelemetry tracing in the server and OAuth test client, adds request IDs to Gin requests, validates CORS origins from config, and updates Go and Node dependencies for the tracing stack. ChangesDistributed tracing and request context
Sequence Diagram(s)sequenceDiagram
participant Main as cmd/server/main.go
participant InitTracer as internal/telementry.InitTracer
participant Provider as TracerProvider
participant Server as srv
Main->>InitTracer: initialize OpenTelemetry
InitTracer->>Provider: configure OTLP exporter, resource, and propagator
Main->>Provider: tp.Shutdown(timeoutCtx)
Main->>Server: srv.Shutdown(timeoutCtx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/routes/routes.go (1)
70-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDuplicate CORS and Security middleware registration.
CORSMiddleware(cfg)andSecurityMiddleware()are already registered at lines 71-72 and then registered again at lines 76-77. Each request will run both twice, emitting duplicate CORS/security headers. MultipleAccess-Control-Allow-Originvalues cause browsers to reject the response, breaking CORS. Remove the original pair and keepRequestIDMiddleware()first so the correlation ID is available to all downstream middleware.As per coding guidelines, the middleware stack (security headers, CORS, ...) is registered via `routes.SetupRoutes()`; the duplicated registration violates a clean single stack.🔧 Proposed fix
- // Apply global middleware - router.Use(middleware.CORSMiddleware(cfg)) - router.Use(middleware.SecurityMiddleware()) // Security headers - - // Apply global middleware - router.Use(middleware.RequestIDMiddleware()) - router.Use(middleware.CORSMiddleware(cfg)) - router.Use(middleware.SecurityMiddleware()) // Security headers + // Apply global middleware + router.Use(middleware.RequestIDMiddleware()) + router.Use(middleware.CORSMiddleware(cfg)) + router.Use(middleware.SecurityMiddleware()) // Security headers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/routes/routes.go` around lines 70 - 78, The middleware stack in routes.SetupRoutes() registers CORSMiddleware(cfg) and SecurityMiddleware() twice, which causes duplicate CORS/security headers on every request. Remove the earlier duplicate pair and keep RequestIDMiddleware() at the start of the chain so the correlation ID is available to downstream middleware, while preserving a single clean middleware registration sequence.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/telementry/tracer.go (1)
12-12: 📐 Maintainability & Code Quality | 🔵 TrivialAlign
semconvwith OpenTelemetryv1.44.0
internal/telementry/tracer.gostill importsgo.opentelemetry.io/otel/semconv/v1.4.0;otel v1.44.0ships withsemconv/v1.41.0, so switching to that keeps resource attribute names and schema versions in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/telementry/tracer.go` at line 12, The tracer setup is importing an outdated OpenTelemetry semconv package, so update the import in tracer.go to use the semconv version that matches otel v1.44.0 instead of v1.4.0. Locate the change in the tracer initialization code where semconv is used for resource/schema attributes and switch the reference there to the v1.41.0 package so the attribute names and schema version stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/oauth-test-client/main.go`:
- Around line 21-28: The deferred tracer flush in main never runs because
log.Fatal on http.ListenAndServe exits the process before defers execute. Update
main to handle server lifecycle explicitly: start the HTTP server without
log.Fatal, listen for a shutdown signal, and call tp.Shutdown from that shutdown
path so spans are flushed reliably. Use the existing tp.Shutdown deferred block
and the server startup around http.ListenAndServe to locate the fix.
- Around line 8-9: The telemetry import in main should use the actual module
path from go.mod, not the placeholder module name. Update the import in
cmd/oauth-test-client/main.go that references internal/telemetry so it matches
github.com/roshankumar0036singh/auth-server/internal/telemetry, and verify any
related imports in main() compile with the corrected path.
In `@cmd/server/main.go`:
- Around line 116-129: The shutdown sequence in the server’s cleanup path is
reversed: `tp.Shutdown` is called before `srv.Shutdown`, which can drop spans
from requests still draining. Reorder the logic so the HTTP server is shut down
first using the existing `shutdownCtx`, then call `tp.Shutdown` after the server
has stopped accepting and processing requests. Keep the cleanup in the same
shutdown block and preserve the existing `srv.Shutdown` and `tp.Shutdown` error
handling.
In `@go.mod`:
- Around line 79-97: Add the missing OpenTelemetry Gin instrumentation
dependency to keep cmd/server/main.go buildable. Update go.mod to include
go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin,
then run the module tidy/update flow so go.sum is generated or refreshed. Make
sure the dependency is recorded alongside the existing otel-related entries so
the otelgin import resolves cleanly.
In `@internal/config/config.go`:
- Around line 109-114: The CORS origin validation in the config loader is too
permissive because it accepts any structurally valid URL, including paths,
queries, fragments, and trailing slashes that will never match a browser Origin
value. Tighten the validation in the config parsing logic around
url.ParseRequestURI so only bare scheme+host origins are accepted, and
explicitly reject entries with Path, RawQuery, Fragment, or a non-root trailing
slash before they reach AllowOrigins. Keep the warning log in the same
validation branch so invalid non-origin entries are skipped early.
In `@internal/middleware/cors.go`:
- Around line 13-18: The CORS fallback in cors.go is effectively bypassed
because cfg.Security.AllowedOrigins is always populated by config.go, so the
default localhost dev origin for Vite is lost. Update the allow-list handling in
the middleware around the allowOrigins assignment so the defaults are merged
with the configured origins instead of being replaced, or otherwise ensure the
default dev origins are seeded into the config path. Use the existing symbols
cfg.Security.AllowedOrigins and allowOrigins in internal/middleware/cors.go, and
make sure http://localhost:5173 remains permitted without requiring every caller
to set CORS_ALLOWED_ORIGINS explicitly.
In `@internal/telementry/tracer.go`:
- Line 1: The package directory is misspelled, so Go imports won’t resolve this
module correctly. Rename the package location and update the tracer package so
it lives under internal/telemetry instead of internal/telementry, and make sure
any references to the telemetry package still match the corrected directory
name.
---
Outside diff comments:
In `@internal/routes/routes.go`:
- Around line 70-78: The middleware stack in routes.SetupRoutes() registers
CORSMiddleware(cfg) and SecurityMiddleware() twice, which causes duplicate
CORS/security headers on every request. Remove the earlier duplicate pair and
keep RequestIDMiddleware() at the start of the chain so the correlation ID is
available to downstream middleware, while preserving a single clean middleware
registration sequence.
---
Nitpick comments:
In `@internal/telementry/tracer.go`:
- Line 12: The tracer setup is importing an outdated OpenTelemetry semconv
package, so update the import in tracer.go to use the semconv version that
matches otel v1.44.0 instead of v1.4.0. Locate the change in the tracer
initialization code where semconv is used for resource/schema attributes and
switch the reference there to the v1.41.0 package so the attribute names and
schema version stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62223459-f893-4d63-bac6-802c4a095842
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
cmd/oauth-test-client/main.gocmd/server/main.gogo.modinternal/config/config.gointernal/middleware/cors.gointernal/middleware/request_id.gointernal/routes/routes.gointernal/telementry/tracer.gopackage.json
| // Validate if it is a structurally valid URL | ||
| parsedURL, err := url.ParseRequestURI(trimmed) | ||
| if err != nil || parsedURL.Scheme == "" || parsedURL.Host == "" { | ||
| log.Printf("Warning: Skipping invalid CORS origin configuration: %s", trimmed) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Does gin-contrib/cors v1.7.6 match the AllowOrigins entries against the request Origin header using exact string comparison, and does it normalize trailing slashes or paths?
💡 Result:
In gin-contrib/cors v1.7.6, the AllowOrigins entries are matched against the request Origin header using exact string comparison for standard entries [1][2]. Specifically, the validation logic iterates through the configured AllowOrigins list and performs a direct equality check (value == origin) when the entry is not a regular expression or a wildcard pattern [2]. Regarding normalization: 1. Trailing Slashes: The middleware does not explicitly normalize trailing slashes in the Origin header or the AllowOrigins configuration during the matching process [2][3]. Because the matching is primarily based on exact string equality, a mismatch in trailing slashes (e.g., "https://example.com" vs. "https://example.com/") will result in a validation failure unless specifically configured otherwise [2]. 2. Paths: The middleware also does not normalize paths [2]. The Origin header (as defined in RFC 6454) typically consists of a scheme, host, and port, and does not include a path component; therefore, path normalization is not applicable to the Origin header validation performed by this middleware [2][3]. If your use case requires flexible matching (such as ignoring trailing slashes or other variations), you should use the AllowOriginFunc configuration option, which allows you to implement custom validation logic for the Origin string [4][1].
Citations:
- 1: https://github.com/gin-contrib/cors/tree/v1.7.6
- 2: https://github.com/gin-contrib/cors/blob/master/config.go
- 3: https://github.com/gin-contrib/cors/blob/v1.7.7/config.go
- 4: https://pkg.go.dev/github.com/gin-contrib/cors@v1.7.6
🏁 Script executed:
sed -n '1,220p' internal/config/config.go && printf '\n--- go.mod ---\n' && sed -n '1,220p' go.modRepository: roshankumar0036singh/auth-server
Length of output: 9644
🏁 Script executed:
sed -n '1,220p' internal/config/config.go && printf '\n--- go.mod ---\n' && sed -n '1,220p' go.modRepository: roshankumar0036singh/auth-server
Length of output: 9644
Reject non-origin CORS entries
gin-contrib/cors v1.7.6 matches AllowOrigins by exact string, so any path, query, fragment, or trailing slash will never match the browser Origin header. Reject these at config load instead of letting a bad entry fail silently at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/config.go` around lines 109 - 114, The CORS origin validation
in the config loader is too permissive because it accepts any structurally valid
URL, including paths, queries, fragments, and trailing slashes that will never
match a browser Origin value. Tighten the validation in the config parsing logic
around url.ParseRequestURI so only bare scheme+host origins are accepted, and
explicitly reject entries with Path, RawQuery, Fragment, or a non-root trailing
slash before they reach AllowOrigins. Keep the warning log in the same
validation branch so invalid non-origin entries are skipped early.
|
I have read the CLA and agree to its terms |
there should be a full stop at the end. Please re-comment Also kindly attend to the merge conflicts. |
|
|
I have read the CLA and agree to its terms. |
|
Please tend to these:
|
2 participants
Context
Resolves the tedious nature of debugging cross-microservice OAuth flow failures by replacing standard text logs with a comprehensive, unified distributed tracing layer.
Changes
internal/telemetry/tracer.go.cmd/server/main.go).otelginrouting middleware to automatically capture, trace, and traceparent-propagate every incoming and outgoing HTTP context boundary.Closes #189
Summary by CodeRabbit
New Features
Bug Fixes