Updating cron and calendar to slog method signatures. - #40446
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Migrates cron- and calendar-related logging call sites from go-kit/log/level style to the repository’s *logging.Logger slog-backed DebugContext/InfoContext/WarnContext/ErrorContext methods, aligning these areas with the ongoing slog migration work for #40054.
Changes:
- Replace go-kit
level.*(...).Log(...)usage with*logging.Loggercontext-aware methods across cron, calendar, and related services. - Thread
context.Contextinto a couple helper functions to support context-aware logging. - Remove now-unused go-kit
levelimports from updated files.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/calendar/calendar.go | Switch error logging to ErrorContext in calendar event body generation helpers. |
| server/service/apple_mdm_cmd_results.go | Convert warn/debug logs in VPP install verification results handler to *Context methods. |
| server/cron/calendar_cron.go | Update cron calendar logging to *Context methods and pass context into helper logging. |
| ee/server/service/teams.go | Convert informational/error logs to InfoContext/ErrorContext in team modification paths. |
| ee/server/service/orbit.go | Convert setup-experience related logs to *Context methods. |
| ee/server/service/digicert/digicert.go | Convert DigiCert debug/error logs to *Context methods. |
| ee/server/service/calendar.go | Convert calendar webhook/service logging to *Context methods. |
| ee/server/calendar/google_calendar.go | Convert Google Calendar implementation logs to *Context methods. |
| cmd/fleet/cron.go | Convert vulnerability/cron logs and helpers to *Context methods; adjust helper signature to accept context. |
Comments suppressed due to low confidence (1)
ee/server/calendar/google_calendar.go:272
withRetrylogs usingcontext.TODO(), which drops any trace/span correlation and request-scoped values available on the caller context. Please thread a real context intowithRetry(e.g., acceptctx context.Contextand pass through from the public methods) or store the configured context onGoogleCalendarLowLevelAPIinConfigureand use that for logging.
lowLevelAPI.logger.DebugContext(context.TODO(), "rate limited by Google calendar API", "err", err)
return err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR migrates logging across nine files from go-kit log/level package calls to context-aware logger methods. Changes replace Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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/cron/calendar_cron.go (1)
165-170:⚠️ Potential issue | 🟡 MinorDuplicate
team_idkey in structured log output.Line 128 already enriches the logger with
"team_id"vialogger = logger.With("team_id", team.ID). Passing"team_id", team.IDagain at line 166 will produce a duplicate key in the log entry.Proposed fix
logger.DebugContext(ctx, "summary", - "team_id", team.ID, "passing_hosts", len(passingHosts), "failing_hosts", len(failingHosts), "failing_hosts_without_associated_email", len(failingHostsWithoutAssociatedEmail), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cron/calendar_cron.go` around lines 165 - 170, The structured log call passes "team_id" twice: it was already added via logger = logger.With("team_id", team.ID), so remove the duplicate arguments from the logger.DebugContext call (the "team_id", team.ID pair) in the DebugContext invocation; keep the other fields (passing_hosts, failing_hosts, failing_hosts_without_associated_email) unchanged so the log still reports those metrics using logger.DebugContext.
🧹 Nitpick comments (2)
server/cron/calendar_cron.go (2)
94-94: Error logged at Info level — intentional?This logs a potentially actionable per-team error (
err) atInfoContext. If the original code also usedlevel.Info, this is just a faithful migration, but consider whetherErrorContextorWarnContextwould be more appropriate so that operators can filter on severity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cron/calendar_cron.go` at line 94, The current logging call uses logger.InfoContext to record per-team errors (logger.InfoContext(ctx, "events calendar cron", "team_id", team.ID, "err", err)); change this to an appropriate higher severity (e.g., logger.ErrorContext or logger.WarnContext) so actionable errors are surfaced correctly, and keep the same context fields ("events calendar cron", "team_id", team.ID) while including the err value; update any surrounding logic in the function that calls logger.InfoContext to use the new logger method.
738-740:fmt.Sprintfin the message arg is unnecessary with structured logging.With slog-style logging, the message should be a static string and dynamic data should go into key-value pairs. The
domainvalue is already available as a field on the logger or can be passed as a separate key.Proposed fix
- logger.DebugContext(ctx, fmt.Sprintf("no %s Google account associated with the hosts", domain), - "host_ids", fmt.Sprintf("%+v", hostIDs), + logger.DebugContext(ctx, "no Google account associated with the hosts", + "domain", domain, + "host_ids", hostIDs, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cron/calendar_cron.go` around lines 738 - 740, The log call uses fmt.Sprintf for the message; change the DebugContext call so the message is a static string and pass domain and hostIDs as separate key-value fields: update the call to logger.DebugContext(ctx, "no Google account associated with the hosts", "domain", domain, "host_ids", hostIDs) (referencing DebugContext, the domain variable and hostIDs) so structured logging carries dynamic data instead of embedding it in the message.
🤖 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/cron/calendar_cron.go`:
- Around line 165-170: The structured log call passes "team_id" twice: it was
already added via logger = logger.With("team_id", team.ID), so remove the
duplicate arguments from the logger.DebugContext call (the "team_id", team.ID
pair) in the DebugContext invocation; keep the other fields (passing_hosts,
failing_hosts, failing_hosts_without_associated_email) unchanged so the log
still reports those metrics using logger.DebugContext.
---
Nitpick comments:
In `@server/cron/calendar_cron.go`:
- Line 94: The current logging call uses logger.InfoContext to record per-team
errors (logger.InfoContext(ctx, "events calendar cron", "team_id", team.ID,
"err", err)); change this to an appropriate higher severity (e.g.,
logger.ErrorContext or logger.WarnContext) so actionable errors are surfaced
correctly, and keep the same context fields ("events calendar cron", "team_id",
team.ID) while including the err value; update any surrounding logic in the
function that calls logger.InfoContext to use the new logger method.
- Around line 738-740: The log call uses fmt.Sprintf for the message; change the
DebugContext call so the message is a static string and pass domain and hostIDs
as separate key-value fields: update the call to logger.DebugContext(ctx, "no
Google account associated with the hosts", "domain", domain, "host_ids",
hostIDs) (referencing DebugContext, the domain variable and hostIDs) so
structured logging carries dynamic data instead of embedding it in the message.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/fleet/cron.goee/server/calendar/google_calendar.goee/server/service/calendar.goee/server/service/digicert/digicert.goee/server/service/orbit.goee/server/service/teams.goserver/cron/calendar_cron.goserver/service/apple_mdm_cmd_results.goserver/service/calendar/calendar.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40446 +/- ##
==========================================
+ Coverage 66.26% 66.29% +0.03%
==========================================
Files 2460 2461 +1
Lines 197220 197395 +175
Branches 8716 8702 -14
==========================================
+ Hits 130682 130860 +178
+ Misses 54713 54685 -28
- Partials 11825 11850 +25
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:
|
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
once again my frail human orbs have detected no issues 👁️
Related issue: Resolves #40054
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit