HYPERFLEET-1492 - feat: add WithStackTrace option for selective stack trace capture - #5
Open
kuudori wants to merge 1 commit into
Conversation
… 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.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe handler now captures error-level stack traces only when Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Logger
participant Handler
participant StackTraceFilter
participant LogOutput
Logger->>Handler: Emit record
Handler->>StackTraceFilter: Evaluate error record
StackTraceFilter-->>Handler: Return capture decision
Handler->>LogOutput: Attach stack trace when accepted
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: no go files to analyze: running 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)
Comment on lines
+250
to
+255
| func (p *pool[T]) Get() T { | ||
| if v, ok := p.p.Get().(T); ok { | ||
| return v | ||
| } | ||
| var zero T | ||
| return zero |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return a fresh pooled value after a type mismatch.
For pcsPool, the zero value is a nil *[]uintptr. captureStackTrace dereferences it at line 271. An unexpected pool value now causes a nil-pointer panic instead of being tolerated. This is CWE-476.
Proposed fix
func (p *pool[T]) Get() T {
- if v, ok := p.p.Get().(T); ok {
+ v := p.p.Get()
+ if v, ok := v.(T); ok {
return v
}
- var zero T
- return zero
+ return p.p.New().(T)
}As per path instructions, flag nil access without guards.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (p *pool[T]) Get() T { | |
| if v, ok := p.p.Get().(T); ok { | |
| return v | |
| } | |
| var zero T | |
| return zero | |
| func (p *pool[T]) Get() T { | |
| v := p.p.Get() | |
| if v, ok := v.(T); ok { | |
| return v | |
| } | |
| return p.p.New().(T) |
🤖 Prompt for 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.
In `@handler.go` around lines 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.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)