Repository: open-telemetry/opentelemetry-go
Pull request #5306 (2024-05-07 08:20:53)

log: Fix comparison of unordered map values
Map attributes should be equal even if their order is different, which can cause issues, for example in https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5356.

```goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/log
                                                                               │     bench-main     │             bench-branch              │
                                                                               │       sec/op       │       sec/op        vs base           │
ValueEqual/[l:[3_foo]_b:[3_5_7]_e:<nil>]_with_[l:[3_foo]_b:[3_5_7]_e:<nil>]-10   0.000001100n ± ∞ ¹   0.000001700n ± ∞ ¹  ~ (p=1.000 n=1) ²
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                                                                               │ bench-main  │          bench-branch          │
                                                                               │    B/op     │    B/op      vs base           │
ValueEqual/[l:[3_foo]_b:[3_5_7]_e:<nil>]_with_[l:[3_foo]_b:[3_5_7]_e:<nil>]-10   0.000 ± ∞ ¹   0.000 ± ∞ ¹  ~ (p=1.000 n=1) ²
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal

                                                                               │ bench-main  │          bench-branch          │
                                                                               │  allocs/op  │  allocs/op   vs base           │
ValueEqual/[l:[3_foo]_b:[3_5_7]_e:<nil>]_with_[l:[3_foo]_b:[3_5_7]_e:<nil>]-10   0.000 ± ∞ ¹   0.000 ± ∞ ¹  ~ (p=1.000 n=1) ²
¹ need >= 6 samples for confidence interval at level 0.95
² all samples are equal
```

dmathieu: My linked PR for logrus has different orders based on the OS (my mac, and linux on CI), which caused failures.
MrAlias: > My linked PR for logrus has different orders based on the OS (my mac, and linux on CI), which caused failures.

Can you sort your slice values before creating the attribute?
pellared: > Can you sort your slice values before creating the attribute?

Or sort in tests if for logrus the order is not preserved.
dmathieu: Actually, my failing logrus test is because of a map, not a slice. So I have removed slice sorting from here.
https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5356/files#diff-f54db292144cf7bd4ae4be9b3f4b703416a1352cf27304bca223d8cf00c90dc5R339-R345
pellared: Can you update the benchmark results? I believe they will be different 😉 
dmathieu: I have updated them (with only map->map comparison to ease reading). Surprisingly, the difference is very small.
MrAlias: @MadVikingGod PTAL

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 61db0af8..b0658330 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -31,6 +31,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ### Fixed
 
 - Fix the empty output of `go.opentelemetry.io/otel/log.Value` in `go.opentelemetry.io/otel/exporters/stdout/stdoutlog`. (#5311)
+- Comparison of unordered maps in `go.opentelemetry.io/otel/log.KeyValue` and `go.opentelemetry.io/otel/log.Value`. (#5306)
 
 ## [1.26.0/0.48.0/0.2.0-alpha] 2024-04-24
 
diff --git a/log/keyvalue.go b/log/keyvalue.go
index 10920d21..8258defe 100644
--- a/log/keyvalue.go
+++ b/log/keyvalue.go
@@ -7,6 +7,7 @@ package log // import "go.opentelemetry.io/otel/log"
 
 import (
 	"bytes"
+	"cmp"
 	"errors"
 	"fmt"
 	"math"
@@ -256,7 +257,9 @@ func (v Value) Equal(w Value) bool {
 	case KindSlice:
 		return slices.EqualFunc(v.asSlice(), w.asSlice(), Value.Equal)
 	case KindMap:
-		return slices.EqualFunc(v.asMap(), w.asMap(), KeyValue.Equal)
+		sv := sortMap(v.asMap())
+		sw := sortMap(w.asMap())
+		return slices.EqualFunc(sv, sw, KeyValue.Equal)
 	case KindBytes:
 		return bytes.Equal(v.asBytes(), w.asBytes())
 	case KindEmpty:
@@ -267,6 +270,16 @@ func (v Value) Equal(w Value) bool {
 	}
 }
 
+func sortMap(m []KeyValue) []KeyValue {
+	sm := make([]KeyValue, len(m))
+	copy(sm, m)
+	slices.SortFunc(sm, func(a, b KeyValue) int {
+		return cmp.Compare(a.Key, b.Key)
+	})
+
+	return sm
+}
+
 // String returns Value's value as a string, formatted like [fmt.Sprint].
 //
 // The returned string is meant for debugging;


Test patch:
diff --git a/log/keyvalue_bench_test.go b/log/keyvalue_bench_test.go
index ede54e54..247fdf5b 100644
--- a/log/keyvalue_bench_test.go
+++ b/log/keyvalue_bench_test.go
@@ -199,3 +199,34 @@ func BenchmarkString(b *testing.B) {
 		}
 	})
 }
+
+func BenchmarkValueEqual(b *testing.B) {
+	vals := []log.Value{
+		{},
+		log.Int64Value(1),
+		log.Int64Value(2),
+		log.Float64Value(3.5),
+		log.Float64Value(3.7),
+		log.BoolValue(true),
+		log.BoolValue(false),
+		log.StringValue("hi"),
+		log.StringValue("bye"),
+		log.BytesValue([]byte{1, 3, 5}),
+		log.SliceValue(log.StringValue("foo")),
+		log.SliceValue(log.IntValue(3), log.StringValue("foo")),
+		log.MapValue(log.Bool("b", true), log.Int("i", 3)),
+		log.MapValue(
+			log.Slice("l", log.IntValue(3), log.StringValue("foo")),
+			log.Bytes("b", []byte{3, 5, 7}),
+			log.Empty("e"),
+		),
+	}
+	for _, v1 := range vals {
+		for _, v2 := range vals {
+			b.Run(v1.String()+" with "+v2.String(), func(b *testing.B) {
+				b.ReportAllocs()
+				_ = v1.Equal(v2)
+			})
+		}
+	}
+}
diff --git a/log/keyvalue_test.go b/log/keyvalue_test.go
index 2f021116..043fab09 100644
--- a/log/keyvalue_test.go
+++ b/log/keyvalue_test.go
@@ -70,6 +70,31 @@ func TestValueEqual(t *testing.T) {
 	}
 }
 
+func TestSortedValueEqual(t *testing.T) {
+	testCases := []struct {
+		value  log.Value
+		value2 log.Value
+	}{
+		{
+			value: log.MapValue(
+				log.Slice("l", log.IntValue(3), log.StringValue("foo")),
+				log.Bytes("b", []byte{3, 5, 7}),
+				log.Empty("e"),
+			),
+			value2: log.MapValue(
+				log.Bytes("b", []byte{3, 5, 7}),
+				log.Slice("l", log.IntValue(3), log.StringValue("foo")),
+				log.Empty("e"),
+			),
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.value.String(), func(t *testing.T) {
+			assert.Equal(t, true, tc.value.Equal(tc.value2), "%v.Equal(%v)", tc.value, tc.value2)
+		})
+	}
+}
+
 func TestValueEmpty(t *testing.T) {
 	v := log.Value{}
 	t.Run("Value.Empty", func(t *testing.T) {