Move convert values to internal/shared/logutil

open-telemetry · Oct 14, 2024 · a1c5708 · a1c5708
1 parent d6305c0
commit a1c5708
Show file tree

Hide file tree

Showing 18 changed files with 1,362 additions and 222 deletions.
diff --git 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
@@ -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,15 +94,15 @@ 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...)
 	case reflect.Ptr, reflect.Interface:
 		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
@@ -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) <nil>"),
+		},
 	} {
 		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
@@ -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
@@ -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
@@ -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)
+		})
+	}
+}