Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntegrates OpenTelemetry tracing: adds packageName constants, updates server.Handler to accept a handler name and start spans, instruments error middleware to create/record spans and set statuses, updates call sites/tests, and updates go.mod dependencies for OpenTelemetry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gin as "Gin Handler"
participant Server as "pkg/server.Handler"
participant Middleware as "Error Middleware"
participant Tracer as "OpenTelemetry Tracer"
Client->>Gin: HTTP request
Gin->>Middleware: enter middleware chain (context)
Middleware->>Tracer: Start span (middleware scope)
Middleware->>Server: call Handler with ctx (context contains middleware span)
Server->>Tracer: Start span (handlerName)
Server->>Server: execute business handler (may return error)
alt success
Server->>Client: JSON success response (status)
else error
Server->>Middleware: ctx.Error(...) or return error
Middleware->>Tracer: Record error, set span status
Middleware->>Client: write error response / recovery
end
Server->>Tracer: End handler span
Middleware->>Tracer: End middleware span
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
pkg/server/utils.go (1)
100-112: Consider recording error status on the span.The tracing integration looks correct. However, when the handler returns an error, the span doesn't capture error information. Recording the error on the span would improve trace visibility for debugging.
💡 Optional: Record error on span
import ( "github.com/gin-gonic/gin" "github.com/gin-gonic/gin/binding" "github.com/itsLeonB/ezutil/v2" "github.com/itsLeonB/ginkgo/pkg/response" "github.com/itsLeonB/ungerr" "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/codes" )func Handler(handlerName string, successCode int, handler func(ctx *gin.Context) (any, error)) gin.HandlerFunc { tracer := otel.GetTracerProvider().Tracer(packageName) return func(ctx *gin.Context) { c, span := tracer.Start(ctx.Request.Context(), handlerName) ctx.Request = ctx.Request.WithContext(c) defer span.End() if resp, err := handler(ctx); err == nil { ctx.JSON(successCode, response.JSONResponse{Data: resp}) } else { + span.RecordError(err) + span.SetStatus(codes.Error, err.Error()) _ = ctx.Error(err) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/utils.go` around lines 100 - 112, In Handler, when the inner handler returns an error you should record it on the OpenTelemetry span: call span.RecordError(err) and set the span status to error (e.g., span.SetStatus(codes.Error, err.Error())) before calling ctx.Error(err); also add the required import for the codes package from go.opentelemetry.io/otel/codes. This ensures errors returned by Handler (function name: Handler, variables: tracer, span) are visible in traces.pkg/middleware/error.go (1)
50-54: Consider recording errors on the span for better trace visibility.The tracing integration is correctly implemented. Similar to the suggestion for
Handler, recording error details on the span would enrich trace data for observability.💡 Optional: Record errors on span
You could record the error when responding with an error status. For example, before
ctx.AbortWithStatusJSON:span.RecordError(err) span.SetStatus(codes.Error, "error handled")This would require importing
go.opentelemetry.io/otel/codes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/middleware/error.go` around lines 50 - 54, The span in errorMiddleware.handle should record error details before sending the error response: locate error handling in errorMiddleware.handle (where you call ctx.AbortWithStatusJSON) and, when an error is about to be returned, call span.RecordError(err) and span.SetStatus(codes.Error, "error handled") (import go.opentelemetry.io/otel/codes), using the actual error (e.g., the error you pass to AbortWithStatusJSON or ctx.Errors.Last().Err) so the trace shows the recorded error and error status prior to ctx.AbortWithStatusJSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/middleware/error.go`:
- Around line 50-54: The span in errorMiddleware.handle should record error
details before sending the error response: locate error handling in
errorMiddleware.handle (where you call ctx.AbortWithStatusJSON) and, when an
error is about to be returned, call span.RecordError(err) and
span.SetStatus(codes.Error, "error handled") (import
go.opentelemetry.io/otel/codes), using the actual error (e.g., the error you
pass to AbortWithStatusJSON or ctx.Errors.Last().Err) so the trace shows the
recorded error and error status prior to ctx.AbortWithStatusJSON.
In `@pkg/server/utils.go`:
- Around line 100-112: In Handler, when the inner handler returns an error you
should record it on the OpenTelemetry span: call span.RecordError(err) and set
the span status to error (e.g., span.SetStatus(codes.Error, err.Error())) before
calling ctx.Error(err); also add the required import for the codes package from
go.opentelemetry.io/otel/codes. This ensures errors returned by Handler
(function name: Handler, variables: tracer, span) are visible in traces.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
example/handler.gogo.modpkg/middleware/const.gopkg/middleware/error-handling-convention.mdpkg/middleware/error.gopkg/server/const.gopkg/server/utils.gopkg/server/utils_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/middleware/error.go (1)
150-171:⚠️ Potential issue | 🟡 MinorRecord panic on the span before the early-return path.
When
ctx.Writer.Written()is true, the function returns at line 166 before reaching thespan.RecordErrorandspan.SetStatuscalls at lines 170–171, so panic is logged but not recorded in the trace.🔧 Proposed fix
func (em *errorMiddleware) handlePanic(r any, ctx *gin.Context, span trace.Span) { + panicErr := fmt.Errorf("panic recovered: %v", r) + span.RecordError(panicErr) + span.SetStatus(codes.Error, "panic recovered") + em.logger. WithContext(ctx.Request.Context()). WithFields(map[string]any{ "handler": ctx.HandlerName(), "panic.type": fmt.Sprintf("%T", r), "panic.value": fmt.Sprintf("%v", r), "stack_trace": string(debug.Stack()), }). Error("panic recovered") if ctx.Writer.Written() { em.logger. WithContext(ctx.Request.Context()). WithField("http.status_code", ctx.Writer.Status()). Error("response already written after panic, could not send error JSON") return } appError := ungerr.InternalServerError() - span.RecordError(appError) - span.SetStatus(codes.Error, fmt.Sprintf("panic: %v", r)) ctx.AbortWithStatusJSON(appError.HttpStatus(), appErrorToErrorObject(appError)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/middleware/error.go` around lines 150 - 171, In handlePanic, ensure the panic is recorded on the tracing span before any early return: create or obtain the appError (appError := ungerr.InternalServerError()), call span.RecordError(appError) and span.SetStatus(codes.Error, fmt.Sprintf("panic: %v", r)) immediately after logging the panic and before checking ctx.Writer.Written(); then keep the existing early-return behavior if the response was already written. This guarantees span.RecordError and span.SetStatus always run for the recovered panic.
🧹 Nitpick comments (1)
pkg/middleware/error.go (1)
75-76: Use low-cardinality span status descriptions instead of dynamic error text.
span.SetStatus(codes.Error, <dynamic message>)increases cardinality and can leak sensitive details. Per OpenTelemetry best practices, keep span status descriptions as predictable, constant "error bucket labels" (e.g., "application error", "validation failed"). Usespan.RecordError()(already in place) to capture detailed error information separately.
- Line 75: Replace
appError.Error()with"application error"- Line 86: Replace
cause.Error()with"wrapped error"- Line 88: Replace
appError.Error()with"identified error"(or similar)- Line 97: Replace
err.Error()with"unexpected error"- Line 171: Replace
fmt.Sprintf("panic: %v", r)with"panic recovered"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/middleware/error.go` around lines 75 - 76, Replace dynamic error text passed to span.SetStatus to use low-cardinality constant labels: change span.SetStatus(codes.Error, appError.Error()) to span.SetStatus(codes.Error, "application error"); change usages where cause.Error(), appError.Error(), and err.Error() are passed to span.SetStatus to use "wrapped error", "identified error" (or another stable bucket label), and "unexpected error" respectively; also replace fmt.Sprintf("panic: %v", r) used as a span status description with "panic recovered". Keep span.RecordError(...) calls as-is so full details are still recorded separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/middleware/error.go`:
- Around line 150-171: In handlePanic, ensure the panic is recorded on the
tracing span before any early return: create or obtain the appError (appError :=
ungerr.InternalServerError()), call span.RecordError(appError) and
span.SetStatus(codes.Error, fmt.Sprintf("panic: %v", r)) immediately after
logging the panic and before checking ctx.Writer.Written(); then keep the
existing early-return behavior if the response was already written. This
guarantees span.RecordError and span.SetStatus always run for the recovered
panic.
---
Nitpick comments:
In `@pkg/middleware/error.go`:
- Around line 75-76: Replace dynamic error text passed to span.SetStatus to use
low-cardinality constant labels: change span.SetStatus(codes.Error,
appError.Error()) to span.SetStatus(codes.Error, "application error"); change
usages where cause.Error(), appError.Error(), and err.Error() are passed to
span.SetStatus to use "wrapped error", "identified error" (or another stable
bucket label), and "unexpected error" respectively; also replace
fmt.Sprintf("panic: %v", r) used as a span status description with "panic
recovered". Keep span.RecordError(...) calls as-is so full details are still
recorded separately.
1 participant
Summary by CodeRabbit
New Features
Chores
Behavioral