Migrated logging and google calendar files to use slog - #40541
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR continues the ongoing slog migration (part 3 of issue #40540), focusing on migrating the logging infrastructure in the server/logging package and its callers from go-kit/log to the standard library's slog package.
Changes:
- Migrated all log writers in
server/loggingpackage to use*slog.Loggerinstead of*platformlogging.Logger - Added
context.Contextparameter toNewPubSubLogWriterandNewNatsLogWriterfor client initialization - Updated all logging calls from
level.Debug/Info/Error(logger).Log(...)tologger.DebugContext/InfoContext/ErrorContext(ctx, ...)
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/logging/filesystem.go | Updated NewFilesystemLogWriter to accept *slog.Logger; migrated logging calls to slog |
| server/logging/filesystem_test.go | Updated test logger initialization to use slog.New(slog.DiscardHandler) |
| server/logging/firehose.go | Migrated firehoseLogWriter struct and functions to use *slog.Logger |
| server/logging/firehose_test.go | Updated test logger initialization to use slog.New(slog.DiscardHandler) |
| server/logging/kinesis.go | Migrated kinesisLogWriter struct and functions to use *slog.Logger |
| server/logging/kinesis_test.go | Updated test logger initialization to use slog.New(slog.DiscardHandler) |
| server/logging/lambda.go | Migrated lambdaLogWriter struct and functions to use *slog.Logger |
| server/logging/lambda_test.go | Updated test logger initialization to use slog.New(slog.DiscardHandler) |
| server/logging/logging.go | Updated NewJSONLogger to accept context.Context and *slog.Logger; passed context to PubSub and NATS constructors |
| server/logging/nats.go | Added context.Context parameter to NewNatsLogWriter; migrated logging calls to slog |
| server/logging/nats_test.go | Updated all test calls to pass context and use slog.New(slog.DiscardHandler) |
| server/logging/pubsub.go | Added context.Context parameter to NewPubSubLogWriter; migrated logging calls to slog; removed context.Background() usage |
| server/logging/webhook.go | Migrated webhookLogWriter struct and functions to use *slog.Logger |
| server/logging/webhook_test.go | Updated test logger initialization to use slog.New(slog.DiscardHandler) |
| server/service/testing_utils.go | Updated call to NewFilesystemLogWriter to use logger.SlogLogger() adapter |
| cmd/fleet/serve.go | Updated all NewJSONLogger calls to pass cmd.Context() and use logger.SlogLogger() adapter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR continues the migration from platform/go-kit logging to the standard library slog across the codebase. It changes logger types from the project-specific logger to *slog.Logger, adds context.Context parameters to several logging constructors (e.g., NewJSONLogger, NewNatsLogWriter, NewPubSubLogWriter), and replaces level-based calls with context-aware slog methods (InfoContext, DebugContext, ErrorContext). Tests and call sites (including cmd/fleet/serve.go and server/service/testing_utils.go) are updated to pass logger.SlogLogger() and the required context where applicable. No control-flow or error-handling logic was altered. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/logging/pubsub.go (1)
56-57:⚠️ Potential issue | 🔴 CriticalAvoid nil-pointer panic when oversized logs are dropped.
On Line 56,
resultsis sized tolen(logs), but oversized entriescontinueon Line 85 without assigningresults[i]. Then Line 98 callsresult.Get(ctx)on nil and panics. Buildresultswithappendonly for published messages.🔧 Proposed fix
func (w *pubSubLogWriter) Write(ctx context.Context, logs []json.RawMessage) error { - results := make([]*pubsub.PublishResult, len(logs)) + results := make([]*pubsub.PublishResult, 0, len(logs)) // Add all of the messages to the global pubsub queue - for i, log := range logs { + for _, log := range logs { data, err := log.MarshalJSON() if err != nil { return ctxerr.Wrap(ctx, err, "marshal message into JSON") @@ message := &pubsub.Message{ Data: data, Attributes: attributes, } - results[i] = w.topic.Publish(ctx, message) + results = append(results, w.topic.Publish(ctx, message)) }Also applies to: 80-85, 97-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/logging/pubsub.go` around lines 56 - 57, The slice results is pre-sized to len(logs) and later some log entries are skipped (continue), leaving nil entries which cause a nil-pointer panic when calling Get on them; change the code that creates and populates results so you only append a PublishResult when you actually call pubsub.Publish (i.e., replace indexed assignment results[i] = ... with results = append(results, res)), and when iterating results call Get on every element (or check for nil before calling) to avoid dereferencing nil; update any loops that assumed results length equals len(logs) (e.g., the publish loop and the subsequent Get loop) to use the new results slice.
🧹 Nitpick comments (1)
server/logging/filesystem.go (1)
56-63:context.TODO()in the SIGHUP goroutine is acceptable but could be improved.Since the goroutine runs for the lifetime of the process with no cancellation path,
context.TODO()is correct here. A minor improvement would be to thread in a context fromNewFilesystemLogWriterand use it for both log calls and goroutine lifetime, but that is non-trivial and not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/logging/filesystem.go` around lines 56 - 63, The goroutine handling SIGHUP currently uses context.TODO() for appLogger.ErrorContext and has no cancellation; update NewFilesystemLogWriter to accept a context (or add a context field to the filesystem log writer struct), store it on the fs writer, and replace context.TODO() with that context when calling appLogger.ErrorContext; also use that stored ctx in the goroutine loop (select on ctx.Done() and <-sig) so the goroutine can exit if the parent context is cancelled while still reacting to sig and calling fsLogger.Rotate() and appLogger.ErrorContext as before (referencing NewFilesystemLogWriter, fsLogger.Rotate, appLogger.ErrorContext, and sig).
🤖 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 `@server/logging/pubsub.go`:
- Around line 56-57: The slice results is pre-sized to len(logs) and later some
log entries are skipped (continue), leaving nil entries which cause a
nil-pointer panic when calling Get on them; change the code that creates and
populates results so you only append a PublishResult when you actually call
pubsub.Publish (i.e., replace indexed assignment results[i] = ... with results =
append(results, res)), and when iterating results call Get on every element (or
check for nil before calling) to avoid dereferencing nil; update any loops that
assumed results length equals len(logs) (e.g., the publish loop and the
subsequent Get loop) to use the new results slice.
---
Nitpick comments:
In `@server/logging/filesystem.go`:
- Around line 56-63: The goroutine handling SIGHUP currently uses context.TODO()
for appLogger.ErrorContext and has no cancellation; update
NewFilesystemLogWriter to accept a context (or add a context field to the
filesystem log writer struct), store it on the fs writer, and replace
context.TODO() with that context when calling appLogger.ErrorContext; also use
that stored ctx in the goroutine loop (select on ctx.Done() and <-sig) so the
goroutine can exit if the parent context is cancelled while still reacting to
sig and calling fsLogger.Rotate() and appLogger.ErrorContext as before
(referencing NewFilesystemLogWriter, fsLogger.Rotate, appLogger.ErrorContext,
and sig).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/fleet/serve.goserver/logging/filesystem.goserver/logging/filesystem_test.goserver/logging/firehose.goserver/logging/firehose_test.goserver/logging/kinesis.goserver/logging/kinesis_test.goserver/logging/lambda.goserver/logging/lambda_test.goserver/logging/logging.goserver/logging/nats.goserver/logging/nats_test.goserver/logging/pubsub.goserver/logging/webhook.goserver/logging/webhook_test.goserver/service/testing_utils.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40541 +/- ##
==========================================
+ Coverage 66.29% 66.30% +0.01%
==========================================
Files 2466 2466
Lines 197559 197514 -45
Branches 8764 8742 -22
==========================================
- Hits 130964 130963 -1
+ Misses 54748 54704 -44
Partials 11847 11847
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
getvictor
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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)
server/logging/pubsub.go (1)
80-85:⚠️ Potential issue | 🔴 CriticalPotential panic when dropping oversized Pub/Sub logs.
At Line 83,
string(log[:100])can panic if payload is shorter than 100 bytes (the limit at Line 80 can be exceeded by attributes alone). Also, thecontinueat Line 85 leaves nil slots inresults, and Line 98 can then dereference nil viaresult.Get(ctx).🐛 Proposed fix
func (w *pubSubLogWriter) Write(ctx context.Context, logs []json.RawMessage) error { - results := make([]*pubsub.PublishResult, len(logs)) + results := make([]*pubsub.PublishResult, 0, len(logs)) // Add all of the messages to the global pubsub queue - for i, log := range logs { + for _, log := range logs { data, err := log.MarshalJSON() if err != nil { return ctxerr.Wrap(ctx, err, "marshal message into JSON") } @@ if len(data)+estimateAttributeSize(attributes) > pubsub.MaxPublishRequestBytes { + previewLen := len(log) + if previewLen > 100 { + previewLen = 100 + } w.logger.InfoContext(ctx, "dropping log over 10MB PubSub limit", "size", len(data), - "log", string(log[:100])+"...", + "log", string(log[:previewLen])+"...", ) continue } @@ - results[i] = w.topic.Publish(ctx, message) + results = append(results, w.topic.Publish(ctx, message)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/logging/pubsub.go` around lines 80 - 85, The code can panic when building the preview string and leaves nil entries in results causing a nil deref at result.Get(ctx); fix by (1) safely slicing the log preview (e.g., use preview := string(log[:min(100, len(log))])) before calling w.logger.InfoContext and (2) instead of continue, set results[i] to a completed failed publish result so later result.Get(ctx) is safe (implement a small helper like failedPublishResult(err) that returns a PublishResult whose Get(ctx) returns the error and assign results[i] = failedPublishResult(fmt.Errorf("dropped oversized log"))) so the loop never leaves nil in results. Ensure you reference the same variables: data, attributes, log, results, and result.Get(ctx).
🤖 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 `@server/logging/pubsub.go`:
- Around line 80-85: The code can panic when building the preview string and
leaves nil entries in results causing a nil deref at result.Get(ctx); fix by (1)
safely slicing the log preview (e.g., use preview := string(log[:min(100,
len(log))])) before calling w.logger.InfoContext and (2) instead of continue,
set results[i] to a completed failed publish result so later result.Get(ctx) is
safe (implement a small helper like failedPublishResult(err) that returns a
PublishResult whose Get(ctx) returns the error and assign results[i] =
failedPublishResult(fmt.Errorf("dropped oversized log"))) so the loop never
leaves nil in results. Ensure you reference the same variables: data,
attributes, log, results, and result.Get(ctx).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
cmd/fleet/serve.goee/server/calendar/google_calendar.goee/server/calendar/google_calendar_integration_test.goee/server/calendar/google_calendar_load.goee/server/calendar/google_calendar_mock.goee/server/calendar/google_calendar_test.goee/server/service/calendar.goserver/cron/calendar_cron.goserver/logging/filesystem.goserver/logging/filesystem_test.goserver/logging/firehose.goserver/logging/firehose_test.goserver/logging/kinesis.goserver/logging/kinesis_test.goserver/logging/lambda.goserver/logging/lambda_test.goserver/logging/logging.goserver/logging/nats.goserver/logging/nats_test.goserver/logging/pubsub.goserver/logging/webhook.goserver/logging/webhook_test.goserver/service/calendar/calendar.goserver/service/testing_utils.go
Resolved conflict in server/logging/webhook.go where PostJSONWithTimeout gained a logger parameter on main.
mostlikelee
left a comment
mostlikelee
left a comment
There was a problem hiding this comment.
Looks good, just one comment around a TODO context
| <-sig // block on signal | ||
| if err := fsLogger.Rotate(); err != nil { | ||
| appLogger.Log("err", err) | ||
| appLogger.ErrorContext(context.TODO(), "log rotation error", "err", err) |
There was a problem hiding this comment.
do we need to pass a context into the parent func signature?
There was a problem hiding this comment.
Yes, that's an easy change. I'll make it.
Related issue: Resolves #40540
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Refactor
Tests