HYPERFLEET-1492 - feat: add WithStackTrace option for selective stack trace capture - #5
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe handler captures error-level stack traces only when Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@handler.go`:
- Around line 250-255: Update the generic pool method Get so a type mismatch
does not return the zero value when that value may be nil and later
dereferenced. For pcsPool callers such as captureStackTrace, allocate and return
a fresh valid pooled value of the expected type on mismatch, while preserving
the existing successful type-assertion path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5a288180-ff3e-4dd5-8de4-8cd2b4400cb5
⛔ Files ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (6)
MakefileREADME.mddoc.gohandler.gohandler_test.gotools/go.mod
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
… trace capture The shared handler previously attached a stack trace to every ERROR-level record unconditionally, with no way to opt out. Consumers like the Adapter classify some errors as expected (K8s NotFound/Conflict, network blips, HyperFleet API 4xx/5xx) and want to skip stack traces for those specifically, which is a per-log-call decision that a construction-time flag can't express. WithStackTrace registers a filter, consulted only at slog.LevelError or above, that decides whether a given record gets a stack trace - with no implicit default, so each consumer opts in explicitly. Also adds the Makefile and pinned tools/go.mod required by this repo's hyperfleet-hooks pre-commit configuration (gofmt/lint/go-vet targets), which were missing, and fixes a pre-existing errcheck lint violation in pool.Get() surfaced once golangci-lint could actually run via make lint.
1aa0889 to
1a6f127
Compare
| } | ||
|
|
||
| // alwaysCaptureStackTrace is a WithStackTrace filter that always returns true. | ||
| func alwaysCaptureStackTrace(context.Context, slog.Record) bool { return true } |
There was a problem hiding this comment.
alwaysCaptureStackTrace is duplicated across 6+ tests in this file. Consider exporting it (e.g. AlwaysCaptureStackTrace) so it's reusable outside the package too.
There was a problem hiding this comment.
I'd leave it unexported for now, since there's no package that would use it outside this one. It's just a helper for the stack trace tests here 🤔
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 10: Fix the shell-injection risk in the help recipe around the awk
invocation and the GO-related recipes at the referenced lines: do not
interpolate MAKEFILE_LIST or other externally controllable variables unquoted
into shell commands, and prevent option injection by using a fixed trusted
makefile or safely escaped paths with an explicit awk option terminator. Keep GO
restricted to a validated executable path and invoke it as one separately quoted
argument.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5d88fbc6-e853-47c2-93f0-d16319628eb6
📒 Files selected for processing (4)
MakefileREADME.mdhandler.gohandler_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (3)
- handler.go
- handler_test.go
- README.md
f4646f6 to
d93eb5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 10: Update the help-target awk pattern to use the portable POSIX ERE form
`.*##` instead of `.*?##`, preserving the existing target matching and formatted
output behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2815e247-0932-40d8-954d-cb69171e3aab
📒 Files selected for processing (1)
Makefile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
…akefile, tests, docs Make pool[T].Get() call its constructor directly on a type-mismatch fallback instead of risking a nil zero value, add a help target to the Makefile, collapse duplicate stack-trace filter tests into a table, export the shared test filter helper, and restore the stack_trace example in the README.
d93eb5f to
bcf3ebc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 10: Update the help recipe’s awk invocation around MAKEFILE_LIST to
prevent shell interpretation of makefile paths: quote or otherwise safely
shell-escape each expanded path before passing it to awk, while preserving the
existing help-target filtering and output behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 29020d75-1504-4eb8-a13b-c074df037133
📒 Files selected for processing (1)
Makefile
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
2 participants
Summary
WithStackTrace(func(ctx context.Context, r slog.Record) bool) OptiontoNewHandler. The filter is consulted only atslog.LevelErroror above; it decides whether that record gets astack_tracefield.ERROR+ record unconditionally. That matches Sentinel's original (pre-extraction) logger, so it's not a regression for Sentinel - but it doesn't generalize. The Adapter's own logger classifies some errors as expected/routine (K8sNotFound/Conflict, network blips, HyperFleet API 4xx/5xx) and skips stack traces for those specifically, while still logging atERRORlevel. That's a per-log-call decision, not a per-handler-instance one, so a simple on/off flag can't express it without forcing every such consumer to hand-write a full wrappingslog.Handler. A caller-supplied filter, consulted directly insideHandle(), solves this with no extra plumbing required from consumers.Makefileand pinnedtools/go.modthis repo'shyperfleet-hookspre-commit config (hyperfleet-gofmt,hyperfleet-golangci-lint,hyperfleet-go-vet) requires - both were missing, so the pre-commit hooks could never actually run in this repo.errchecklint violation inpool.Get(), surfaced oncegolangci-lintcould actually run viamake lint.Test plan
go build ./...go vet ./...go test ./... -race -count=1(40/40 passing)make lint(0 issues via pinnedtools/go.modgolangci-lint)gofmt -l .clean.With()/.WithGroup()handler chaining (verified this test actually fails without the corresponding fix inWithAttrs/WithGroup)