refactor: migrate logging to Go slog#1552
Conversation
Replace go.uber.org/zap and github.com/raystack/salt/log with log/slog across the entire codebase. This removes external logging dependencies in favor of Go's built-in structured logging introduced in Go 1.21. Key changes: - Rewrite pkg/logger with slog-based InitLogger, ToContext, FromContext - Replace salt log.Logger interface with *slog.Logger throughout - Replace grpczap context propagation with custom context helpers - Convert all zap typed fields (zap.String, zap.Error) to key-value pairs - Update Connect RPC logger interceptor to use *slog.Logger - Update all tests to use slog with io.Discard handler - Remove go.uber.org/zap and grpc-zap-middleware dependencies
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 17 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR migrates the codebase from Zap/salt/log/grpczap logging to Go's standard library structured logger ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Fatal inside a function that returns error is redundant and prevents callers from handling the error gracefully. Keep Fatal only in the shutdown goroutine where returning is not possible.
Replace the zap-inherited pattern of storing/extracting a *slog.Logger from context with the idiomatic slog approach: - Add ContextHandler that auto-appends request-scoped attrs from context - Add AppendCtx helper to store slog.Attr in context - Interceptor stores request_id/method as context attrs, not a logger - API handlers use slog.ErrorContext(ctx) with default logger - Billing services get explicit *slog.Logger struct field - Remove ToContext/FromContext entirely This follows the Go slog design principle: loggers are explicit dependencies (struct fields), request-scoped data flows through context via a custom handler.
All s.log.Error/Warn/Debug/Info calls in functions that have a ctx parameter now use the *Context variant (e.g., ErrorContext(ctx, ...)). This ensures request-scoped attributes (request_id, method) from the ContextHandler appear in every log line. 47 calls updated across core/authenticate, core/membership, core/domain, core/userpat, internal/store/postgres, and internal/store/blob.
Replace fmt.Sprintf("msg: %v", err) with proper slog key-value pairs
("error", err) in authenticators.go. This keeps errors as structured
fields that are queryable in log aggregation tools.
Copy the existing attrs slice before appending to avoid mutating the parent context's backing array when multiple goroutines fork from the same context and both call AppendCtx.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
core/webhook/service.go (1)
111-153:⚠️ Potential issue | 🟡 MinorDetached goroutine logs against request ctx.
The goroutine outlives the request and uses the parent
ctxforslog.ErrorContext. Once the request returns andctxis cancelled, logs still emit fine (slog doesn't check cancellation), and context values (e.g., request_id extracted byContextHandler) remain accessible — so this is functionally OK. Just flagging to confirm this matches the intended request-scoped attribute propagation pattern described in the PR, especially since the DB call intentionally uses adetachedRepoContext. If you want request-scoped attrs on these logs, the current ctx is correct; if not, consider usingdetachedRepoContextconsistently.Also minor: key
"errs"at line 152 serializes a[]error— slog's default JSON encoder will stringify viafmt. Considererrors.Join(errs...).Error()or logging count + first error for cleaner structured output.internal/api/v1beta1connect/session.go (1)
106-119:⚠️ Potential issue | 🟠 MajorRedact or hash session IDs before logging.
session_idis session-control data; writing the raw value to logs increases blast radius if logs are exposed. Keep correlation by logging a deterministic hash or short redacted token instead.🛡️ Proposed direction
errorLogger.LogServiceError(ctx, request, "RevokeSession.GetByID", err, - "session_id", sessionID.String()) + "session_id_hash", hashLogID(sessionID.String()))Apply the same pattern to the
PingUserSession,PingUserSession.GetByID, andRevokeUserSessionlog fields.Also applies to: 142-153, 211-214
internal/api/v1beta1connect/user.go (1)
138-150:⚠️ Potential issue | 🟠 MajorSanitize emails and user names before logging.
These changed slog arguments persist raw emails/user names in error logs. Prefer user IDs already present in the log context, boolean presence flags, or hashed identifiers.
🛡️ Proposed sanitization pattern
errorLogger.LogServiceError(ctx, request, "CreateUser", err, - "email", email, - "name", name) + "email_present", email != "", + "name_present", name != "") @@ errorLogger.LogUnexpectedError(ctx, request, "CreateUser", err, - "user_email", email, - "user_name", name) + "email_present", email != "", + "name_present", name != "") @@ errorLogger.LogServiceError(ctx, request, "UpdateUser", err, "user_id", id, - "email", email) + "email_present", email != "") @@ errorLogger.LogUnexpectedError(ctx, request, "UpdateUser", err, "user_id", id, - "user_email", email) + "email_present", email != "") @@ errorLogger.LogUnexpectedError(ctx, request, "ListOrganizationsByUser", err, "user_id", userID, - "user_email", userData.Email) + "user_email_present", userData.Email != "") @@ errorLogger.LogUnexpectedError(ctx, request, "ListOrganizationsByCurrentUser", err, "principal_id", principal.ID, "principal_type", principal.Type, - "user_email", principal.User.Email) + "user_email_present", principal.User.Email != "")Also applies to: 286-300, 747-749, 812-815
core/authenticate/service.go (1)
111-116:⚠️ Potential issue | 🟡 MinorGuard against nil logger injection.
Servicenow dereferencess.login runtime paths; a nil constructor argument will panic. Default it once inNewService.🛡️ Proposed nil-safe fallback
func NewService(logger *slog.Logger, config Config, flowRepo FlowRepository, mailDialer mailer.Dialer, tokenService TokenService, sessionService SessionService, userService UserService, serviceUserService ServiceUserService, webAuthConfig *webauthn.WebAuthn, userPATService UserPATService) *Service { + if logger == nil { + logger = slog.Default() + } r := &Service{ log: logger,core/userpat/alert_service.go (1)
81-99:⚠️ Potential issue | 🟡 MinorDefault nil loggers at construction.
Run,sendExpiryReminders,sendExpiredNotices, andsendAlertall dereferences.logger; passingnilwill panic when alerts execute.🛡️ Proposed nil-safe fallback
func NewAlertService( repo AlertRepository, userSvc AlertUserService, orgSvc AlertOrgService, @@ logger *slog.Logger, auditRepo AlertAuditRepository, ) *AlertService { + if logger == nil { + logger = slog.Default() + } return &AlertService{pkg/server/connect_interceptors/logger.go (1)
48-75:⚠️ Potential issue | 🟡 MinorUse
connect.CodeOffor proper error code extraction and remove duplicate request-scoped attributes.Lines 57–60 use
connect.Code(0)(an invalid code) and direct type assertion to extract error codes, which misses wrapped*connect.Errorvalues. Additionally, lines 65 and 68 explicitly logrequest_idandmethod, duplicating the context attributes already set on lines 48–51. When usingContextHandler, these fields are automatically included from context, making them redundant.Refactor to use
connect.CodeOffor proper error code extraction (which handles wrapped errors), map non-Connect errors likecontext.Canceledto appropriate codes, and remove explicitmethodandrequest_idfrom theattrsslice.Suggested patch
import ( "context" + "errors" "log/slog" "time" @@ - code := connect.Code(0) - if connectErr, ok := err.(*connect.Error); ok { - code = connectErr.Code() - } + code := "ok" + level := slog.LevelInfo + if err != nil { + connectCode := codeForError(err) + code = connectCode.String() + level = levelForCode(connectCode) + } attrs := []any{ "system", "connect_rpc", "start_time", startTime, - "method", req.Spec().Procedure, "time_ms", duration.Milliseconds(), - "code", code.String(), - "request_id", requestID, + "code", code, } if err != nil { attrs = append(attrs, "error", err) } - level := levelForCode(code, err) logger.Log(ctx, level, "finished call", attrs...) @@ -func levelForCode(code connect.Code, err error) slog.Level { - if err == nil { - return slog.LevelInfo - } +func codeForError(err error) connect.Code { + switch { + case errors.Is(err, context.Canceled): + return connect.CodeCanceled + case errors.Is(err, context.DeadlineExceeded): + return connect.CodeDeadlineExceeded + default: + return connect.CodeOf(err) + } +} + +func levelForCode(code connect.Code) slog.Level { switch code { case connect.CodeCanceled, connect.CodeDeadlineExceeded,
🧹 Nitpick comments (6)
core/invitation/service.go (1)
121-121: Consider injecting*slog.Loggerinto the Service for consistency.
slog.SetDefault()is initialized at startup (cmd/server.go:86), so the package-level logger will have yourContextHandler. However, other services in the codebase (e.g.,core/userpat/,core/membership/,core/authenticate/) wire the logger directly to improve consistency and avoid relying on global state. The logger is already available at construction time; consider adding it to the Service struct for deterministic behavior aligned with the rest of the codebase.core/authenticate/authenticators.go (1)
41-242: Inconsistent error attribute key:"error"vs"err".Within this file, lines 41 and 51 use
"error"while lines 69, 76, 117, 125, 139, 148, 161, 192, 207, 226, 242 use"err". Other migrated files in this PR (e.g.,core/webhook/service.go) standardize on"error". Pick one key project-wide so log consumers/dashboards can filter reliably —slogalso has a dedicatedslog.Any("error", err)convention worth adopting.Proposed fix
- s.log.DebugContext(ctx, "PAT validation failed", "err", err) + s.log.DebugContext(ctx, "PAT validation failed", "error", err)(apply similarly to all
"err"occurrences in this file)internal/api/v1beta1connect/user_orgs.go (1)
38-44: Consider: redundant logging on theErrBadInputpath.On Line 38 a service error is logged for
postgres.ErrBadInput, which is then returned to the client asCodeInvalidArgument. Caller-induced bad-input errors typically don't warrant error-level server logs (they'll generate noise for every malformed client request). Other handlers in this PR (e.g.organization_tokens.go,organization_serviceuser.go) only log in the unexpected-error branch for the same condition. Consider dropping the log on Line 38-39 or downgrading it, for consistency.internal/api/v1beta1connect/relation.go (1)
103-119: Avoid double-logging unexpected relation errors.These paths call
LogServiceErrorbefore theswitch, then callLogUnexpectedErroragain in thedefaultbranch with the same attrs. Pick one log call per error path to avoid duplicate error records and noisy alerting.♻️ Proposed shape
if err != nil { - errorLogger.LogServiceError(ctx, request, "CreateRelation", err, - "subject", request.Msg.GetBody().GetSubject(), - "object", request.Msg.GetBody().GetObject(), - "relation", request.Msg.GetBody().GetRelation(), - "subject_sub_relation", request.Msg.GetBody().GetSubjectSubRelation()) - switch { case errors.Is(err, relation.ErrInvalidDetail): return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) default: errorLogger.LogUnexpectedError(ctx, request, "CreateRelation", err,Apply the same pattern in
GetRelationandDeleteRelation.Also applies to: 138-150, 187-202
core/event/listener.go (1)
5-6: Keep the failure reason structured.Line 42 stringifies the wrapped error into the log message, so downstream log queries cannot filter on a stable
errorfield. Prefer a constant message with the original error as an attribute.Proposed cleanup
import ( "context" - "fmt" "log/slog" "github.com/raystack/frontier/core/audit" ) @@ case audit.OrgCreatedEvent.String(): if err := l.processor.EnsureDefaultPlan(ctx, log.OrgID); err != nil { - slog.ErrorContext(ctx, fmt.Errorf("EnsureDefaultPlan: %w", err).Error(), "event", log.Action) + slog.ErrorContext(ctx, "failed to ensure default plan", "error", err, "event", log.Action) } } }Also applies to: 41-42
billing/customer/service_test.go (1)
6-6: Use a discard logger in tests instead of the process-global default.These tests should not depend on or write through
slog.Default(). A local discard logger keeps the migration behavior deterministic and avoids noisy test output.Proposed test logger helper
import ( "context" "errors" + "io" "log/slog" "testing" @@ var sampleError = errors.New("sample error") + +func testLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} @@ - return customer.NewService(slog.Default(), stripeClient, mockRepo, cfg, mockCredit) + return customer.NewService(testLogger(), stripeClient, mockRepo, cfg, mockCredit)Apply the same
testLogger()replacement to the othercustomer.NewService(...)calls in this file.Also applies to: 76-76, 108-108, 172-172, 265-265, 288-288, 336-336, 391-391, 408-408, 475-475, 492-492
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a83a8978-ba79-49f0-940a-498c1b92c895
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (113)
billing/checkout/service.gobilling/customer/service.gobilling/customer/service_test.gobilling/invoice/service.gobilling/subscription/service.gobilling/subscription/service_test.gocmd/migrate.gocmd/serve.gocmd/server.gocore/authenticate/authenticators.gocore/authenticate/service.gocore/authenticate/service_test.gocore/authenticate/session/service.gocore/authenticate/session/service_test.gocore/domain/service.gocore/event/listener.gocore/event/service.gocore/invitation/service.gocore/membership/service.gocore/membership/service_test.gocore/userpat/alert_service.gocore/userpat/alert_service_test.gocore/userpat/service.gocore/userpat/service_test.gocore/userpat/validator.gocore/userpat/validator_test.gocore/webhook/service.gogo.modinternal/api/v1beta1connect/audit_record.gointernal/api/v1beta1connect/authenticate.gointernal/api/v1beta1connect/authenticate_test.gointernal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/billing_check.gointernal/api/v1beta1connect/billing_checkout.gointernal/api/v1beta1connect/billing_customer.gointernal/api/v1beta1connect/billing_invoice.gointernal/api/v1beta1connect/billing_plan.gointernal/api/v1beta1connect/billing_product.gointernal/api/v1beta1connect/billing_subscription.gointernal/api/v1beta1connect/billing_usage.gointernal/api/v1beta1connect/billing_webhook.gointernal/api/v1beta1connect/deleter.gointernal/api/v1beta1connect/domain.gointernal/api/v1beta1connect/error_handler.gointernal/api/v1beta1connect/group.gointernal/api/v1beta1connect/invitations.gointernal/api/v1beta1connect/kyc.gointernal/api/v1beta1connect/metaschema.gointernal/api/v1beta1connect/namespace.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_invoices.gointernal/api/v1beta1connect/organization_pats.gointernal/api/v1beta1connect/organization_projects.gointernal/api/v1beta1connect/organization_serviceuser.gointernal/api/v1beta1connect/organization_serviceuser_credentials.gointernal/api/v1beta1connect/organization_tokens.gointernal/api/v1beta1connect/organization_users.gointernal/api/v1beta1connect/permission.gointernal/api/v1beta1connect/permission_check.gointernal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/policy.gointernal/api/v1beta1connect/preferences.gointernal/api/v1beta1connect/project.gointernal/api/v1beta1connect/project_users.gointernal/api/v1beta1connect/prospect.gointernal/api/v1beta1connect/relation.gointernal/api/v1beta1connect/resource.gointernal/api/v1beta1connect/role.gointernal/api/v1beta1connect/serviceuser.gointernal/api/v1beta1connect/session.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_orgs.gointernal/api/v1beta1connect/user_pat.gointernal/api/v1beta1connect/user_projects.gointernal/api/v1beta1connect/v1beta1connect.gointernal/api/v1beta1connect/webhook.gointernal/bootstrap/service.gointernal/store/blob/resources_repository.gointernal/store/postgres/audit_record_repository_test.gointernal/store/postgres/billing_customer_repository_test.gointernal/store/postgres/billing_product_repository_test.gointernal/store/postgres/domain_repository.gointernal/store/postgres/flow_repository.gointernal/store/postgres/group_repository_test.gointernal/store/postgres/invitation_repository.gointernal/store/postgres/invitation_repository_test.gointernal/store/postgres/kyc_repository_test.gointernal/store/postgres/lock_test.gointernal/store/postgres/metaschema_repository.gointernal/store/postgres/namespace_repository_test.gointernal/store/postgres/organization_repository_test.gointernal/store/postgres/permission_repository_test.gointernal/store/postgres/policy_repository_test.gointernal/store/postgres/postgres_test.gointernal/store/postgres/preference_repository_test.gointernal/store/postgres/project_repository_test.gointernal/store/postgres/prospect_repository_test.gointernal/store/postgres/relation_repository_test.gointernal/store/postgres/resource_repository_test.gointernal/store/postgres/role_repository_test.gointernal/store/postgres/session_repository.gointernal/store/postgres/user_repository_test.gointernal/store/postgres/userpat_repository_test.gointernal/store/postgres/webhook_endpoint_repository_test.gointernal/store/spicedb/relation_repository.gointernal/store/spicedb/schema_repository.gointernal/store/spicedb/spicedb.gopkg/logger/logger.gopkg/server/connect_interceptors/logger.gopkg/server/server.gotest/e2e/testbench/frontier.gotest/e2e/testbench/spicedb.gotest/e2e/testbench/stripe.go
💤 Files with no reviewable changes (1)
- internal/api/v1beta1connect/v1beta1connect.go
| errorLogger.LogServiceError(ctx, request, "CreateProjectResource", err, | ||
| zap.String("resource_id", request.Msg.GetId()), | ||
| zap.String("project_id", request.Msg.GetProjectId()), | ||
| zap.String("resource_name", request.Msg.GetBody().GetName()), | ||
| zap.String("namespace", request.Msg.GetBody().GetNamespace()), | ||
| zap.String("principal", request.Msg.GetBody().GetPrincipal())) | ||
| "resource_id", request.Msg.GetId(), | ||
| "project_id", request.Msg.GetProjectId(), | ||
| "resource_name", request.Msg.GetBody().GetName(), | ||
| "namespace", request.Msg.GetBody().GetNamespace(), | ||
| "principal", request.Msg.GetBody().GetPrincipal()) |
There was a problem hiding this comment.
Avoid logging raw principals from resource requests.
principal can contain a user identifier such as an email or subject ID. Prefer logging non-identifying context like principal_type plus presence, or a hashed value if correlation is required.
🛡️ Proposed sanitization
"resource_id", request.Msg.GetId(),
"project_id", request.Msg.GetProjectId(),
"resource_name", request.Msg.GetBody().GetName(),
"namespace", request.Msg.GetBody().GetNamespace(),
- "principal", request.Msg.GetBody().GetPrincipal())
+ "principal_type", principalType,
+ "principal_present", request.Msg.GetBody().GetPrincipal() != "")Apply the same replacement in both CreateProjectResource and UpdateProjectResource service/unexpected error logs.
Also applies to: 130-135, 224-229, 242-247
There was a problem hiding this comment.
@rohilsurana Shall we solve PII is this PR itself or new one?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Pull request overview
Migrates the codebase from zap/salt/log/grpc-zap middleware to Go’s standard log/slog, adding context-aware request-scoped attributes via a custom handler and updating call sites accordingly.
Changes:
- Introduces
pkg/loggerslog infrastructure (ContextHandler,AppendCtx,InitLogger,Fatal) to standardize structured logging. - Updates ConnectRPC server/interceptors and many services/repositories/tests to accept/use
*slog.Loggerandslog.*ContextAPIs. - Removes direct zap logging dependencies and adjusts
go.mod/go.sumto reflect the new logging stack.
Reviewed changes
Copilot reviewed 113 out of 114 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/testbench/stripe.go | Updates testbench helpers to pass *slog.Logger into Stripe mock/client setup. |
| test/e2e/testbench/spicedb.go | Updates SpiceDB testbench startup to use *slog.Logger. |
| test/e2e/testbench/frontier.go | Updates E2E testbench server/migration helpers to accept *slog.Logger and use new Fatal. |
| pkg/server/server.go | Refactors UI/Connect server startup to use *slog.Logger, new Connect logging interceptor, and improved error returns. |
| pkg/server/connect_interceptors/logger.go | Replaces zap-based interceptor logging with slog + request-scoped context attributes. |
| pkg/logger/logger.go | Adds slog initialization, context attribute propagation handler, and a Fatal helper. |
| internal/store/spicedb/spicedb.go | Updates SpiceDB constructor signature to accept *slog.Logger. |
| internal/store/spicedb/schema_repository.go | Switches schema repository logger to *slog.Logger and uses slog debug gating. |
| internal/store/spicedb/relation_repository.go | Moves debug trace logging from grpc-zap to slog.InfoContext. |
| internal/store/postgres/webhook_endpoint_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/userpat_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/user_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/session_repository.go | Updates repository logger field to *slog.Logger and uses DebugContext. |
| internal/store/postgres/role_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/resource_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/relation_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/prospect_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/project_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/preference_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/postgres_test.go | Updates test DB helpers to use slog debug gating and remove logger writer dependency. |
| internal/store/postgres/policy_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/permission_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/organization_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/namespace_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/metaschema_repository.go | Updates repository logger field to *slog.Logger and uses DebugContext. |
| internal/store/postgres/lock_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/kyc_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/invitation_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/invitation_repository.go | Updates repository logger field to *slog.Logger and uses DebugContext. |
| internal/store/postgres/group_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/flow_repository.go | Updates repository logger field to *slog.Logger and uses DebugContext. |
| internal/store/postgres/domain_repository.go | Updates repository logger field to *slog.Logger and uses DebugContext. |
| internal/store/postgres/billing_product_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/billing_customer_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/postgres/audit_record_repository_test.go | Updates tests to construct discard slog logger instead of salt/zap logger. |
| internal/store/blob/resources_repository.go | Updates blob repository to use *slog.Logger and *Context log methods. |
| internal/bootstrap/service.go | Switches bootstrap logging to slog.*Context. |
| internal/api/v1beta1connect/webhook.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/v1beta1connect.go | Removes zap logger extraction helper and related context key usage. |
| internal/api/v1beta1connect/user_projects.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/user_pat.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/user_orgs.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/session.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/serviceuser.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/role.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/resource.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/relation.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/prospect.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/project_users.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/project.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/preferences.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/policy.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/platform.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/permission_check.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/permission.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/organization_users.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/organization_tokens.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/organization_serviceuser_credentials.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/organization_serviceuser.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/organization_projects.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/organization_pats.go | Switches limit override warning to slog.WarnContext. |
| internal/api/v1beta1connect/organization_invoices.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/namespace.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/metaschema.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/kyc.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/invitations.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/error_handler.go | Refactors centralized error logger to use slog key/value args. |
| internal/api/v1beta1connect/domain.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/deleter.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_webhook.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_usage.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_subscription.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_product.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_plan.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_invoice.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_customer.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_checkout.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/billing_check.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/authorize.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/authenticate_test.go | Removes zap logger context dependency in tests. |
| internal/api/v1beta1connect/authenticate.go | Replaces zap fields in error logging calls with slog key/value args. |
| internal/api/v1beta1connect/audit_record.go | Replaces zap fields in error logging calls with slog key/value args. |
| go.sum | Removes zap module sums consistent with dependency cleanup. |
| go.mod | Removes direct zap dependency and reclassifies grpc-middleware v1 as indirect. |
| core/webhook/service.go | Replaces grpc-zap logging with slog.*Context for webhook publishing errors. |
| core/userpat/validator_test.go | Updates tests to use discard slog logger instead of salt/log noop. |
| core/userpat/validator.go | Changes validator logger type to *slog.Logger. |
| core/userpat/service.go | Changes service logger type to *slog.Logger and updates to *Context methods. |
| core/userpat/alert_service_test.go | Updates tests to use discard slog logger instead of salt/log noop. |
| core/userpat/alert_service.go | Changes alert service logger type to *slog.Logger and replaces zap fields. |
| core/membership/service_test.go | Updates tests to use discard slog logger instead of salt/log noop. |
| core/membership/service.go | Changes membership logger type to *slog.Logger and updates to *Context methods. |
| core/invitation/service.go | Replaces logger.Ctx(ctx)/zap usage with slog.ErrorContext. |
| core/event/service.go | Replaces grpc-zap logging with slog.*Context for billing webhook processing. |
| core/event/listener.go | Replaces grpc-zap logging with slog.ErrorContext in audit event listener. |
| core/domain/service.go | Changes domain service logger type to *slog.Logger and updates to *Context methods. |
| core/authenticate/session/service_test.go | Updates tests to use discard slog logger instead of salt/log logrus. |
| core/authenticate/session/service.go | Changes session service logger type to *slog.Logger and uses WarnContext. |
| core/authenticate/service_test.go | Updates tests to use discard slog logger instead of salt/log logrus. |
| core/authenticate/service.go | Changes authenticate service logger type to *slog.Logger and uses *Context logging. |
| core/authenticate/authenticators.go | Replaces string-formatted debug logs with structured slog context logs. |
| cmd/server.go | Sets slog default logger during server startup. |
| cmd/serve.go | Propagates *slog.Logger across server setup and removes ctxzap context injection. |
| cmd/migrate.go | Updates migrations commands to accept *slog.Logger. |
| billing/subscription/service_test.go | Updates subscription tests to pass a slog logger into the service. |
| billing/subscription/service.go | Adds *slog.Logger to subscription service and replaces grpc-zap logging. |
| billing/invoice/service.go | Adds *slog.Logger to invoice service and replaces grpc-zap/zap logging. |
| billing/customer/service_test.go | Updates customer tests to pass a slog logger into the service. |
| billing/customer/service.go | Adds *slog.Logger to customer service and replaces grpc-zap logging. |
| billing/checkout/service.go | Adds *slog.Logger to checkout service and replaces grpc-zap logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Store request-scoped attrs in context for downstream loggers | ||
| requestID := req.Header().Get(consts.RequestIDHeader) | ||
| ctx = frontierlogger.AppendCtx(ctx, | ||
| slog.String("request_id", requestID), | ||
| slog.String("method", req.Spec().Procedure), | ||
| ) |
There was a problem hiding this comment.
UnaryConnectLoggerInterceptor appends request_id and method into the context via AppendCtx, but the log call later also includes "method" and "request_id" in attrs. With the ContextHandler this will emit duplicate attributes/keys on the same record. Consider removing these keys from attrs (or stop appending them to ctx for this interceptor) so each field appears only once per log line.
| func (r SchemaRepository) WriteSchema(ctx context.Context, schema string) error { | ||
| if r.logger.Level() == "debug" { | ||
| if r.logger.Enabled(ctx, slog.LevelDebug) { | ||
| fmt.Println(schema) | ||
| } |
There was a problem hiding this comment.
In debug mode WriteSchema prints the schema using fmt.Println, which bypasses the configured logger/handler (JSON vs text), output destination, and any request-scoped context attributes. Prefer logging via r.logger.DebugContext(...) (potentially with a dedicated attribute like schema) so output is structured and consistent.
| case audit.OrgCreatedEvent.String(): | ||
| if err := l.processor.EnsureDefaultPlan(ctx, log.OrgID); err != nil { | ||
| stdLogger.Error(fmt.Errorf("EnsureDefaultPlan: %w", err).Error()) | ||
| slog.ErrorContext(ctx, fmt.Errorf("EnsureDefaultPlan: %w", err).Error(), "event", log.Action) |
There was a problem hiding this comment.
This logs the error by embedding it into the message string (err.Error()), which loses the structured error field and makes log filtering harder. Prefer a stable message (e.g., "EnsureDefaultPlan failed") and add the wrapped error as an attribute (e.g., "error", err) along with event/org_id.
…ures Resolved conflicts in: - core/domain/service.go: added membershipService with slog logger - core/membership/service_test.go: added ProjectService/GroupService mocks with slog logger - internal/api/v1beta1connect/domain.go: adopted main's simplified JoinOrganization flow - internal/api/v1beta1connect/invitations.go: adopted main's simplified AcceptOrganizationInvitation flow - internal/api/v1beta1connect/organization.go: adopted main's RemoveOrganizationMember with slog-style logging - internal/api/v1beta1connect/user_pat.go: adopted main's mapPATError helper with slog-style logging
- Fix race condition in resources_repository.go: use local variable instead of repo.cached after mutex release - Fix nil err on passkey_type type assertion failure in authenticate.go - Add slog.SetDefault in migrate and migrate-rollback commands - Map "fatal" log level to slog.LevelError in parseLevel - Fix duplicate logging in SearchProjectUsers: log via LogServiceError only for BadInput errors, use LogUnexpectedError for the rest
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)
core/userpat/service.go (1)
619-621:⚠️ Potential issue | 🟡 MinorUse
ErrorContexthere for consistency with the slog migration.
createPATPolicyreceivesctxbut logs vias.logger.Error(...)instead ofErrorContext(ctx, ...). Per the PR's ContextHandler design, context-scoped attributes (request_id, method, etc.) are only appended when the*Contextvariants are used, so this call will drop request-scoped fields unlike the other migrated sites in this file (Delete, Regenerate, Update, Create, validateProjectAccess, enrichWithScope).Proposed fix
- s.logger.Error("failed to create PAT policy", - "pat_id", patID, "role_id", roleID, "resource_id", resourceID, - "resource_type", resourceType, "grant_relation", grantRelation, "error", err) + s.logger.ErrorContext(ctx, "failed to create PAT policy", + "pat_id", patID, "role_id", roleID, "resource_id", resourceID, + "resource_type", resourceType, "grant_relation", grantRelation, "error", err)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b195940e-0a52-47a3-aef9-295b6f77d90c
📒 Files selected for processing (12)
cmd/serve.gocore/domain/service.gocore/invitation/service.gocore/membership/service.gocore/membership/service_test.gocore/userpat/service.gocore/userpat/service_test.gointernal/api/v1beta1connect/authorize.gointernal/api/v1beta1connect/domain.gointernal/api/v1beta1connect/invitations.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/user_pat.go
✅ Files skipped from review due to trivial changes (4)
- internal/api/v1beta1connect/domain.go
- internal/api/v1beta1connect/user_pat.go
- internal/api/v1beta1connect/invitations.go
- core/userpat/service_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/invitation/service.go
- core/membership/service.go
- core/domain/service.go
- core/membership/service_test.go
- cmd/serve.go
Tests added by main still used the old log.NewNoop() logger. Replaced with slog.New(slog.NewTextHandler(io.Discard, nil)) to match the slog migration.
Merged latest main which added ServiceuserService to membership. Resolved conflicts keeping slog logger + new service parameter.
frontierlogger.Fatal calls os.Exit which bypasses deferred cleanup when invoked inside the graceful shutdown goroutine.
Summary
go.uber.org/zap,github.com/raystack/salt/log, andgrpc-zap-middlewarewith Go's standard librarylog/slogacross the entire codebase (114 files)ContextHandlerthat auto-appends request-scoped attributes from contextgo.modArchitecture
pkg/logger/— Core logging infrastructure:ContextHandlerwraps anyslog.Handler, extracts attrs stored in context viaAppendCtx, and appends them to every log recordAppendCtx(ctx, slog.String("key", val))stores attributes in context (not a logger)InitLogger(cfg)returns*slog.Loggerwith JSON or text handler, wrapped inContextHandlerFatalhelper forslog.Error+os.Exit(1)(slog has no Fatal level)Request-scoped logging flow:
Logging patterns used:
s.log.ErrorContext(ctx, "msg", "key", val)slog.ErrorContext(ctx, "msg", "key", val)logger.Info("msg", "key", val)(no ctx available)slog.New(slog.NewTextHandler(io.Discard, nil))Dependencies removed:
go.uber.org/zapgithub.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzapgithub.com/raystack/salt/log(salt itself stays for config/cli/rql)Test plan
go build ./...passesgo vet ./...passesgo mod tidycleangolangci-lint run ./...— 0 issueszap,salt/log, orctxzap*Context(ctx, ...)variants used when ctx is availablego test ./...(unit tests)