Add ability to enable/disable logs by topic - #40126
Conversation
| // Wrap slog's ErrorContext method. | ||
| func (a *Logger) ErrorContext(ctx context.Context, msg string, keyvals ...any) { | ||
| a.logger.ErrorContext(ctx, msg, keyvals...) | ||
| } | ||
|
|
||
| // Wrap slog's WarnContext method. | ||
| func (a *Logger) WarnContext(ctx context.Context, msg string, keyvals ...any) { | ||
| a.logger.WarnContext(ctx, msg, keyvals...) | ||
| } | ||
|
|
||
| // Wrap slog's InfoContext method. | ||
| func (a *Logger) InfoContext(ctx context.Context, msg string, keyvals ...any) { | ||
| a.logger.InfoContext(ctx, msg, keyvals...) | ||
| } | ||
|
|
||
| // Wrap slog's DebugContext method. | ||
| func (a *Logger) DebugContext(ctx context.Context, msg string, keyvals ...any) { | ||
| a.logger.DebugContext(ctx, msg, keyvals...) | ||
| } | ||
|
|
There was a problem hiding this comment.
I added these wrappers so that we can start using slog semantics before the full migration, e.g.
svc.logger.ErrorContext(ctx, "some log")
There was a problem hiding this comment.
Modeled this on the Otel wrapper. It just skips logs if there's a disabled topic in the context.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40126 +/- ##
==========================================
+ Coverage 66.28% 66.33% +0.04%
==========================================
Files 2446 2452 +6
Lines 195912 196624 +712
Branches 8574 8574
==========================================
+ Hits 129869 130424 +555
- Misses 54279 54388 +109
- Partials 11764 11812 +48
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR implements per-topic logging control functionality. It introduces a topic state management system in the logging package that tracks disabled topics via a thread-safe map and provides enable/disable APIs. A new TopicFilterHandler wraps the logging handler chain to filter out records with disabled topics. Configuration support is added via logging.enable_topics and logging.disable_topics keys, with CLI flag overrides in fleetctl. Server-side initialization applies these overrides during startup. The kitlog adapter is extended with context-aware logging methods (ErrorContext, WarnContext, InfoContext, DebugContext). Topics are enabled by default unless explicitly disabled. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
server/platform/logging/topic_handler.go (1)
23-28: Redundantif/returnpattern — simplify toreturn h.base.Enabled(ctx, level).♻️ Proposed fix
func (h *TopicFilterHandler) Enabled(ctx context.Context, level slog.Level) bool { - if !h.base.Enabled(ctx, level) { - return false - } - return true + return h.base.Enabled(ctx, level) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/logging/topic_handler.go` around lines 23 - 28, The Enabled method on TopicFilterHandler uses a redundant if/return pattern; replace the body of TopicFilterHandler.Enabled so it directly returns the result of h.base.Enabled(ctx, level) (i.e., change the implementation of Enabled to simply "return h.base.Enabled(ctx, level)") to simplify the logic and remove the unnecessary conditional.server/platform/logging/topics.go (2)
47-50:ResetTopicsis test-only but exported from the main package.The comment explicitly says "intended for use in tests to ensure isolation," yet it's a regular exported symbol visible to any consumer. Exposing it in production code risks accidental misuse and clutters the package's public API. Consider moving it to a
export_test.goortesting_helpers_test.gofile within the same package, or gating it under atestingpackage guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/logging/topics.go` around lines 47 - 50, ResetTopics is exported but intended only for tests; make it test-only by moving the ResetTopics implementation into a test file (e.g., export_test.go or testing_helpers_test.go) in the same package or change its visibility there (e.g., make it unexported) so production consumers can't call it; update any test callers to import/use the relocated ResetTopics (or the new unexported helper within the package tests) and remove the exported ResetTopics from topics.go (which contains disabledTopicsMu and disabledTopics) so the public API no longer exposes this test-only helper.
9-12:map[string]bool— usemap[string]struct{}for a set.The map only ever stores
true;map[string]struct{}is the idiomatic Go representation for a set and avoids the wasted bool byte per entry.♻️ Proposed refactor
var ( - disabledTopics = make(map[string]bool) + disabledTopics = make(map[string]struct{}) disabledTopicsMu sync.RWMutex ) func DisableTopic(name string) { disabledTopicsMu.Lock() - disabledTopics[name] = true + disabledTopics[name] = struct{}{} disabledTopicsMu.Unlock() } func TopicEnabled(name string) bool { disabledTopicsMu.RLock() - disabled := disabledTopics[name] + _, disabled := disabledTopics[name] disabledTopicsMu.RUnlock() return !disabled } func ResetTopics() { disabledTopicsMu.Lock() - disabledTopics = make(map[string]bool) + disabledTopics = make(map[string]struct{}) disabledTopicsMu.Unlock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/platform/logging/topics.go` around lines 9 - 12, Replace the boolean-set implementation with an empty-struct set: change the declaration of disabledTopics from map[string]bool to map[string]struct{} (and its initializer to make(map[string]struct{})), update places that add entries to use disabledTopics[key] = struct{}{} instead of true, check membership with the comma-ok pattern (_, ok := disabledTopics[key]) or by checking presence, and use delete(disabledTopics, key) for removals; keep disabledTopicsMu as-is for synchronization.cmd/fleetctl/fleetctl/flags.go (1)
103-116: ConsolidateparseLogTopicsListwith existingparseLogTopicsfunction.Both
cmd/fleetctl/fleetctl/flags.goandcmd/fleet/serve.gocontain identical implementations of comma-separated string parsing for log topics. Move the shared logic toserver/platform/loggingpackage (which both files already import) to eliminate duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/flags.go` around lines 103 - 116, There are duplicate comma-separated topic parsers (parseLogTopicsList in flags.go and parseLogTopics in serve.go); factor the shared logic into the server/platform/logging package (add an exported function, e.g. ParseLogTopics or ParseTopics) and remove the local parseLogTopicsList and parseLogTopics implementations; update callers in cmd/fleetctl/fleetctl/flags.go and cmd/fleet/serve.go to call the new server/platform/logging.ParseLogTopics and adjust imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/platform/logging/kitlog_adapter_test.go`:
- Around line 109-155: The subtests run in parallel but share the single handler
and adapter declared at the top, causing races; fix TestKitlogSlogWrappers by
creating a fresh test handler and logger inside each subtest (move the
testutils.NewTestHandler() and NewLogger(slog.New(...)) calls into the t.Run
closure before tc.logFunc is invoked) so tc.logFunc, handler.LastRecord() and
testutils.RecordAttrs(record) operate on per-subtest instances and avoid
concurrent mutation.
In `@server/platform/logging/topic_handler.go`:
- Around line 10-12: The doc comment for TopicFilterHandler incorrectly states
it filters based on the context; update it to describe that filtering is
attribute-based by reading the "log_topic" attribute from the slog.Record rather
than from context.Context. Mention TopicFilterHandler and that it implements
slog.Handler, and explicitly state that records are dropped when their
slog.Record "log_topic" attribute matches a disabled topic.
- Around line 47-50: The TopicFilterHandler currently ignores topics provided
via logger.With because WithAttrs only delegates to base.WithAttrs and the
Handle method only checks r.Attrs(); update WithAttrs to extract any "log_topic"
attr from the incoming attrs and return a TopicFilterHandler that stores that
captured topic (e.g., a topic string or disabled flag) alongside base:
TopicFilterHandler{base: h.base.WithAttrs(attrs), capturedTopic: "..."} so that
Handle (in TopicFilterHandler.Handle) short-circuits filtering by checking both
r.Attrs() and the stored capturedTopic from the handler before deciding to pass
or drop the record; ensure you reference TopicFilterHandler.WithAttrs,
TopicFilterHandler.Handle, base, r.Attrs(), and the "log_topic" name when
implementing the change.
---
Nitpick comments:
In `@cmd/fleetctl/fleetctl/flags.go`:
- Around line 103-116: There are duplicate comma-separated topic parsers
(parseLogTopicsList in flags.go and parseLogTopics in serve.go); factor the
shared logic into the server/platform/logging package (add an exported function,
e.g. ParseLogTopics or ParseTopics) and remove the local parseLogTopicsList and
parseLogTopics implementations; update callers in cmd/fleetctl/fleetctl/flags.go
and cmd/fleet/serve.go to call the new server/platform/logging.ParseLogTopics
and adjust imports accordingly.
In `@server/platform/logging/topic_handler.go`:
- Around line 23-28: The Enabled method on TopicFilterHandler uses a redundant
if/return pattern; replace the body of TopicFilterHandler.Enabled so it directly
returns the result of h.base.Enabled(ctx, level) (i.e., change the
implementation of Enabled to simply "return h.base.Enabled(ctx, level)") to
simplify the logic and remove the unnecessary conditional.
In `@server/platform/logging/topics.go`:
- Around line 47-50: ResetTopics is exported but intended only for tests; make
it test-only by moving the ResetTopics implementation into a test file (e.g.,
export_test.go or testing_helpers_test.go) in the same package or change its
visibility there (e.g., make it unexported) so production consumers can't call
it; update any test callers to import/use the relocated ResetTopics (or the new
unexported helper within the package tests) and remove the exported ResetTopics
from topics.go (which contains disabledTopicsMu and disabledTopics) so the
public API no longer exposes this test-only helper.
- Around line 9-12: Replace the boolean-set implementation with an empty-struct
set: change the declaration of disabledTopics from map[string]bool to
map[string]struct{} (and its initializer to make(map[string]struct{})), update
places that add entries to use disabledTopics[key] = struct{}{} instead of true,
check membership with the comma-ok pattern (_, ok := disabledTopics[key]) or by
checking presence, and use delete(disabledTopics, key) for removals; keep
disabledTopicsMu as-is for synchronization.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
I meant to flush this buffer but then got pulled into another thing. Going to finish reviewing now, sorry.
| } | ||
|
|
||
| // parseLogTopics splits a comma-separated string into trimmed, non-empty topic names. | ||
| func parseLogTopics(s string) []string { |
There was a problem hiding this comment.
We're adding this twice, but have something extremely similar in splitCleanSemicolonSeparated, which is used once, so would be easy enough to add a delimiter option there and go from three functions to one.
There was a problem hiding this comment.
I'm down, but I don't know where to put a utility like this. Currently we have some in /server/fleet/utils.go which seems bad from a modularity standpoint, even if all the places that would use it currently import the fleet package anyway. So I could go with a new /pkg/utils or put it in /server/platform/utils. Any preferences? cc: @getvictor
There was a problem hiding this comment.
pkg/str so we don't wind up with the utils junk drawer disease?
There was a problem hiding this comment.
We can't use strings.Split(s, delim) here?
I'm ok with pkg/str
There was a problem hiding this comment.
We can't use strings.Split(s, delim) here?
Alas we want to trim as well, and remove empty values. I'll add a generic "split string by delimiter and trim" function to /pkg/str and use it in all three places.
|
@sgress454 actually this seems like it's WIP due to architectural/edge case limitations. Ping me again when this is ready for another review if you like, though @getvictor is going to have a bit more context on this particular thing. |
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good. Please fix lint issues in topic_handler.go
| } | ||
|
|
||
| // parseLogTopics splits a comma-separated string into trimmed, non-empty topic names. | ||
| func parseLogTopics(s string) []string { |
There was a problem hiding this comment.
We can't use strings.Split(s, delim) here?
I'm ok with pkg/str
| errList, warnList := splitCleanSemicolonSeparated(errors), splitCleanSemicolonSeparated(warnings) | ||
| errList, warnList := str.SplitAndTrim(errors, ";", true), str.SplitAndTrim(warnings, ";", true) |
There was a problem hiding this comment.
There's no test for the directIngestMunkiInfo function this is in, but I at least had two bots independently verify that the logic was equivalent.
3 participants
Related issue: Resolves #40124
Details
Implements the proposal in https://docs.google.com/document/d/16qe6oVLKK25nA9GEIPR9Gw_IJ342_wlJRdnWEMmWdas/edit?tab=t.0#heading=h.nlw4agv1xs3g
Allows doing e.g.
or
Topics can be disabled at the app level, and enabled/disabled at the command-line level.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
No logs have this in prod yet, but I added some manually in a branch and verified that I could enable/disable them via CLI options and env vars, including enabling topics that were disabled on the server. Tested for both server and
fleetctl gitops.Summary by CodeRabbit
Release Notes