From a21e9c6a98ee9b73958b193305fff2dee365047a Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Thu, 28 May 2026 16:43:58 +0200 Subject: [PATCH 1/4] perf: fast-path common pointer types in dereferencePointers + inline tag dedup - Add type-switch cases for *string, *int, *int64, *uint, *uint64, *float64, *bool in dereferencePointers to avoid reflect.ValueOf for the most common pointer types - Inline tag deduplication in Tags() using a seen map, removing the lo.Uniq pass after accumulation - Add TestSnapshot covering all snapshot merge semantics (13 cases) --- error.go | 10 ++- kv.go | 54 +++++++++++++++-- kv_test.go | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 230 insertions(+), 8 deletions(-) diff --git a/error.go b/error.go index 56fc988..85f6e8f 100644 --- a/error.go +++ b/error.go @@ -286,13 +286,19 @@ func (o OopsError) Domain() string { // Tags are merged from all errors in the chain and deduplicated. func (o OopsError) Tags() []string { tags := make([]string, 0, 8) // reasonable initial capacity for tags + seen := map[string]struct{}{} recursive(o, func(e OopsError) bool { - tags = append(tags, e.tags...) + for _, tag := range e.tags { + if _, ok := seen[tag]; !ok { + tags = append(tags, tag) + seen[tag] = struct{}{} + } + } return true }) - return lo.Uniq(tags) + return tags } // HasTag checks if the error or any of its wrapped errors contain the specified tag. diff --git a/kv.go b/kv.go index 76c519d..8fcc2aa 100644 --- a/kv.go +++ b/kv.go @@ -35,17 +35,59 @@ func dereferencePointers(data map[string]any) map[string]any { } for key, value := range data { - // Fast path: only use reflect for types that could be pointers - switch value.(type) { + switch v := value.(type) { case nil, string, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64, bool, []byte, map[string]any, []any: continue // not a pointer, skip - } - val := reflect.ValueOf(value) - if val.Kind() == reflect.Pointer { - data[key] = dereferencePointerRecursive(val, 0) + case *string: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + case *int: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + case *int64: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + case *uint: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + case *uint64: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + case *float64: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + case *bool: + if v != nil { + data[key] = *v + } else { + data[key] = nil + } + default: + val := reflect.ValueOf(value) + if val.Kind() == reflect.Pointer { + data[key] = dereferencePointerRecursive(val, 0) + } } } diff --git a/kv_test.go b/kv_test.go index 273fe57..a45e512 100644 --- a/kv_test.go +++ b/kv_test.go @@ -6,6 +6,7 @@ import ( "fmt" "reflect" "testing" + "time" "unsafe" "github.com/stretchr/testify/assert" @@ -809,3 +810,176 @@ func TestCollectMaps(t *testing.T) { }) } } + +func TestSnapshot(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + run func(is *assert.Assertions) + }{ + { + name: "single_error_scalar_fields_preserved", + run: func(is *assert.Assertions) { + now := time.Now().Truncate(time.Second) + dur := 5 * time.Second + err := Code("E001"). + Time(now). + Duration(dur). + In("payments"). + Hint("check config"). + Public("payment failed"). + Owner("team-payments"). + With("key", "val"). + Tags("tag1", "tag2"). + Errorf("base error").(OopsError) + s := snapshot(err) + is.Equal("E001", s.code) + is.Equal(now, s.time) + is.Equal(dur, s.duration) + is.Equal("payments", s.domain) + is.Equal("check config", s.hint) + is.Equal("payment failed", s.public) + is.Equal("team-payments", s.owner) + is.Equal(map[string]any{"key": "val"}, s.context) + is.ElementsMatch([]string{"tag1", "tag2"}, s.tags) + }, + }, + { + name: "chained_innermost_scalar_wins", + run: func(is *assert.Assertions) { + inner := Code("INNER").In("inner-domain").Hint("inner-hint").Errorf("inner") + outer := Code("OUTER").In("outer-domain").Hint("outer-hint").Wrap(inner).(OopsError) + s := snapshot(outer) + is.Equal("INNER", s.code) + is.Equal("inner-domain", s.domain) + is.Equal("inner-hint", s.hint) + }, + }, + { + name: "chained_inner_code_with_outer_fallback", + run: func(is *assert.Assertions) { + inner := Errorf("inner") // no code + outer := Code("OUTER").Wrap(inner).(OopsError) + s := snapshot(outer) + is.Equal("OUTER", s.code) + }, + }, + { + name: "context_inner_key_wins_outer_key_merged", + run: func(is *assert.Assertions) { + inner := With("shared", "inner", "inner_only", 1).Errorf("inner") + outer := With("shared", "outer", "outer_only", 2).Wrap(inner).(OopsError) + s := snapshot(outer) + is.Equal("inner", s.context["shared"]) + is.Equal(1, s.context["inner_only"]) + is.Equal(2, s.context["outer_only"]) + }, + }, + { + name: "tags_deduplicated_across_chain", + run: func(is *assert.Assertions) { + inner := Tags("a", "b").Errorf("inner") + outer := Tags("b", "c").Wrap(inner).(OopsError) + s := snapshot(outer) + is.ElementsMatch([]string{"a", "b", "c"}, s.tags) + }, + }, + { + name: "explicit_trace_wins_over_auto_generated", + run: func(is *assert.Assertions) { + // outer has explicit trace; inner gets auto-generated + inner := Errorf("inner") // traceAutoGenerated=true + outer := Trace("explicit-trace-id").Wrap(inner).(OopsError) + s := snapshot(outer) + is.Equal("explicit-trace-id", s.trace) + }, + }, + { + name: "auto_trace_used_when_no_explicit", + run: func(is *assert.Assertions) { + err := Errorf("err").(OopsError) + s := snapshot(err) + // AutoTraceID is on by default: a non-empty trace should be present + is.NotEmpty(s.trace) + }, + }, + { + name: "context_lazy_value_evaluated", + run: func(is *assert.Assertions) { + err := With("computed", func() any { return "evaluated" }).Errorf("err").(OopsError) + s := snapshot(err) + is.Equal("evaluated", s.context["computed"]) + }, + }, + { + name: "context_pointer_dereferenced", + run: func(is *assert.Assertions) { + val := "deref_me" + err := With("ptr", &val).Errorf("err").(OopsError) + s := snapshot(err) + is.Equal("deref_me", s.context["ptr"]) + }, + }, + { + name: "user_innermost_wins_data_merged", + run: func(is *assert.Assertions) { + inner := User("uid-inner", "role", "admin", "name", "inner").Errorf("inner") + outer := User("uid-outer", "name", "outer").Wrap(inner).(OopsError) + s := snapshot(outer) + is.Equal("uid-inner", s.userID) + is.Equal("admin", s.userData["role"]) + is.Equal("inner", s.userData["name"]) // inner wins + }, + }, + { + name: "tenant_innermost_wins_data_merged", + run: func(is *assert.Assertions) { + inner := Tenant("tid-inner", "plan", "enterprise").Errorf("inner") + outer := Tenant("tid-outer", "plan", "free", "region", "us").Wrap(inner).(OopsError) + s := snapshot(outer) + is.Equal("tid-inner", s.tenantID) + is.Equal("enterprise", s.tenantData["plan"]) // inner wins + is.Equal("us", s.tenantData["region"]) // outer-only key preserved + }, + }, + { + name: "empty_error_zero_snapshot", + run: func(is *assert.Assertions) { + err := Errorf("bare").(OopsError) + s := snapshot(err) + is.Nil(s.code) + is.Equal("", s.domain) + is.Equal("", s.hint) + is.Equal("", s.public) + is.Equal("", s.owner) + is.Equal("", s.userID) + is.Equal("", s.tenantID) + is.Empty(s.tags) + is.Empty(s.context) + is.Empty(s.userData) + is.Empty(s.tenantData) + }, + }, + { + name: "snapshot_fields_not_cached_in_output", + run: func(is *assert.Assertions) { + // snapshot must not copy stacktrace, err, or cache fields + err := Code("X").With("k", "v").Errorf("msg").(OopsError) + s := snapshot(err) + is.Nil(s.err) + is.Nil(s.stacktrace) + is.Nil(s.cacheOnce) + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + is := assert.New(t) + tt.run(is) + }) + } +} From c3818a53b0cae0a06dccac1473d3b5a720ce719a Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Thu, 28 May 2026 16:47:11 +0200 Subject: [PATCH 2/4] fix(lint): suppress gocyclo on dereferencePointers type-switch --- kv.go | 2 +- kv_test.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/kv.go b/kv.go index 8fcc2aa..221bb68 100644 --- a/kv.go +++ b/kv.go @@ -29,7 +29,7 @@ import ( // result := dereferencePointers(data) // // result["user"] will be User{Name: "John"} instead of *User // // result["count"] will be 42 instead of *int -func dereferencePointers(data map[string]any) map[string]any { +func dereferencePointers(data map[string]any) map[string]any { //nolint:gocyclo if !DereferencePointers { return data } diff --git a/kv_test.go b/kv_test.go index a45e512..d453bac 100644 --- a/kv_test.go +++ b/kv_test.go @@ -949,12 +949,12 @@ func TestSnapshot(t *testing.T) { err := Errorf("bare").(OopsError) s := snapshot(err) is.Nil(s.code) - is.Equal("", s.domain) - is.Equal("", s.hint) - is.Equal("", s.public) - is.Equal("", s.owner) - is.Equal("", s.userID) - is.Equal("", s.tenantID) + is.Empty(s.domain) + is.Empty(s.hint) + is.Empty(s.public) + is.Empty(s.owner) + is.Empty(s.userID) + is.Empty(s.tenantID) is.Empty(s.tags) is.Empty(s.context) is.Empty(s.userData) @@ -967,7 +967,7 @@ func TestSnapshot(t *testing.T) { // snapshot must not copy stacktrace, err, or cache fields err := Code("X").With("k", "v").Errorf("msg").(OopsError) s := snapshot(err) - is.Nil(s.err) + is.NoError(s.err) is.Nil(s.stacktrace) is.Nil(s.cacheOnce) }, From 6e04a9fa3e290d35a5ccd5beecec43c826b240d7 Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Thu, 28 May 2026 16:49:59 +0200 Subject: [PATCH 3/4] refactor: use derefPtr generic helper in dereferencePointers --- kv.go | 51 +++++++++++++++------------------------------------ 1 file changed, 15 insertions(+), 36 deletions(-) diff --git a/kv.go b/kv.go index 221bb68..a72810e 100644 --- a/kv.go +++ b/kv.go @@ -29,7 +29,14 @@ import ( // result := dereferencePointers(data) // // result["user"] will be User{Name: "John"} instead of *User // // result["count"] will be 42 instead of *int -func dereferencePointers(data map[string]any) map[string]any { //nolint:gocyclo +func derefPtr[T any](p *T) any { + if p == nil { + return nil + } + return *p +} + +func dereferencePointers(data map[string]any) map[string]any { if !DereferencePointers { return data } @@ -42,47 +49,19 @@ func dereferencePointers(data map[string]any) map[string]any { //nolint:gocyclo map[string]any, []any: continue // not a pointer, skip case *string: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) case *int: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) case *int64: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) case *uint: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) case *uint64: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) case *float64: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) case *bool: - if v != nil { - data[key] = *v - } else { - data[key] = nil - } + data[key] = derefPtr(v) default: val := reflect.ValueOf(value) if val.Kind() == reflect.Pointer { From 524315a78be55833c0abd84706069d38d657c0a0 Mon Sep 17 00:00:00 2001 From: Samuel Berthe Date: Thu, 28 May 2026 16:53:40 +0200 Subject: [PATCH 4/4] test: add fast-path coverage for *int64, *uint, *uint64, *float64, *bool (nil and non-nil) --- kv_test.go | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 84 insertions(+), 3 deletions(-) diff --git a/kv_test.go b/kv_test.go index d453bac..1c3ce1d 100644 --- a/kv_test.go +++ b/kv_test.go @@ -125,12 +125,93 @@ func TestDereferencePointerEdgeCases(t *testing.T) { { name: "int_pointer", run: func(is *assert.Assertions) { - intValue := 42 - intPtr := &intValue - err := With("int", intPtr).Errorf(anErrorStr).(OopsError) + v := 42 + err := With("int", &v).Errorf(anErrorStr).(OopsError) is.Equal(map[string]any{"int": 42}, err.Context()) }, }, + { + name: "nil_int_fast_path", + run: func(is *assert.Assertions) { + err := With("v", (*int)(nil)).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": nil}, err.Context()) + }, + }, + { + name: "int64_pointer", + run: func(is *assert.Assertions) { + v := int64(100) + err := With("v", &v).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": int64(100)}, err.Context()) + }, + }, + { + name: "nil_int64_fast_path", + run: func(is *assert.Assertions) { + err := With("v", (*int64)(nil)).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": nil}, err.Context()) + }, + }, + { + name: "uint_pointer", + run: func(is *assert.Assertions) { + v := uint(7) + err := With("v", &v).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": uint(7)}, err.Context()) + }, + }, + { + name: "nil_uint_fast_path", + run: func(is *assert.Assertions) { + err := With("v", (*uint)(nil)).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": nil}, err.Context()) + }, + }, + { + name: "uint64_pointer", + run: func(is *assert.Assertions) { + v := uint64(999) + err := With("v", &v).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": uint64(999)}, err.Context()) + }, + }, + { + name: "nil_uint64_fast_path", + run: func(is *assert.Assertions) { + err := With("v", (*uint64)(nil)).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": nil}, err.Context()) + }, + }, + { + name: "float64_pointer", + run: func(is *assert.Assertions) { + v := 3.14 + err := With("v", &v).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": 3.14}, err.Context()) + }, + }, + { + name: "nil_float64_fast_path", + run: func(is *assert.Assertions) { + err := With("v", (*float64)(nil)).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": nil}, err.Context()) + }, + }, + { + name: "bool_pointer", + run: func(is *assert.Assertions) { + v := true + err := With("v", &v).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": true}, err.Context()) + }, + }, + { + name: "nil_bool_fast_path", + run: func(is *assert.Assertions) { + err := With("v", (*bool)(nil)).Errorf(anErrorStr).(OopsError) + is.Equal(map[string]any{"v": nil}, err.Context()) + }, + }, { name: "struct_pointer", run: func(is *assert.Assertions) {