Skip to content

HYPERFLEET-1492 - feat: add WithStackTrace option for selective stack trace capture #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kuudori wants to merge 1 commit into
base: main
Choose a base branch
from
Open

HYPERFLEET-1492 - feat: add WithStackTrace option for selective stack trace capture #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
GO := go
TOOL_MOD := tools/go.mod
gotool = "$(GO)" tool -modfile="$(TOOL_MOD)" $(1)

.PHONY: lint fmt gofmt go-vet test install-hooks

# Run linter
lint:
$(call gotool,golangci-lint) run ./...

# Format code
fmt:
gofmt -s -w .

# Alias for fmt (required by hyperfleet-hooks)
gofmt: fmt

# Run go vet (required by hyperfleet-hooks)
go-vet:
$(GO) vet ./...

# Run unit tests
test:
$(GO) test -race ./...

# Install pre-commit hooks
install-hooks:
pre-commit install
31 changes: 14 additions & 17 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# HyperFleet Logger

Shared `log/slog` handler and context helpers for all HyperFleet Go components. Provides a single `NewHandler` call that enforces the [HyperFleet Logging Specification](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/logging-specification.md) - structured JSON (or human-readable text) with automatic enrichment fields, context-propagated trace/resource IDs, and stack traces on errors.
Shared `log/slog` handler and context helpers for all HyperFleet Go components, implementing the [HyperFleet Logging Specification](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/logging-specification.md).

HyperFleet components (API, Sentinel, Adapter) adopt this library to keep logs uniform across the platform. Callers use stdlib `slog` directly; this package only configures the handler.

Expand All @@ -9,7 +9,7 @@ HyperFleet components (API, Sentinel, Adapter) adopt this library to keep logs u
- **Automatic enrichment** - `component`, `version`, and `hostname` on every record, at root level regardless of `WithGroup` nesting
- **Context field extraction** - `trace_id`, `span_id`, `resource_type`, `resource_id` pulled from `context.Context` automatically
- **Extensible context fields** - register component-specific fields (e.g. `request_id`, `event_id`, `cluster_id`) via `WithContextFields`
- **Stack traces on errors** - `ERROR`-level and above automatically include a filtered Go stack trace; use `WARN` for expected/handled errors that don't need traces
- **Opt-in stack traces on errors** - register a `WithStackTrace` filter to attach a filtered Go stack trace to `ERROR`-level (and above) records; each component decides for itself which errors are worth tracing
- **Dual format** - JSON (default) for production, human-readable text for local development
- **Zero dependencies** - stdlib only (`log/slog`, `os`, `io`, `context`)
- **Environment-driven config** - `ParseLevel`, `ParseFormat`, `ParseOutput` parse `HYPERFLEET_LOG_LEVEL`, `HYPERFLEET_LOG_FORMAT`, `HYPERFLEET_LOG_OUTPUT` strings
Expand Down Expand Up @@ -48,7 +48,7 @@ func main() {
fmt.Fprintf(os.Stderr, "invalid HYPERFLEET_LOG_OUTPUT: %v\n", err)
}

handler := hfl.NewHandler("sentinel", "v1.2.3",
handler := hfl.NewHandler("my-service", "v1.2.3",
hfl.WithLevel(level),
hfl.WithFormat(format),
hfl.WithOutput(output),
Expand All @@ -70,7 +70,7 @@ func main() {
"timestamp": "2025-01-15T10:30:00.000Z",
"level": "info",
"message": "reconciling cluster",
"component": "sentinel",
"component": "my-service",
"version": "v1.2.3",
"hostname": "pod-abc",
"trace_id": "4bf92f3577b34da6a3ce929d0e0e4736",
Expand All @@ -82,7 +82,7 @@ func main() {
**Text output** (`HYPERFLEET_LOG_FORMAT=text`):

```text
2025-01-15T10:30:00.000Z INFO [sentinel] [v1.2.3] [pod-abc] reconciling cluster trace_id=4bf92f3577b34da6a3ce929d0e0e4736 resource_type=cluster resource_id=cls-abc-123
2025-01-15T10:30:00.000Z INFO [my-service] [v1.2.3] [pod-abc] reconciling cluster trace_id=4bf92f3577b34da6a3ce929d0e0e4736 resource_type=cluster resource_id=cls-abc-123
```

## API Reference
Expand All @@ -102,6 +102,7 @@ Creates a `slog.Handler` with automatic enrichment. Options:
| `WithOutput(io.Writer)` | `os.Stdout` | Log output destination |
| `WithHostname(string)` | `os.Hostname()` | Override the `hostname` field |
| `WithContextFields(...ContextField)` | built-in set | Register additional context-extracted fields |
| `WithStackTrace(func(context.Context, slog.Record) bool)` | none (never captures) | Filter deciding whether an `ERROR`-level (or above) record gets a stack trace; only consulted at that level or above |

### Context Helpers

Expand All @@ -123,7 +124,7 @@ Register component-specific fields that are automatically extracted from the con
```go
var reqIDKey = hfl.NewKey[string]("request_id")

handler := hfl.NewHandler("api", "v1.4.0",
handler := hfl.NewHandler("my-service", "v1.4.0",
hfl.WithContextFields(
hfl.StringField(reqIDKey),
),
Expand Down Expand Up @@ -159,18 +160,14 @@ hfl.FieldStackTrace // "stack_trace"

## Error Stack Traces

`ERROR`-level and above automatically include a `stack_trace` field with a filtered call stack (slog/runtime/testing internals excluded). For expected or handled errors (validation failures, 404s, retries) use `WARN` level to avoid the stack trace overhead:
Stack trace capture is opt-in - no filter registered means no `stack_trace` field is ever added, even at `ERROR` level. `WithStackTrace` takes a predicate; it's only consulted at `ERROR` level or above:

```json
{
"level": "error",
"message": "failed to update cluster",
"component": "api",
"stack_trace": [
"main.handleRequest() server.go:142",
"main.main() main.go:28"
]
}
```go
handler := hfl.NewHandler("my-service", "v1.2.3",
hfl.WithStackTrace(func(ctx context.Context, r slog.Record) bool {
return true // or any caller-defined condition
}),
)
```

## License
Expand Down
7 changes: 5 additions & 2 deletions doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//
// Typical usage:
//
// handler := hyperfleetlogger.NewHandler("sentinel", "v1.2.3")
// handler := hyperfleetlogger.NewHandler("my-service", "v1.2.3")
// slog.SetDefault(slog.New(handler))
// ctx := hyperfleetlogger.WithResourceType(context.Background(), "cluster")
// ctx = hyperfleetlogger.WithResourceID(ctx, "cluster-1")
Expand All @@ -18,12 +18,15 @@
// ctx = hyperfleetlogger.Set(ctx, RetryCountKey, 3)
//
// // Register as a context field
// handler := hyperfleetlogger.NewHandler("sentinel", "v1.2.3",
// handler := hyperfleetlogger.NewHandler("my-service", "v1.2.3",
// hyperfleetlogger.WithContextFields(
// hyperfleetlogger.FieldFromKey(RetryCountKey, slog.IntValue),
// ),
// )
//
// Stack traces on error-level records are opt-in, not automatic - see
// WithStackTrace.
//
// The package intentionally stays thin: it provides handler construction,
// context field helpers, and field name constants while callers use stdlib
// slog directly.
Expand Down
53 changes: 35 additions & 18 deletions handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type Option func(*config)

type config struct {
output io.Writer
stackTraceFilter func(ctx context.Context, r slog.Record) bool
hostname string
extraContextFields []ContextField
level slog.Level
Expand Down Expand Up @@ -73,9 +74,14 @@ func WithSanitize() Option {
return func(cfg *config) { cfg.sanitize = true }
}

// WithStackTrace sets a filter (checked only at LevelError+) deciding whether to attach a stack trace; unset = never.
func WithStackTrace(filter func(ctx context.Context, r slog.Record) bool) Option {
return func(cfg *config) { cfg.stackTraceFilter = filter }
}

// NewHandler returns a slog.Handler that adds component, version, and
// hostname to every record, extracts registered context fields, and
// attaches stack traces to error-level logs.
// hostname to every record and extracts registered context fields. Stack
// traces on error-level records are opt-in - see WithStackTrace.
func NewHandler(component, version string, opts ...Option) slog.Handler {
cfg := defaultConfig()
for _, opt := range opts {
Expand All @@ -93,8 +99,9 @@ func NewHandler(component, version string, opts ...Option) slog.Handler {
}

return &hyperfleetHandler{
inner: enriched,
contextFields: deduplicateContextFields(defaultContextFields, cfg.extraContextFields),
inner: enriched,
contextFields: deduplicateContextFields(defaultContextFields, cfg.extraContextFields),
stackTraceFilter: cfg.stackTraceFilter,
}
}

Expand All @@ -104,10 +111,11 @@ type groupedAttrs struct {
}

type hyperfleetHandler struct {
inner slog.Handler
groups []string
preAttrs []groupedAttrs
contextFields []ContextField
inner slog.Handler
stackTraceFilter func(ctx context.Context, r slog.Record) bool
groups []string
preAttrs []groupedAttrs
contextFields []ContextField
}

func defaultConfig() config {
Expand Down Expand Up @@ -139,7 +147,7 @@ func (h *hyperfleetHandler) Handle(ctx context.Context, r slog.Record) error {
nr.AddAttrs(slog.Attr{Key: f.Name, Value: v})
}
}
if r.Level >= slog.LevelError {
if r.Level >= slog.LevelError && h.stackTraceFilter != nil && h.stackTraceFilter(ctx, r) {
nr.AddAttrs(slog.Any(FieldStackTrace, captureStackTrace()))
}

Expand All @@ -162,10 +170,11 @@ func (h *hyperfleetHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
return h
}
return &hyperfleetHandler{
inner: h.inner,
groups: h.groups,
preAttrs: append(slices.Clone(h.preAttrs), groupedAttrs{groups: h.groups, attrs: attrs}),
contextFields: h.contextFields,
inner: h.inner,
groups: h.groups,
preAttrs: append(slices.Clone(h.preAttrs), groupedAttrs{groups: h.groups, attrs: attrs}),
contextFields: h.contextFields,
stackTraceFilter: h.stackTraceFilter,
}
}

Expand All @@ -174,10 +183,11 @@ func (h *hyperfleetHandler) WithGroup(name string) slog.Handler {
return h
}
return &hyperfleetHandler{
inner: h.inner,
groups: append(slices.Clone(h.groups), name),
preAttrs: h.preAttrs,
contextFields: h.contextFields,
inner: h.inner,
groups: append(slices.Clone(h.groups), name),
preAttrs: h.preAttrs,
contextFields: h.contextFields,
stackTraceFilter: h.stackTraceFilter,
}
}

Expand Down Expand Up @@ -237,7 +247,14 @@ func newPool[T any](fn func() T) pool[T] {
return pool[T]{p: sync.Pool{New: func() any { return fn() }}}
}

func (p *pool[T]) Get() T { return p.p.Get().(T) }
func (p *pool[T]) Get() T {
if v, ok := p.p.Get().(T); ok {
return v
}
var zero T
return zero
Comment on lines +250 to +255

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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

}

func (p *pool[T]) Put(v T) { p.p.Put(v) }

var pcsPool = newPool(func() *[]uintptr {
Expand Down
Loading