From 36b650765bdd2deff8cb309ef30e31022f2996fb Mon Sep 17 00:00:00 2001 From: m1heng <18018422+m1heng@users.noreply.github.com> Date: Sat, 12 Oct 2024 08:15:06 +0000 Subject: [PATCH 1/5] Move convert values to internal/shared/logutil --- CHANGELOG.md | 3 + bridges/otellogr/convert.go | 51 +--- bridges/otellogr/convert_test.go | 84 ++---- bridges/otellogr/gen.go | 8 + bridges/otellogr/logsink.go | 37 +++ bridges/otellogr/logsink_test.go | 80 ++++++ bridges/otellogrus/convert.go | 123 +++++++++ bridges/otellogrus/convert_test.go | 265 +++++++++++++++++++ bridges/otellogrus/gen.go | 8 + bridges/otellogrus/hook.go | 58 +--- bridges/otellogrus/hook_test.go | 16 ++ bridges/otelzap/convert.go | 123 +++++++++ bridges/otelzap/convert_test.go | 265 +++++++++++++++++++ bridges/otelzap/encoder.go | 59 +---- bridges/otelzap/encoder_test.go | 8 + bridges/otelzap/gen.go | 8 + internal/shared/logutil/convert.go.tmpl | 123 +++++++++ internal/shared/logutil/convert_test.go.tmpl | 265 +++++++++++++++++++ 18 files changed, 1362 insertions(+), 222 deletions(-) create mode 100644 bridges/otellogr/gen.go create mode 100644 bridges/otellogrus/convert.go create mode 100644 bridges/otellogrus/convert_test.go create mode 100644 bridges/otellogrus/gen.go create mode 100644 bridges/otelzap/convert.go create mode 100644 bridges/otelzap/convert_test.go create mode 100644 bridges/otelzap/gen.go create mode 100644 internal/shared/logutil/convert.go.tmpl create mode 100644 internal/shared/logutil/convert_test.go.tmpl diff --git a/CHANGELOG.md b/CHANGELOG.md index 68e2e751314..e261367d6f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,11 +22,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `NewSDK` in `go.opentelemetry.io/contrib/config` now returns a no-op SDK if `disabled` is set to `true`. (#6185) - The deprecated `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho` package has found a Code Owner. The package is no longer deprecated. (#6207) +- Aligned convertvalue in `otellogr`, `otellogrus` and `otelzap` with convert tmpl. (#6237) ### Fixed - Possible nil dereference panic in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#5965) - `logrus.Level` transformed to appropriate `log.Severity` in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6191) +- Possible nil dereference panic in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6237) +- Possible nil dereference panic in `go.opentelemetry.io/contrib/bridges/otelzap`. (#6237) ### Removed diff --git a/bridges/otellogr/convert.go b/bridges/otellogr/convert.go index 35a79e5248f..b703ad23535 100644 --- a/bridges/otellogr/convert.go +++ b/bridges/otellogr/convert.go @@ -1,10 +1,12 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert.go.tmpl + // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 package otellogr // import "go.opentelemetry.io/contrib/bridges/otellogr" import ( - "context" "fmt" "math" "reflect" @@ -14,43 +16,8 @@ import ( "go.opentelemetry.io/otel/log" ) -// convertKVs converts a list of key-value pairs to a list of [log.KeyValue]. -// The last [context.Context] value is returned as the context. -// If no context is found, the original context is returned. -func convertKVs(ctx context.Context, keysAndValues ...any) (context.Context, []log.KeyValue) { - if len(keysAndValues) == 0 { - return ctx, nil - } - if len(keysAndValues)%2 != 0 { - // Ensure an odd number of items here does not corrupt the list. - keysAndValues = append(keysAndValues, nil) - } - - kvs := make([]log.KeyValue, 0, len(keysAndValues)/2) - for i := 0; i < len(keysAndValues); i += 2 { - k, ok := keysAndValues[i].(string) - if !ok { - // Ensure that the key is a string. - k = fmt.Sprintf("%v", keysAndValues[i]) - } - - v := keysAndValues[i+1] - if vCtx, ok := v.(context.Context); ok { - // Special case when a field is of context.Context type. - ctx = vCtx - continue - } - - kvs = append(kvs, log.KeyValue{ - Key: k, - Value: convertValue(v), - }) - } - - return ctx, kvs -} - -func convertValue(v any) log.Value { +// Convert various types to log.Value. +func ConvertValue(v any) log.Value { // Handling the most common types without reflect is a small perf win. switch val := v.(type) { case bool: @@ -112,7 +79,7 @@ func convertValue(v any) log.Value { case reflect.Slice, reflect.Array: items := make([]log.Value, 0, val.Len()) for i := 0; i < val.Len(); i++ { - items = append(items, convertValue(val.Index(i).Interface())) + items = append(items, ConvertValue(val.Index(i).Interface())) } return log.SliceValue(items...) case reflect.Map: @@ -127,7 +94,7 @@ func convertValue(v any) log.Value { } kvs = append(kvs, log.KeyValue{ Key: key, - Value: convertValue(val.MapIndex(k).Interface()), + Value: ConvertValue(val.MapIndex(k).Interface()), }) } return log.MapValue(kvs...) @@ -135,7 +102,7 @@ func convertValue(v any) log.Value { if val.IsNil() { return log.Value{} } - return convertValue(val.Elem().Interface()) + return ConvertValue(val.Elem().Interface()) } // Try to handle this as gracefully as possible. @@ -146,7 +113,7 @@ func convertValue(v any) log.Value { return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) } -// convertUintValue converts a uint64 to a log.Value. +// ConvertUintValue converts a uint64 to a log.Value. // If the value is too large to fit in an int64, it is converted to a string. func convertUintValue(v uint64) log.Value { if v > math.MaxInt64 { diff --git a/bridges/otellogr/convert_test.go b/bridges/otellogr/convert_test.go index d39afb9137e..c0e1a3d1d7f 100644 --- a/bridges/otellogr/convert_test.go +++ b/bridges/otellogr/convert_test.go @@ -1,3 +1,6 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert_test.go.tmpl + // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 @@ -15,70 +18,6 @@ import ( "go.opentelemetry.io/otel/log" ) -func TestConvertKVs(t *testing.T) { - ctx := context.WithValue(context.Background(), "key", "value") // nolint: revive,staticcheck // test context - - for _, tt := range []struct { - name string - kvs []any - wantKVs []log.KeyValue - wantCtx context.Context - }{ - { - name: "empty", - kvs: []any{}, - }, - { - name: "single_value", - kvs: []any{"key", "value"}, - wantKVs: []log.KeyValue{ - log.String("key", "value"), - }, - }, - { - name: "multiple_values", - kvs: []any{"key1", "value1", "key2", "value2"}, - wantKVs: []log.KeyValue{ - log.String("key1", "value1"), - log.String("key2", "value2"), - }, - }, - { - name: "missing_value", - kvs: []any{"key1", "value1", "key2"}, - wantKVs: []log.KeyValue{ - log.String("key1", "value1"), - {Key: "key2", Value: log.Value{}}, - }, - }, - { - name: "key_not_string", - kvs: []any{42, "value"}, - wantKVs: []log.KeyValue{ - log.String("42", "value"), - }, - }, - { - name: "context", - kvs: []any{"ctx", ctx, "key", "value"}, - wantKVs: []log.KeyValue{log.String("key", "value")}, - wantCtx: ctx, - }, - { - name: "last_context", - kvs: []any{"key", context.Background(), "ctx", ctx}, - wantKVs: []log.KeyValue{}, - wantCtx: ctx, - }, - } { - t.Run(tt.name, func(t *testing.T) { - ctx, kvs := convertKVs(nil, tt.kvs...) // nolint: staticcheck // pass nil context - assert.Equal(t, tt.wantKVs, kvs) - assert.Equal(t, tt.wantCtx, ctx) - }) - } -} - func TestConvertValue(t *testing.T) { for _, tt := range []struct { name string @@ -293,20 +232,33 @@ func TestConvertValue(t *testing.T) { }, wantValue: log.StringValue("{Name:John Age:42}"), }, + { + name: "nil_struct_ptr", + value: (*struct { + Name string + Age int + })(nil), + wantValue: log.Value{}, + }, { name: "ctx", value: context.Background(), wantValue: log.StringValue("context.Background"), }, + { + name: "unhandled type", + value: chan int(nil), + wantValue: log.StringValue("unhandled: (chan int) "), + }, } { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.wantValue, convertValue(tt.value)) + assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) }) } } func TestConvertValueFloat32(t *testing.T) { - value := convertValue(float32(3.14)) + value := ConvertValue(float32(3.14)) want := log.Float64Value(3.14) assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) diff --git a/bridges/otellogr/gen.go b/bridges/otellogr/gen.go new file mode 100644 index 00000000000..bef73947bd6 --- /dev/null +++ b/bridges/otellogr/gen.go @@ -0,0 +1,8 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otellogr // import "go.opentelemetry.io/contrib/bridges/otellogr" + +// Generate convert: +//go:generate gotmpl --body=../../internal/shared/logutil/convert_test.go.tmpl "--data={ \"pkg\": \"otellogr\" }" --out=convert_test.go +//go:generate gotmpl --body=../../internal/shared/logutil/convert.go.tmpl "--data={ \"pkg\": \"otellogr\" }" --out=convert.go diff --git a/bridges/otellogr/logsink.go b/bridges/otellogr/logsink.go index 1acd6c55e56..5d473359e7d 100644 --- a/bridges/otellogr/logsink.go +++ b/bridges/otellogr/logsink.go @@ -45,6 +45,7 @@ package otellogr // import "go.opentelemetry.io/contrib/bridges/otellogr" import ( "context" + "fmt" "github.com/go-logr/logr" @@ -198,3 +199,39 @@ func (l LogSink) WithValues(keysAndValues ...any) logr.LogSink { l.ctx = ctx return &l } + +// convertKVs converts a list of key-value pairs to a list of [log.KeyValue]. +// The last [context.Context] value is returned as the context. +// If no context is found, the original context is returned. +func convertKVs(ctx context.Context, keysAndValues ...any) (context.Context, []log.KeyValue) { + if len(keysAndValues) == 0 { + return ctx, nil + } + if len(keysAndValues)%2 != 0 { + // Ensure an odd number of items here does not corrupt the list. + keysAndValues = append(keysAndValues, nil) + } + + kvs := make([]log.KeyValue, 0, len(keysAndValues)/2) + for i := 0; i < len(keysAndValues); i += 2 { + k, ok := keysAndValues[i].(string) + if !ok { + // Ensure that the key is a string. + k = fmt.Sprintf("%v", keysAndValues[i]) + } + + v := keysAndValues[i+1] + if vCtx, ok := v.(context.Context); ok { + // Special case when a field is of context.Context type. + ctx = vCtx + continue + } + + kvs = append(kvs, log.KeyValue{ + Key: k, + Value: ConvertValue(v), + }) + } + + return ctx, kvs +} diff --git a/bridges/otellogr/logsink_test.go b/bridges/otellogr/logsink_test.go index 8b47f871313..b0ba3446b1a 100644 --- a/bridges/otellogr/logsink_test.go +++ b/bridges/otellogr/logsink_test.go @@ -3,6 +3,7 @@ package otellogr import ( + "context" "testing" "time" @@ -216,6 +217,21 @@ func TestLogSink(t *testing.T) { }, }, }, + { + name: "info_with_normal_attr_and_nil_pointer_attr", + f: func(l *logr.Logger) { + var p *int + l.WithValues("key", "value", "nil_pointer", p).Info("info message with attrs") + }, + wantRecords: map[string][]log.Record{ + name: { + buildRecord(log.StringValue("info message with attrs"), time.Time{}, log.SeverityInfo, []log.KeyValue{ + log.String("key", "value"), + log.Empty("nil_pointer"), + }), + }, + }, + }, } { t.Run(tt.name, func(t *testing.T) { rec := logtest.NewRecorder() @@ -263,3 +279,67 @@ func assertRecords(t *testing.T, want, got []logtest.EmittedRecord) { logtest.AssertRecordEqual(t, j.Record, got[i].Record) } } + +func TestConvertKVs(t *testing.T) { + ctx := context.WithValue(context.Background(), "key", "value") // nolint: revive,staticcheck // test context + + for _, tt := range []struct { + name string + kvs []any + wantKVs []log.KeyValue + wantCtx context.Context + }{ + { + name: "empty", + kvs: []any{}, + }, + { + name: "single_value", + kvs: []any{"key", "value"}, + wantKVs: []log.KeyValue{ + log.String("key", "value"), + }, + }, + { + name: "multiple_values", + kvs: []any{"key1", "value1", "key2", "value2"}, + wantKVs: []log.KeyValue{ + log.String("key1", "value1"), + log.String("key2", "value2"), + }, + }, + { + name: "missing_value", + kvs: []any{"key1", "value1", "key2"}, + wantKVs: []log.KeyValue{ + log.String("key1", "value1"), + {Key: "key2", Value: log.Value{}}, + }, + }, + { + name: "key_not_string", + kvs: []any{42, "value"}, + wantKVs: []log.KeyValue{ + log.String("42", "value"), + }, + }, + { + name: "context", + kvs: []any{"ctx", ctx, "key", "value"}, + wantKVs: []log.KeyValue{log.String("key", "value")}, + wantCtx: ctx, + }, + { + name: "last_context", + kvs: []any{"key", context.Background(), "ctx", ctx}, + wantKVs: []log.KeyValue{}, + wantCtx: ctx, + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctx, kvs := convertKVs(nil, tt.kvs...) // nolint: staticcheck // pass nil context + assert.Equal(t, tt.wantKVs, kvs) + assert.Equal(t, tt.wantCtx, ctx) + }) + } +} diff --git a/bridges/otellogrus/convert.go b/bridges/otellogrus/convert.go new file mode 100644 index 00000000000..3fa5101d500 --- /dev/null +++ b/bridges/otellogrus/convert.go @@ -0,0 +1,123 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert.go.tmpl + +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otellogrus // import "go.opentelemetry.io/contrib/bridges/otellogrus" + +import ( + "fmt" + "math" + "reflect" + "strconv" + "time" + + "go.opentelemetry.io/otel/log" +) + +// Convert various types to log.Value. +func ConvertValue(v any) log.Value { + // Handling the most common types without reflect is a small perf win. + switch val := v.(type) { + case bool: + return log.BoolValue(val) + case string: + return log.StringValue(val) + case int: + return log.Int64Value(int64(val)) + case int8: + return log.Int64Value(int64(val)) + case int16: + return log.Int64Value(int64(val)) + case int32: + return log.Int64Value(int64(val)) + case int64: + return log.Int64Value(val) + case uint: + return convertUintValue(uint64(val)) + case uint8: + return log.Int64Value(int64(val)) + case uint16: + return log.Int64Value(int64(val)) + case uint32: + return log.Int64Value(int64(val)) + case uint64: + return convertUintValue(val) + case uintptr: + return convertUintValue(uint64(val)) + case float32: + return log.Float64Value(float64(val)) + case float64: + return log.Float64Value(val) + case time.Duration: + return log.Int64Value(val.Nanoseconds()) + case complex64: + r := log.Float64("r", real(complex128(val))) + i := log.Float64("i", imag(complex128(val))) + return log.MapValue(r, i) + case complex128: + r := log.Float64("r", real(val)) + i := log.Float64("i", imag(val)) + return log.MapValue(r, i) + case time.Time: + return log.Int64Value(val.UnixNano()) + case []byte: + return log.BytesValue(val) + case error: + return log.StringValue(val.Error()) + } + + t := reflect.TypeOf(v) + if t == nil { + return log.Value{} + } + val := reflect.ValueOf(v) + switch t.Kind() { + case reflect.Struct: + return log.StringValue(fmt.Sprintf("%+v", v)) + case reflect.Slice, reflect.Array: + items := make([]log.Value, 0, val.Len()) + for i := 0; i < val.Len(); i++ { + items = append(items, ConvertValue(val.Index(i).Interface())) + } + return log.SliceValue(items...) + case reflect.Map: + kvs := make([]log.KeyValue, 0, val.Len()) + for _, k := range val.MapKeys() { + var key string + switch k.Kind() { + case reflect.String: + key = k.String() + default: + key = fmt.Sprintf("%+v", k.Interface()) + } + kvs = append(kvs, log.KeyValue{ + Key: key, + Value: ConvertValue(val.MapIndex(k).Interface()), + }) + } + return log.MapValue(kvs...) + case reflect.Ptr, reflect.Interface: + if val.IsNil() { + return log.Value{} + } + return ConvertValue(val.Elem().Interface()) + } + + // Try to handle this as gracefully as possible. + // + // Don't panic here. it is preferable to have user's open issue + // asking why their attributes have a "unhandled: " prefix than + // say that their code is panicking. + return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) +} + +// ConvertUintValue converts a uint64 to a log.Value. +// If the value is too large to fit in an int64, it is converted to a string. +func convertUintValue(v uint64) log.Value { + if v > math.MaxInt64 { + return log.StringValue(strconv.FormatUint(v, 10)) + } + return log.Int64Value(int64(v)) +} diff --git a/bridges/otellogrus/convert_test.go b/bridges/otellogrus/convert_test.go new file mode 100644 index 00000000000..329d3a3bd1e --- /dev/null +++ b/bridges/otellogrus/convert_test.go @@ -0,0 +1,265 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert_test.go.tmpl + +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otellogrus + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/log" +) + +func TestConvertValue(t *testing.T) { + for _, tt := range []struct { + name string + value any + wantValue log.Value + }{ + { + name: "bool", + value: true, + wantValue: log.BoolValue(true), + }, + { + name: "string", + value: "value", + wantValue: log.StringValue("value"), + }, + { + name: "int", + value: 10, + wantValue: log.Int64Value(10), + }, + { + name: "int8", + value: int8(127), + wantValue: log.Int64Value(127), + }, + { + name: "int16", + value: int16(32767), + wantValue: log.Int64Value(32767), + }, + { + name: "int32", + value: int32(2147483647), + wantValue: log.Int64Value(2147483647), + }, + { + name: "int64", + value: int64(9223372036854775807), + wantValue: log.Int64Value(9223372036854775807), + }, + { + name: "uint", + value: uint(42), + wantValue: log.Int64Value(42), + }, + { + name: "uint8", + value: uint8(255), + wantValue: log.Int64Value(255), + }, + { + name: "uint16", + value: uint16(65535), + wantValue: log.Int64Value(65535), + }, + { + name: "uint32", + value: uint32(4294967295), + wantValue: log.Int64Value(4294967295), + }, + { + name: "uint64", + value: uint64(9223372036854775807), + wantValue: log.Int64Value(9223372036854775807), + }, + { + name: "uint64-max", + value: uint64(18446744073709551615), + wantValue: log.StringValue("18446744073709551615"), + }, + { + name: "uintptr", + value: uintptr(12345), + wantValue: log.Int64Value(12345), + }, + { + name: "float64", + value: float64(3.14159), + wantValue: log.Float64Value(3.14159), + }, + { + name: "time.Duration", + value: time.Second, + wantValue: log.Int64Value(1_000_000_000), + }, + { + name: "complex64", + value: complex64(complex(float32(1), float32(2))), + wantValue: log.MapValue(log.Float64("r", 1), log.Float64("i", 2)), + }, + { + name: "complex128", + value: complex(float64(3), float64(4)), + wantValue: log.MapValue(log.Float64("r", 3), log.Float64("i", 4)), + }, + { + name: "time.Time", + value: time.Unix(1000, 1000), + wantValue: log.Int64Value(time.Unix(1000, 1000).UnixNano()), + }, + { + name: "[]byte", + value: []byte("hello"), + wantValue: log.BytesValue([]byte("hello")), + }, + { + name: "error", + value: errors.New("test error"), + wantValue: log.StringValue("test error"), + }, + { + name: "error", + value: errors.New("test error"), + wantValue: log.StringValue("test error"), + }, + { + name: "error-nested", + value: fmt.Errorf("test error: %w", errors.New("nested error")), + wantValue: log.StringValue("test error: nested error"), + }, + { + name: "nil", + value: nil, + wantValue: log.Value{}, + }, + { + name: "nil_ptr", + value: (*int)(nil), + wantValue: log.Value{}, + }, + { + name: "int_ptr", + value: func() *int { i := 93; return &i }(), + wantValue: log.Int64Value(93), + }, + { + name: "string_ptr", + value: func() *string { s := "hello"; return &s }(), + wantValue: log.StringValue("hello"), + }, + { + name: "bool_ptr", + value: func() *bool { b := true; return &b }(), + wantValue: log.BoolValue(true), + }, + { + name: "int_empty_array", + value: []int{}, + wantValue: log.SliceValue([]log.Value{}...), + }, + { + name: "int_array", + value: []int{1, 2, 3}, + wantValue: log.SliceValue([]log.Value{ + log.Int64Value(1), + log.Int64Value(2), + log.Int64Value(3), + }...), + }, + { + name: "key_value_map", + value: map[string]int{"one": 1}, + wantValue: log.MapValue( + log.Int64("one", 1), + ), + }, + { + name: "int_string_map", + value: map[int]string{1: "one"}, + wantValue: log.MapValue( + log.String("1", "one"), + ), + }, + { + name: "nested_map", + value: map[string]map[string]int{"nested": {"one": 1}}, + wantValue: log.MapValue( + log.Map("nested", + log.Int64("one", 1), + ), + ), + }, + { + name: "struct_key_map", + value: map[struct{ Name string }]int{ + {Name: "John"}: 42, + }, + wantValue: log.MapValue( + log.Int64("{Name:John}", 42), + ), + }, + { + name: "struct", + value: struct { + Name string + Age int + }{ + Name: "John", + Age: 42, + }, + wantValue: log.StringValue("{Name:John Age:42}"), + }, + { + name: "struct_ptr", + value: &struct { + Name string + Age int + }{ + Name: "John", + Age: 42, + }, + wantValue: log.StringValue("{Name:John Age:42}"), + }, + { + name: "nil_struct_ptr", + value: (*struct { + Name string + Age int + })(nil), + wantValue: log.Value{}, + }, + { + name: "ctx", + value: context.Background(), + wantValue: log.StringValue("context.Background"), + }, + { + name: "unhandled type", + value: chan int(nil), + wantValue: log.StringValue("unhandled: (chan int) "), + }, + } { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + }) + } +} + +func TestConvertValueFloat32(t *testing.T) { + value := ConvertValue(float32(3.14)) + want := log.Float64Value(3.14) + + assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) +} diff --git a/bridges/otellogrus/gen.go b/bridges/otellogrus/gen.go new file mode 100644 index 00000000000..f1813dddb7b --- /dev/null +++ b/bridges/otellogrus/gen.go @@ -0,0 +1,8 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otellogrus // import "go.opentelemetry.io/contrib/bridges/otellogrus" + +// Generate convert: +//go:generate gotmpl --body=../../internal/shared/logutil/convert_test.go.tmpl "--data={ \"pkg\": \"otellogrus\" }" --out=convert_test.go +//go:generate gotmpl --body=../../internal/shared/logutil/convert.go.tmpl "--data={ \"pkg\": \"otellogrus\" }" --out=convert.go diff --git a/bridges/otellogrus/hook.go b/bridges/otellogrus/hook.go index 6ebe85d138f..e069a5baa14 100644 --- a/bridges/otellogrus/hook.go +++ b/bridges/otellogrus/hook.go @@ -33,9 +33,6 @@ package otellogrus // import "go.opentelemetry.io/contrib/bridges/otellogrus" import ( - "fmt" - "reflect" - "github.com/sirupsen/logrus" "go.opentelemetry.io/otel/log" @@ -177,7 +174,7 @@ func convertFields(fields logrus.Fields) []log.KeyValue { for k, v := range fields { kvs = append(kvs, log.KeyValue{ Key: k, - Value: convertValue(v), + Value: ConvertValue(v), }) } return kvs @@ -206,56 +203,3 @@ func convertSeverity(level logrus.Level) log.Severity { return log.SeverityUndefined } } - -func convertValue(v interface{}) log.Value { - switch v := v.(type) { - case bool: - return log.BoolValue(v) - case []byte: - return log.BytesValue(v) - case float64: - return log.Float64Value(v) - case int: - return log.IntValue(v) - case int64: - return log.Int64Value(v) - case string: - return log.StringValue(v) - } - - t := reflect.TypeOf(v) - if t == nil { - return log.Value{} - } - val := reflect.ValueOf(v) - switch t.Kind() { - case reflect.Struct: - return log.StringValue(fmt.Sprintf("%+v", v)) - case reflect.Slice, reflect.Array: - items := make([]log.Value, 0, val.Len()) - for i := 0; i < val.Len(); i++ { - items = append(items, convertValue(val.Index(i).Interface())) - } - return log.SliceValue(items...) - case reflect.Map: - kvs := make([]log.KeyValue, 0, val.Len()) - for _, k := range val.MapKeys() { - var key string - // If the key is a struct, use %+v to print the struct fields. - if k.Kind() == reflect.Struct { - key = fmt.Sprintf("%+v", k.Interface()) - } else { - key = fmt.Sprintf("%v", k.Interface()) - } - kvs = append(kvs, log.KeyValue{ - Key: key, - Value: convertValue(val.MapIndex(k).Interface()), - }) - } - return log.MapValue(kvs...) - case reflect.Ptr, reflect.Interface: - return convertValue(val.Elem().Interface()) - } - - return log.StringValue(fmt.Sprintf("unhandled attribute type: (%s) %+v", t, v)) -} diff --git a/bridges/otellogrus/hook_test.go b/bridges/otellogrus/hook_test.go index 6e95bb93ddd..300c59a0253 100644 --- a/bridges/otellogrus/hook_test.go +++ b/bridges/otellogrus/hook_test.go @@ -148,6 +148,7 @@ func TestHookLevels(t *testing.T) { func TestHookFire(t *testing.T) { const name = "name" now := time.Now() + var nilPointer *struct{} for _, tt := range []struct { name string @@ -269,6 +270,21 @@ func TestHookFire(t *testing.T) { }, }, }, + { + name: "emits a log entry with data containing a nil pointer", + entry: &logrus.Entry{ + Data: logrus.Fields{ + "nil_pointer": nilPointer, + }, + }, + wantRecords: map[string][]log.Record{ + name: { + buildRecord(log.StringValue(""), time.Time{}, log.SeverityFatal4, []log.KeyValue{ + {Key: "nil_pointer", Value: log.Value{}}, + }), + }, + }, + }, } { t.Run(tt.name, func(t *testing.T) { rec := logtest.NewRecorder() diff --git a/bridges/otelzap/convert.go b/bridges/otelzap/convert.go new file mode 100644 index 00000000000..7eb55a57ad4 --- /dev/null +++ b/bridges/otelzap/convert.go @@ -0,0 +1,123 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert.go.tmpl + +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otelzap // import "go.opentelemetry.io/contrib/bridges/otelzap" + +import ( + "fmt" + "math" + "reflect" + "strconv" + "time" + + "go.opentelemetry.io/otel/log" +) + +// Convert various types to log.Value. +func ConvertValue(v any) log.Value { + // Handling the most common types without reflect is a small perf win. + switch val := v.(type) { + case bool: + return log.BoolValue(val) + case string: + return log.StringValue(val) + case int: + return log.Int64Value(int64(val)) + case int8: + return log.Int64Value(int64(val)) + case int16: + return log.Int64Value(int64(val)) + case int32: + return log.Int64Value(int64(val)) + case int64: + return log.Int64Value(val) + case uint: + return convertUintValue(uint64(val)) + case uint8: + return log.Int64Value(int64(val)) + case uint16: + return log.Int64Value(int64(val)) + case uint32: + return log.Int64Value(int64(val)) + case uint64: + return convertUintValue(val) + case uintptr: + return convertUintValue(uint64(val)) + case float32: + return log.Float64Value(float64(val)) + case float64: + return log.Float64Value(val) + case time.Duration: + return log.Int64Value(val.Nanoseconds()) + case complex64: + r := log.Float64("r", real(complex128(val))) + i := log.Float64("i", imag(complex128(val))) + return log.MapValue(r, i) + case complex128: + r := log.Float64("r", real(val)) + i := log.Float64("i", imag(val)) + return log.MapValue(r, i) + case time.Time: + return log.Int64Value(val.UnixNano()) + case []byte: + return log.BytesValue(val) + case error: + return log.StringValue(val.Error()) + } + + t := reflect.TypeOf(v) + if t == nil { + return log.Value{} + } + val := reflect.ValueOf(v) + switch t.Kind() { + case reflect.Struct: + return log.StringValue(fmt.Sprintf("%+v", v)) + case reflect.Slice, reflect.Array: + items := make([]log.Value, 0, val.Len()) + for i := 0; i < val.Len(); i++ { + items = append(items, ConvertValue(val.Index(i).Interface())) + } + return log.SliceValue(items...) + case reflect.Map: + kvs := make([]log.KeyValue, 0, val.Len()) + for _, k := range val.MapKeys() { + var key string + switch k.Kind() { + case reflect.String: + key = k.String() + default: + key = fmt.Sprintf("%+v", k.Interface()) + } + kvs = append(kvs, log.KeyValue{ + Key: key, + Value: ConvertValue(val.MapIndex(k).Interface()), + }) + } + return log.MapValue(kvs...) + case reflect.Ptr, reflect.Interface: + if val.IsNil() { + return log.Value{} + } + return ConvertValue(val.Elem().Interface()) + } + + // Try to handle this as gracefully as possible. + // + // Don't panic here. it is preferable to have user's open issue + // asking why their attributes have a "unhandled: " prefix than + // say that their code is panicking. + return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) +} + +// ConvertUintValue converts a uint64 to a log.Value. +// If the value is too large to fit in an int64, it is converted to a string. +func convertUintValue(v uint64) log.Value { + if v > math.MaxInt64 { + return log.StringValue(strconv.FormatUint(v, 10)) + } + return log.Int64Value(int64(v)) +} diff --git a/bridges/otelzap/convert_test.go b/bridges/otelzap/convert_test.go new file mode 100644 index 00000000000..642ea424a0f --- /dev/null +++ b/bridges/otelzap/convert_test.go @@ -0,0 +1,265 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert_test.go.tmpl + +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otelzap + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/log" +) + +func TestConvertValue(t *testing.T) { + for _, tt := range []struct { + name string + value any + wantValue log.Value + }{ + { + name: "bool", + value: true, + wantValue: log.BoolValue(true), + }, + { + name: "string", + value: "value", + wantValue: log.StringValue("value"), + }, + { + name: "int", + value: 10, + wantValue: log.Int64Value(10), + }, + { + name: "int8", + value: int8(127), + wantValue: log.Int64Value(127), + }, + { + name: "int16", + value: int16(32767), + wantValue: log.Int64Value(32767), + }, + { + name: "int32", + value: int32(2147483647), + wantValue: log.Int64Value(2147483647), + }, + { + name: "int64", + value: int64(9223372036854775807), + wantValue: log.Int64Value(9223372036854775807), + }, + { + name: "uint", + value: uint(42), + wantValue: log.Int64Value(42), + }, + { + name: "uint8", + value: uint8(255), + wantValue: log.Int64Value(255), + }, + { + name: "uint16", + value: uint16(65535), + wantValue: log.Int64Value(65535), + }, + { + name: "uint32", + value: uint32(4294967295), + wantValue: log.Int64Value(4294967295), + }, + { + name: "uint64", + value: uint64(9223372036854775807), + wantValue: log.Int64Value(9223372036854775807), + }, + { + name: "uint64-max", + value: uint64(18446744073709551615), + wantValue: log.StringValue("18446744073709551615"), + }, + { + name: "uintptr", + value: uintptr(12345), + wantValue: log.Int64Value(12345), + }, + { + name: "float64", + value: float64(3.14159), + wantValue: log.Float64Value(3.14159), + }, + { + name: "time.Duration", + value: time.Second, + wantValue: log.Int64Value(1_000_000_000), + }, + { + name: "complex64", + value: complex64(complex(float32(1), float32(2))), + wantValue: log.MapValue(log.Float64("r", 1), log.Float64("i", 2)), + }, + { + name: "complex128", + value: complex(float64(3), float64(4)), + wantValue: log.MapValue(log.Float64("r", 3), log.Float64("i", 4)), + }, + { + name: "time.Time", + value: time.Unix(1000, 1000), + wantValue: log.Int64Value(time.Unix(1000, 1000).UnixNano()), + }, + { + name: "[]byte", + value: []byte("hello"), + wantValue: log.BytesValue([]byte("hello")), + }, + { + name: "error", + value: errors.New("test error"), + wantValue: log.StringValue("test error"), + }, + { + name: "error", + value: errors.New("test error"), + wantValue: log.StringValue("test error"), + }, + { + name: "error-nested", + value: fmt.Errorf("test error: %w", errors.New("nested error")), + wantValue: log.StringValue("test error: nested error"), + }, + { + name: "nil", + value: nil, + wantValue: log.Value{}, + }, + { + name: "nil_ptr", + value: (*int)(nil), + wantValue: log.Value{}, + }, + { + name: "int_ptr", + value: func() *int { i := 93; return &i }(), + wantValue: log.Int64Value(93), + }, + { + name: "string_ptr", + value: func() *string { s := "hello"; return &s }(), + wantValue: log.StringValue("hello"), + }, + { + name: "bool_ptr", + value: func() *bool { b := true; return &b }(), + wantValue: log.BoolValue(true), + }, + { + name: "int_empty_array", + value: []int{}, + wantValue: log.SliceValue([]log.Value{}...), + }, + { + name: "int_array", + value: []int{1, 2, 3}, + wantValue: log.SliceValue([]log.Value{ + log.Int64Value(1), + log.Int64Value(2), + log.Int64Value(3), + }...), + }, + { + name: "key_value_map", + value: map[string]int{"one": 1}, + wantValue: log.MapValue( + log.Int64("one", 1), + ), + }, + { + name: "int_string_map", + value: map[int]string{1: "one"}, + wantValue: log.MapValue( + log.String("1", "one"), + ), + }, + { + name: "nested_map", + value: map[string]map[string]int{"nested": {"one": 1}}, + wantValue: log.MapValue( + log.Map("nested", + log.Int64("one", 1), + ), + ), + }, + { + name: "struct_key_map", + value: map[struct{ Name string }]int{ + {Name: "John"}: 42, + }, + wantValue: log.MapValue( + log.Int64("{Name:John}", 42), + ), + }, + { + name: "struct", + value: struct { + Name string + Age int + }{ + Name: "John", + Age: 42, + }, + wantValue: log.StringValue("{Name:John Age:42}"), + }, + { + name: "struct_ptr", + value: &struct { + Name string + Age int + }{ + Name: "John", + Age: 42, + }, + wantValue: log.StringValue("{Name:John Age:42}"), + }, + { + name: "nil_struct_ptr", + value: (*struct { + Name string + Age int + })(nil), + wantValue: log.Value{}, + }, + { + name: "ctx", + value: context.Background(), + wantValue: log.StringValue("context.Background"), + }, + { + name: "unhandled type", + value: chan int(nil), + wantValue: log.StringValue("unhandled: (chan int) "), + }, + } { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + }) + } +} + +func TestConvertValueFloat32(t *testing.T) { + value := ConvertValue(float32(3.14)) + want := log.Float64Value(3.14) + + assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) +} diff --git a/bridges/otelzap/encoder.go b/bridges/otelzap/encoder.go index a5ddfb48d32..c34d8b687e2 100644 --- a/bridges/otelzap/encoder.go +++ b/bridges/otelzap/encoder.go @@ -4,8 +4,6 @@ package otelzap // import "go.opentelemetry.io/contrib/bridges/otelzap" import ( - "fmt" - "reflect" "time" "go.uber.org/zap/zapcore" @@ -121,7 +119,7 @@ func (m *objectEncoder) AddReflected(k string, v interface{}) error { m.cur.attrs = append(m.cur.attrs, log.KeyValue{ Key: k, - Value: convertValue(v), + Value: ConvertValue(v), }) return nil } @@ -221,7 +219,7 @@ func (a *arrayEncoder) AppendObject(v zapcore.ObjectMarshaler) error { } func (a *arrayEncoder) AppendReflected(v interface{}) error { - a.elems = append(a.elems, convertValue(v)) + a.elems = append(a.elems, ConvertValue(v)) return nil } @@ -274,56 +272,3 @@ func (a *arrayEncoder) AppendUint32(v uint32) { a.AppendInt64(int64(v)) func (a *arrayEncoder) AppendUint16(v uint16) { a.AppendInt64(int64(v)) } func (a *arrayEncoder) AppendUint8(v uint8) { a.AppendInt64(int64(v)) } func (a *arrayEncoder) AppendUintptr(v uintptr) { a.AppendUint64(uint64(v)) } - -func convertValue(v interface{}) log.Value { - switch v := v.(type) { - case bool: - return log.BoolValue(v) - case []byte: - return log.BytesValue(v) - case float64: - return log.Float64Value(v) - case int: - return log.IntValue(v) - case int64: - return log.Int64Value(v) - case string: - return log.StringValue(v) - } - - t := reflect.TypeOf(v) - if t == nil { - return log.Value{} - } - val := reflect.ValueOf(v) - switch t.Kind() { - case reflect.Struct: - return log.StringValue(fmt.Sprintf("%+v", v)) - case reflect.Slice, reflect.Array: - items := make([]log.Value, 0, val.Len()) - for i := 0; i < val.Len(); i++ { - items = append(items, convertValue(val.Index(i).Interface())) - } - return log.SliceValue(items...) - case reflect.Map: - kvs := make([]log.KeyValue, 0, val.Len()) - for _, k := range val.MapKeys() { - var key string - // If the key is a struct, use %+v to print the struct fields. - if k.Kind() == reflect.Struct { - key = fmt.Sprintf("%+v", k.Interface()) - } else { - key = fmt.Sprintf("%v", k.Interface()) - } - kvs = append(kvs, log.KeyValue{ - Key: key, - Value: convertValue(val.MapIndex(k).Interface()), - }) - } - return log.MapValue(kvs...) - case reflect.Ptr, reflect.Interface: - return convertValue(val.Elem().Interface()) - } - - return log.StringValue(fmt.Sprintf("unhandled attribute type: (%s) %+v", t, v)) -} diff --git a/bridges/otelzap/encoder_test.go b/bridges/otelzap/encoder_test.go index 4e284b82d35..ba3d726e3c6 100644 --- a/bridges/otelzap/encoder_test.go +++ b/bridges/otelzap/encoder_test.go @@ -72,6 +72,14 @@ func TestObjectEncoder(t *testing.T) { }, expected: map[string]interface{}{"foo": int64(5)}, }, + { + desc: "AddReflected (nil pointer)", + f: func(e zapcore.ObjectEncoder) { + var p *struct{} + assert.NoError(t, e.AddReflected("k", p), "Expected AddReflected to succeed.") + }, + expected: nil, + }, { desc: "AddBinary", f: func(e zapcore.ObjectEncoder) { e.AddBinary("k", []byte("foo")) }, diff --git a/bridges/otelzap/gen.go b/bridges/otelzap/gen.go new file mode 100644 index 00000000000..5c8b2eea7e4 --- /dev/null +++ b/bridges/otelzap/gen.go @@ -0,0 +1,8 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package otelzap // import "go.opentelemetry.io/contrib/bridges/otelzap" + +// Generate convert: +//go:generate gotmpl --body=../../internal/shared/logutil/convert_test.go.tmpl "--data={ \"pkg\": \"otelzap\" }" --out=convert_test.go +//go:generate gotmpl --body=../../internal/shared/logutil/convert.go.tmpl "--data={ \"pkg\": \"otelzap\" }" --out=convert.go diff --git a/internal/shared/logutil/convert.go.tmpl b/internal/shared/logutil/convert.go.tmpl new file mode 100644 index 00000000000..4379cfa314e --- /dev/null +++ b/internal/shared/logutil/convert.go.tmpl @@ -0,0 +1,123 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert.go.tmpl + +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package {{.pkg}} // import "go.opentelemetry.io/contrib/bridges/{{.pkg}}" + +import ( + "fmt" + "math" + "reflect" + "strconv" + "time" + + "go.opentelemetry.io/otel/log" +) + +// Convert various types to log.Value. +func ConvertValue(v any) log.Value { + // Handling the most common types without reflect is a small perf win. + switch val := v.(type) { + case bool: + return log.BoolValue(val) + case string: + return log.StringValue(val) + case int: + return log.Int64Value(int64(val)) + case int8: + return log.Int64Value(int64(val)) + case int16: + return log.Int64Value(int64(val)) + case int32: + return log.Int64Value(int64(val)) + case int64: + return log.Int64Value(val) + case uint: + return convertUintValue(uint64(val)) + case uint8: + return log.Int64Value(int64(val)) + case uint16: + return log.Int64Value(int64(val)) + case uint32: + return log.Int64Value(int64(val)) + case uint64: + return convertUintValue(val) + case uintptr: + return convertUintValue(uint64(val)) + case float32: + return log.Float64Value(float64(val)) + case float64: + return log.Float64Value(val) + case time.Duration: + return log.Int64Value(val.Nanoseconds()) + case complex64: + r := log.Float64("r", real(complex128(val))) + i := log.Float64("i", imag(complex128(val))) + return log.MapValue(r, i) + case complex128: + r := log.Float64("r", real(val)) + i := log.Float64("i", imag(val)) + return log.MapValue(r, i) + case time.Time: + return log.Int64Value(val.UnixNano()) + case []byte: + return log.BytesValue(val) + case error: + return log.StringValue(val.Error()) + } + + t := reflect.TypeOf(v) + if t == nil { + return log.Value{} + } + val := reflect.ValueOf(v) + switch t.Kind() { + case reflect.Struct: + return log.StringValue(fmt.Sprintf("%+v", v)) + case reflect.Slice, reflect.Array: + items := make([]log.Value, 0, val.Len()) + for i := 0; i < val.Len(); i++ { + items = append(items, ConvertValue(val.Index(i).Interface())) + } + return log.SliceValue(items...) + case reflect.Map: + kvs := make([]log.KeyValue, 0, val.Len()) + for _, k := range val.MapKeys() { + var key string + switch k.Kind() { + case reflect.String: + key = k.String() + default: + key = fmt.Sprintf("%+v", k.Interface()) + } + kvs = append(kvs, log.KeyValue{ + Key: key, + Value: ConvertValue(val.MapIndex(k).Interface()), + }) + } + return log.MapValue(kvs...) + case reflect.Ptr, reflect.Interface: + if val.IsNil() { + return log.Value{} + } + return ConvertValue(val.Elem().Interface()) + } + + // Try to handle this as gracefully as possible. + // + // Don't panic here. it is preferable to have user's open issue + // asking why their attributes have a "unhandled: " prefix than + // say that their code is panicking. + return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) +} + +// ConvertUintValue converts a uint64 to a log.Value. +// If the value is too large to fit in an int64, it is converted to a string. +func convertUintValue(v uint64) log.Value { + if v > math.MaxInt64 { + return log.StringValue(strconv.FormatUint(v, 10)) + } + return log.Int64Value(int64(v)) +} diff --git a/internal/shared/logutil/convert_test.go.tmpl b/internal/shared/logutil/convert_test.go.tmpl new file mode 100644 index 00000000000..7317038a811 --- /dev/null +++ b/internal/shared/logutil/convert_test.go.tmpl @@ -0,0 +1,265 @@ +// Code created by gotmpl. DO NOT MODIFY. +// source: internal/shared/logutil/convert_test.go.tmpl + +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package {{.pkg}} + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "go.opentelemetry.io/otel/log" +) + +func TestConvertValue(t *testing.T) { + for _, tt := range []struct { + name string + value any + wantValue log.Value + }{ + { + name: "bool", + value: true, + wantValue: log.BoolValue(true), + }, + { + name: "string", + value: "value", + wantValue: log.StringValue("value"), + }, + { + name: "int", + value: 10, + wantValue: log.Int64Value(10), + }, + { + name: "int8", + value: int8(127), + wantValue: log.Int64Value(127), + }, + { + name: "int16", + value: int16(32767), + wantValue: log.Int64Value(32767), + }, + { + name: "int32", + value: int32(2147483647), + wantValue: log.Int64Value(2147483647), + }, + { + name: "int64", + value: int64(9223372036854775807), + wantValue: log.Int64Value(9223372036854775807), + }, + { + name: "uint", + value: uint(42), + wantValue: log.Int64Value(42), + }, + { + name: "uint8", + value: uint8(255), + wantValue: log.Int64Value(255), + }, + { + name: "uint16", + value: uint16(65535), + wantValue: log.Int64Value(65535), + }, + { + name: "uint32", + value: uint32(4294967295), + wantValue: log.Int64Value(4294967295), + }, + { + name: "uint64", + value: uint64(9223372036854775807), + wantValue: log.Int64Value(9223372036854775807), + }, + { + name: "uint64-max", + value: uint64(18446744073709551615), + wantValue: log.StringValue("18446744073709551615"), + }, + { + name: "uintptr", + value: uintptr(12345), + wantValue: log.Int64Value(12345), + }, + { + name: "float64", + value: float64(3.14159), + wantValue: log.Float64Value(3.14159), + }, + { + name: "time.Duration", + value: time.Second, + wantValue: log.Int64Value(1_000_000_000), + }, + { + name: "complex64", + value: complex64(complex(float32(1), float32(2))), + wantValue: log.MapValue(log.Float64("r", 1), log.Float64("i", 2)), + }, + { + name: "complex128", + value: complex(float64(3), float64(4)), + wantValue: log.MapValue(log.Float64("r", 3), log.Float64("i", 4)), + }, + { + name: "time.Time", + value: time.Unix(1000, 1000), + wantValue: log.Int64Value(time.Unix(1000, 1000).UnixNano()), + }, + { + name: "[]byte", + value: []byte("hello"), + wantValue: log.BytesValue([]byte("hello")), + }, + { + name: "error", + value: errors.New("test error"), + wantValue: log.StringValue("test error"), + }, + { + name: "error", + value: errors.New("test error"), + wantValue: log.StringValue("test error"), + }, + { + name: "error-nested", + value: fmt.Errorf("test error: %w", errors.New("nested error")), + wantValue: log.StringValue("test error: nested error"), + }, + { + name: "nil", + value: nil, + wantValue: log.Value{}, + }, + { + name: "nil_ptr", + value: (*int)(nil), + wantValue: log.Value{}, + }, + { + name: "int_ptr", + value: func() *int { i := 93; return &i }(), + wantValue: log.Int64Value(93), + }, + { + name: "string_ptr", + value: func() *string { s := "hello"; return &s }(), + wantValue: log.StringValue("hello"), + }, + { + name: "bool_ptr", + value: func() *bool { b := true; return &b }(), + wantValue: log.BoolValue(true), + }, + { + name: "int_empty_array", + value: []int{}, + wantValue: log.SliceValue([]log.Value{}...), + }, + { + name: "int_array", + value: []int{1, 2, 3}, + wantValue: log.SliceValue([]log.Value{ + log.Int64Value(1), + log.Int64Value(2), + log.Int64Value(3), + }...), + }, + { + name: "key_value_map", + value: map[string]int{"one": 1}, + wantValue: log.MapValue( + log.Int64("one", 1), + ), + }, + { + name: "int_string_map", + value: map[int]string{1: "one"}, + wantValue: log.MapValue( + log.String("1", "one"), + ), + }, + { + name: "nested_map", + value: map[string]map[string]int{"nested": {"one": 1}}, + wantValue: log.MapValue( + log.Map("nested", + log.Int64("one", 1), + ), + ), + }, + { + name: "struct_key_map", + value: map[struct{ Name string }]int{ + {Name: "John"}: 42, + }, + wantValue: log.MapValue( + log.Int64("{Name:John}", 42), + ), + }, + { + name: "struct", + value: struct { + Name string + Age int + }{ + Name: "John", + Age: 42, + }, + wantValue: log.StringValue("{Name:John Age:42}"), + }, + { + name: "struct_ptr", + value: &struct { + Name string + Age int + }{ + Name: "John", + Age: 42, + }, + wantValue: log.StringValue("{Name:John Age:42}"), + }, + { + name: "nil_struct_ptr", + value: (*struct { + Name string + Age int + })(nil), + wantValue: log.Value{}, + }, + { + name: "ctx", + value: context.Background(), + wantValue: log.StringValue("context.Background"), + }, + { + name: "unhandled type", + value: chan int(nil), + wantValue: log.StringValue("unhandled: (chan int) "), + }, + } { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + }) + } +} + +func TestConvertValueFloat32(t *testing.T) { + value := ConvertValue(float32(3.14)) + want := log.Float64Value(3.14) + + assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) +} From fc38d6068b9cd7c9e9bc325fce7fe63bab61a92b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Mon, 14 Oct 2024 18:24:03 +0200 Subject: [PATCH 2/5] Update CHANGELOG.md --- CHANGELOG.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3c71229431..e3b36f9b0d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Fixed + +- Transform nil attribute values to `log.Value` zero value instead of panicking in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6237) +- Transform nil attribute values to `log.Value` zero value instead of panicking in `go.opentelemetry.io/contrib/bridges/otelzap`. (#6237) + @@ -28,14 +33,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `NewSDK` in `go.opentelemetry.io/contrib/config` now returns a no-op SDK if `disabled` is set to `true`. (#6185) - The deprecated `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho` package has found a Code Owner. The package is no longer deprecated. (#6207) -- Aligned convertvalue in `otellogr`, `otellogrus` and `otelzap` with convert tmpl. (#6237) + ### Fixed - Possible nil dereference panic in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#5965) - `logrus.Level` transformed to appropriate `log.Severity` in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6191) -- Possible nil dereference panic in `go.opentelemetry.io/contrib/bridges/otellogrus`. (#6237) -- Possible nil dereference panic in `go.opentelemetry.io/contrib/bridges/otelzap`. (#6237) ### Removed From b9422f108269709be538379f66a73bc96a35ae93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Mon, 14 Oct 2024 18:27:23 +0200 Subject: [PATCH 3/5] Update CHANGELOG.md --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3b36f9b0d0..de3e71af1b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,6 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The deprecated `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho` package has found a Code Owner. The package is no longer deprecated. (#6207) - ### Fixed - Possible nil dereference panic in `go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`. (#5965) From 9bca788f0be3237fbe3001322b23e1f36f6c8d73 Mon Sep 17 00:00:00 2001 From: m1heng <18018422+m1heng@users.noreply.github.com> Date: Tue, 15 Oct 2024 00:30:20 +0800 Subject: [PATCH 4/5] Update internal/shared/logutil/convert.go.tmpl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- internal/shared/logutil/convert.go.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/shared/logutil/convert.go.tmpl b/internal/shared/logutil/convert.go.tmpl index 4379cfa314e..d28f79decdb 100644 --- a/internal/shared/logutil/convert.go.tmpl +++ b/internal/shared/logutil/convert.go.tmpl @@ -113,7 +113,7 @@ func ConvertValue(v any) log.Value { return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) } -// ConvertUintValue converts a uint64 to a log.Value. +// convertUintValue converts a uint64 to a log.Value. // If the value is too large to fit in an int64, it is converted to a string. func convertUintValue(v uint64) log.Value { if v > math.MaxInt64 { From 27601bc19c0e1c0af8052add1ac5eac446cecf29 Mon Sep 17 00:00:00 2001 From: m1heng <18018422+m1heng@users.noreply.github.com> Date: Tue, 15 Oct 2024 00:30:30 +0800 Subject: [PATCH 5/5] Update internal/shared/logutil/convert.go.tmpl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Robert Pająk --- bridges/otellogr/convert.go | 12 ++++++------ bridges/otellogr/convert_test.go | 4 ++-- bridges/otellogr/logsink.go | 2 +- bridges/otellogrus/convert.go | 12 ++++++------ bridges/otellogrus/convert_test.go | 4 ++-- bridges/otellogrus/hook.go | 2 +- bridges/otelzap/convert.go | 12 ++++++------ bridges/otelzap/convert_test.go | 4 ++-- bridges/otelzap/encoder.go | 4 ++-- internal/shared/logutil/convert.go.tmpl | 10 +++++----- internal/shared/logutil/convert_test.go.tmpl | 4 ++-- 11 files changed, 35 insertions(+), 35 deletions(-) diff --git a/bridges/otellogr/convert.go b/bridges/otellogr/convert.go index b703ad23535..cdf2f005813 100644 --- a/bridges/otellogr/convert.go +++ b/bridges/otellogr/convert.go @@ -16,8 +16,8 @@ import ( "go.opentelemetry.io/otel/log" ) -// Convert various types to log.Value. -func ConvertValue(v any) log.Value { +// convertValue converts various types to log.Value. +func convertValue(v any) log.Value { // Handling the most common types without reflect is a small perf win. switch val := v.(type) { case bool: @@ -79,7 +79,7 @@ func ConvertValue(v any) log.Value { case reflect.Slice, reflect.Array: items := make([]log.Value, 0, val.Len()) for i := 0; i < val.Len(); i++ { - items = append(items, ConvertValue(val.Index(i).Interface())) + items = append(items, convertValue(val.Index(i).Interface())) } return log.SliceValue(items...) case reflect.Map: @@ -94,7 +94,7 @@ func ConvertValue(v any) log.Value { } kvs = append(kvs, log.KeyValue{ Key: key, - Value: ConvertValue(val.MapIndex(k).Interface()), + Value: convertValue(val.MapIndex(k).Interface()), }) } return log.MapValue(kvs...) @@ -102,7 +102,7 @@ func ConvertValue(v any) log.Value { if val.IsNil() { return log.Value{} } - return ConvertValue(val.Elem().Interface()) + return convertValue(val.Elem().Interface()) } // Try to handle this as gracefully as possible. @@ -113,7 +113,7 @@ func ConvertValue(v any) log.Value { return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) } -// ConvertUintValue converts a uint64 to a log.Value. +// convertUintValue converts a uint64 to a log.Value. // If the value is too large to fit in an int64, it is converted to a string. func convertUintValue(v uint64) log.Value { if v > math.MaxInt64 { diff --git a/bridges/otellogr/convert_test.go b/bridges/otellogr/convert_test.go index c0e1a3d1d7f..1a852defbe0 100644 --- a/bridges/otellogr/convert_test.go +++ b/bridges/otellogr/convert_test.go @@ -252,13 +252,13 @@ func TestConvertValue(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + assert.Equal(t, tt.wantValue, convertValue(tt.value)) }) } } func TestConvertValueFloat32(t *testing.T) { - value := ConvertValue(float32(3.14)) + value := convertValue(float32(3.14)) want := log.Float64Value(3.14) assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) diff --git a/bridges/otellogr/logsink.go b/bridges/otellogr/logsink.go index 5d473359e7d..482314c8719 100644 --- a/bridges/otellogr/logsink.go +++ b/bridges/otellogr/logsink.go @@ -229,7 +229,7 @@ func convertKVs(ctx context.Context, keysAndValues ...any) (context.Context, []l kvs = append(kvs, log.KeyValue{ Key: k, - Value: ConvertValue(v), + Value: convertValue(v), }) } diff --git a/bridges/otellogrus/convert.go b/bridges/otellogrus/convert.go index 3fa5101d500..01307719222 100644 --- a/bridges/otellogrus/convert.go +++ b/bridges/otellogrus/convert.go @@ -16,8 +16,8 @@ import ( "go.opentelemetry.io/otel/log" ) -// Convert various types to log.Value. -func ConvertValue(v any) log.Value { +// convertValue converts various types to log.Value. +func convertValue(v any) log.Value { // Handling the most common types without reflect is a small perf win. switch val := v.(type) { case bool: @@ -79,7 +79,7 @@ func ConvertValue(v any) log.Value { case reflect.Slice, reflect.Array: items := make([]log.Value, 0, val.Len()) for i := 0; i < val.Len(); i++ { - items = append(items, ConvertValue(val.Index(i).Interface())) + items = append(items, convertValue(val.Index(i).Interface())) } return log.SliceValue(items...) case reflect.Map: @@ -94,7 +94,7 @@ func ConvertValue(v any) log.Value { } kvs = append(kvs, log.KeyValue{ Key: key, - Value: ConvertValue(val.MapIndex(k).Interface()), + Value: convertValue(val.MapIndex(k).Interface()), }) } return log.MapValue(kvs...) @@ -102,7 +102,7 @@ func ConvertValue(v any) log.Value { if val.IsNil() { return log.Value{} } - return ConvertValue(val.Elem().Interface()) + return convertValue(val.Elem().Interface()) } // Try to handle this as gracefully as possible. @@ -113,7 +113,7 @@ func ConvertValue(v any) log.Value { return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) } -// ConvertUintValue converts a uint64 to a log.Value. +// convertUintValue converts a uint64 to a log.Value. // If the value is too large to fit in an int64, it is converted to a string. func convertUintValue(v uint64) log.Value { if v > math.MaxInt64 { diff --git a/bridges/otellogrus/convert_test.go b/bridges/otellogrus/convert_test.go index 329d3a3bd1e..ad0b30a3668 100644 --- a/bridges/otellogrus/convert_test.go +++ b/bridges/otellogrus/convert_test.go @@ -252,13 +252,13 @@ func TestConvertValue(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + assert.Equal(t, tt.wantValue, convertValue(tt.value)) }) } } func TestConvertValueFloat32(t *testing.T) { - value := ConvertValue(float32(3.14)) + value := convertValue(float32(3.14)) want := log.Float64Value(3.14) assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) diff --git a/bridges/otellogrus/hook.go b/bridges/otellogrus/hook.go index e069a5baa14..16dbc607926 100644 --- a/bridges/otellogrus/hook.go +++ b/bridges/otellogrus/hook.go @@ -174,7 +174,7 @@ func convertFields(fields logrus.Fields) []log.KeyValue { for k, v := range fields { kvs = append(kvs, log.KeyValue{ Key: k, - Value: ConvertValue(v), + Value: convertValue(v), }) } return kvs diff --git a/bridges/otelzap/convert.go b/bridges/otelzap/convert.go index 7eb55a57ad4..6f64c794b76 100644 --- a/bridges/otelzap/convert.go +++ b/bridges/otelzap/convert.go @@ -16,8 +16,8 @@ import ( "go.opentelemetry.io/otel/log" ) -// Convert various types to log.Value. -func ConvertValue(v any) log.Value { +// convertValue converts various types to log.Value. +func convertValue(v any) log.Value { // Handling the most common types without reflect is a small perf win. switch val := v.(type) { case bool: @@ -79,7 +79,7 @@ func ConvertValue(v any) log.Value { case reflect.Slice, reflect.Array: items := make([]log.Value, 0, val.Len()) for i := 0; i < val.Len(); i++ { - items = append(items, ConvertValue(val.Index(i).Interface())) + items = append(items, convertValue(val.Index(i).Interface())) } return log.SliceValue(items...) case reflect.Map: @@ -94,7 +94,7 @@ func ConvertValue(v any) log.Value { } kvs = append(kvs, log.KeyValue{ Key: key, - Value: ConvertValue(val.MapIndex(k).Interface()), + Value: convertValue(val.MapIndex(k).Interface()), }) } return log.MapValue(kvs...) @@ -102,7 +102,7 @@ func ConvertValue(v any) log.Value { if val.IsNil() { return log.Value{} } - return ConvertValue(val.Elem().Interface()) + return convertValue(val.Elem().Interface()) } // Try to handle this as gracefully as possible. @@ -113,7 +113,7 @@ func ConvertValue(v any) log.Value { return log.StringValue(fmt.Sprintf("unhandled: (%s) %+v", t, v)) } -// ConvertUintValue converts a uint64 to a log.Value. +// convertUintValue converts a uint64 to a log.Value. // If the value is too large to fit in an int64, it is converted to a string. func convertUintValue(v uint64) log.Value { if v > math.MaxInt64 { diff --git a/bridges/otelzap/convert_test.go b/bridges/otelzap/convert_test.go index 642ea424a0f..79d41de30b0 100644 --- a/bridges/otelzap/convert_test.go +++ b/bridges/otelzap/convert_test.go @@ -252,13 +252,13 @@ func TestConvertValue(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + assert.Equal(t, tt.wantValue, convertValue(tt.value)) }) } } func TestConvertValueFloat32(t *testing.T) { - value := ConvertValue(float32(3.14)) + value := convertValue(float32(3.14)) want := log.Float64Value(3.14) assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001) diff --git a/bridges/otelzap/encoder.go b/bridges/otelzap/encoder.go index c34d8b687e2..1144ec579a5 100644 --- a/bridges/otelzap/encoder.go +++ b/bridges/otelzap/encoder.go @@ -119,7 +119,7 @@ func (m *objectEncoder) AddReflected(k string, v interface{}) error { m.cur.attrs = append(m.cur.attrs, log.KeyValue{ Key: k, - Value: ConvertValue(v), + Value: convertValue(v), }) return nil } @@ -219,7 +219,7 @@ func (a *arrayEncoder) AppendObject(v zapcore.ObjectMarshaler) error { } func (a *arrayEncoder) AppendReflected(v interface{}) error { - a.elems = append(a.elems, ConvertValue(v)) + a.elems = append(a.elems, convertValue(v)) return nil } diff --git a/internal/shared/logutil/convert.go.tmpl b/internal/shared/logutil/convert.go.tmpl index d28f79decdb..22be1e2a62a 100644 --- a/internal/shared/logutil/convert.go.tmpl +++ b/internal/shared/logutil/convert.go.tmpl @@ -16,8 +16,8 @@ import ( "go.opentelemetry.io/otel/log" ) -// Convert various types to log.Value. -func ConvertValue(v any) log.Value { +// convertValue converts various types to log.Value. +func convertValue(v any) log.Value { // Handling the most common types without reflect is a small perf win. switch val := v.(type) { case bool: @@ -79,7 +79,7 @@ func ConvertValue(v any) log.Value { case reflect.Slice, reflect.Array: items := make([]log.Value, 0, val.Len()) for i := 0; i < val.Len(); i++ { - items = append(items, ConvertValue(val.Index(i).Interface())) + items = append(items, convertValue(val.Index(i).Interface())) } return log.SliceValue(items...) case reflect.Map: @@ -94,7 +94,7 @@ func ConvertValue(v any) log.Value { } kvs = append(kvs, log.KeyValue{ Key: key, - Value: ConvertValue(val.MapIndex(k).Interface()), + Value: convertValue(val.MapIndex(k).Interface()), }) } return log.MapValue(kvs...) @@ -102,7 +102,7 @@ func ConvertValue(v any) log.Value { if val.IsNil() { return log.Value{} } - return ConvertValue(val.Elem().Interface()) + return convertValue(val.Elem().Interface()) } // Try to handle this as gracefully as possible. diff --git a/internal/shared/logutil/convert_test.go.tmpl b/internal/shared/logutil/convert_test.go.tmpl index 7317038a811..0e44527dec6 100644 --- a/internal/shared/logutil/convert_test.go.tmpl +++ b/internal/shared/logutil/convert_test.go.tmpl @@ -252,13 +252,13 @@ func TestConvertValue(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.wantValue, ConvertValue(tt.value)) + assert.Equal(t, tt.wantValue, convertValue(tt.value)) }) } } func TestConvertValueFloat32(t *testing.T) { - value := ConvertValue(float32(3.14)) + value := convertValue(float32(3.14)) want := log.Float64Value(3.14) assert.InDelta(t, value.AsFloat64(), want.AsFloat64(), 0.0001)