otlpmetric with StringSlice produces duplicate metrics #3108

mmidzik · 2022-08-24T18:23:50Z

Description

Metrics added with the same key and a StringSlice value are not deduplicated, resulting in multiple identical output series. If running with a prometheus exporter, this causes the exporter to fail with collected metric "metric_name" … was collected before with the same name and label values from this error.

Environment

Running in golang:1.16 image

go.opentelemetry.io/otel v1.9.0
go.opentelemetry.io/otel/exporters/stdout/stdoutmetric v0.31.0
go.opentelemetry.io/otel/sdk/metric v0.31.0

Steps To Reproduce

package example

import (
	"context"
	"fmt"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
	"go.opentelemetry.io/otel/attribute"
	"go.opentelemetry.io/otel/exporters/stdout/stdoutmetric"
	controller "go.opentelemetry.io/otel/sdk/metric/controller/basic"
	processor "go.opentelemetry.io/otel/sdk/metric/processor/basic"
	"go.opentelemetry.io/otel/sdk/metric/selector/simple"
)

func TestIssue(t *testing.T) {

	exp, err := stdoutmetric.New(stdoutmetric.WithPrettyPrint())
	require.NoError(t, err)

	pusher := controller.New(
		processor.NewFactory(
			simple.NewWithHistogramDistribution(),
			exp,
		),
		controller.WithExporter(exp),
		controller.WithCollectPeriod(time.Second),
	)
	_ = pusher.Start(context.Background())

	Int64Histogram, err := pusher.Meter("test").SyncInt64().Histogram(
		fmt.Sprintf("test_histogram"),
	)
	require.NoError(t, err)

	for i := 0; i < 3; i++ {
		Int64Histogram.Record(context.Background(), 1, attribute.String("foo", "a"))
		Int64Histogram.Record(context.Background(), 1, attribute.StringSlice("bar", []string{"b"}))
	}
	time.Sleep(time.Second * 3)
}

Currently outputs:

[
	{
		"Name": "test_histogram{service.name=unknown_service:example.test,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=test,foo=a}",
		"Sum": 3
	},
	{
		"Name": "test_histogram{service.name=unknown_service:example.test,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=test,bar=[b]}",
		"Sum": 1
	},
	{
		"Name": "test_histogram{service.name=unknown_service:example.test,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=test,bar=[b]}",
		"Sum": 1
	},
	{
		"Name": "test_histogram{service.name=unknown_service:example.test,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=test,bar=[b]}",
		"Sum": 1
	}
]

Expected behavior

I expect the key-values of StringSlice to deduplicate, with the above snippet producing two metrics with a sum of 3.
I expect the values of StringSlice to be deduplicated as-is, without sorting (e.g. the array stringified), as they are input as an ordered array.

[
	{
		"Name": "test_histogram{service.name=unknown_service:example.test,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=test,foo=a}",
		"Sum": 3
	},
	{
		"Name": "test_histogram{service.name=unknown_service:example.test,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=test,bar=[b]}",
		"Sum": 3
	},
]

Note: I recognize this might be intentional behavior, but it surprised me / I wasn't able to find documentation around the behavior. If this is not a bug but rather a documentation issue, I'd be happy to submit a related documentation MR.

The text was updated successfully, but these errors were encountered:

MrAlias · 2022-09-08T16:04:57Z

Thanks for the bug report.

The duplicate metrics are resulting from attribute sets not rendering a truly comparable Distinct value for a slice. For example:

package main

import (
	"fmt"

	"go.opentelemetry.io/otel/attribute"
)

func main() {
	a := attribute.NewSet(attribute.StringSlice("key", []string{"value"}))
	b := attribute.NewSet(attribute.StringSlice("key", []string{"value"}))
	fmt.Println(a.Equivalent() == b.Equivalent())
}

returns false even though the sets are equivalent.

This means that the map key used by the metric instrument does not retrieve existing metrics when they exist.

opentelemetry-go/sdk/metric/sdk.go

Line 161 in 569f743

ordered: rec.attrs.Equivalent(),

This is almost certainly going to be a problem with the new SDK as well:

opentelemetry-go/sdk/metric/internal/histogram.go

Line 94 in 29c9c40

b, ok := s.values[attr]

MrAlias · 2022-09-08T16:42:27Z

This test passes when run against the main branch:

func TestSetComparability(t *testing.T) {
	pairs := [][2]attribute.KeyValue{
		{
			attribute.Bool("Bool", true),
			attribute.Bool("Bool", true),
		},
		{
			attribute.BoolSlice("BoolSlice", []bool{true, false, true}),
			attribute.BoolSlice("BoolSlice", []bool{true, false, true}),
		},
		{
			attribute.Int("Int", 34),
			attribute.Int("Int", 34),
		},
		{
			attribute.IntSlice("IntSlice", []int{312, 1, -2}),
			attribute.IntSlice("IntSlice", []int{312, 1, -2}),
		},
		{
			attribute.Int64("Int64", 98),
			attribute.Int64("Int64", 98),
		},
		{
			attribute.Int64Slice("Int64Slice", []int64{12, 1298, -219, 2}),
			attribute.Int64Slice("Int64Slice", []int64{12, 1298, -219, 2}),
		},
		{
			attribute.Float64("Float64", 19.09),
			attribute.Float64("Float64", 19.09),
		},
		{
			attribute.Float64Slice("Float64Slice", []float64{12398.1, -37.1713873737, 3}),
			attribute.Float64Slice("Float64Slice", []float64{12398.1, -37.1713873737, 3}),
		},
		{
			attribute.String("String", "string value"),
			attribute.String("String", "string value"),
		},
		{
			attribute.StringSlice("StringSlice", []string{"one", "two", "three"}),
			attribute.StringSlice("StringSlice", []string{"one", "two", "three"}),
		},
		{
			attribute.Stringer("Stringer", stringer("stringer value")),
			attribute.Stringer("Stringer", stringer("stringer value")),
		},
	}

	for _, p := range pairs {
		s0, s1 := attribute.NewSet(p[0]), attribute.NewSet(p[1])
		m := map[attribute.Set]struct{}{s0: {}}
		assert.Containsf(t, m, s1, "%s not comparable", p[0].Value.Type())
	}
}

I wonder if the inequality is coming from the descriptor:

opentelemetry-go/sdk/metric/sdk.go

Line 160 in 569f743

descriptor: &b.descriptor,

or maybe the sort slice being reused?

opentelemetry-go/sdk/metric/sdk.go

Line 155 in 569f743

rec.attrs = attribute.NewSetWithSortable(kvs, &rec.sortSlice)

More investigation is needed.

MrAlias · 2022-09-08T16:49:10Z

When a direct map look-up is used, the above test fails:

func TestSetComparability(t *testing.T) {
	pairs := [][2]attribute.KeyValue{
		{
			attribute.Bool("Bool", true),
			attribute.Bool("Bool", true),
		},
		{
			attribute.BoolSlice("BoolSlice", []bool{true, false, true}),
			attribute.BoolSlice("BoolSlice", []bool{true, false, true}),
		},
		{
			attribute.Int("Int", 34),
			attribute.Int("Int", 34),
		},
		{
			attribute.IntSlice("IntSlice", []int{312, 1, -2}),
			attribute.IntSlice("IntSlice", []int{312, 1, -2}),
		},
		{
			attribute.Int64("Int64", 98),
			attribute.Int64("Int64", 98),
		},
		{
			attribute.Int64Slice("Int64Slice", []int64{12, 1298, -219, 2}),
			attribute.Int64Slice("Int64Slice", []int64{12, 1298, -219, 2}),
		},
		{
			attribute.Float64("Float64", 19.09),
			attribute.Float64("Float64", 19.09),
		},
		{
			attribute.Float64Slice("Float64Slice", []float64{12398.1, -37.1713873737, 3}),
			attribute.Float64Slice("Float64Slice", []float64{12398.1, -37.1713873737, 3}),
		},
		{
			attribute.String("String", "string value"),
			attribute.String("String", "string value"),
		},
		{
			attribute.StringSlice("StringSlice", []string{"one", "two", "three"}),
			attribute.StringSlice("StringSlice", []string{"one", "two", "three"}),
		},
		{
			attribute.Stringer("Stringer", stringer("stringer value")),
			attribute.Stringer("Stringer", stringer("stringer value")),
		},
	}

	for _, p := range pairs {
		s0, s1 := attribute.NewSet(p[0]), attribute.NewSet(p[1])
		m := map[attribute.Set]struct{}{s0: {}}
		_, ok := m[s1]
		assert.Truef(t, ok, "%s not comparable", p[0].Value.Type())
	}
}

=== RUN   TestSetComparability
    set_test.go:248:
        	Error Trace:	set_test.go:248
        	Error:      	Should be true
        	Test:       	TestSetComparability
        	Messages:   	BOOLSLICE not comparable
    set_test.go:248:
        	Error Trace:	set_test.go:248
        	Error:      	Should be true
        	Test:       	TestSetComparability
        	Messages:   	INT64SLICE not comparable
    set_test.go:248:
        	Error Trace:	set_test.go:248
        	Error:      	Should be true
        	Test:       	TestSetComparability
        	Messages:   	INT64SLICE not comparable
    set_test.go:248:
        	Error Trace:	set_test.go:248
        	Error:      	Should be true
        	Test:       	TestSetComparability
        	Messages:   	FLOAT64SLICE not comparable
    set_test.go:248:
        	Error Trace:	set_test.go:248
        	Error:      	Should be true
        	Test:       	TestSetComparability
        	Messages:   	STRINGSLICE not comparable
--- FAIL: TestSetComparability (0.00s)
FAIL
FAIL	go.opentelemetry.io/otel/attribute	0.004s
FAIL

jmacd · 2022-09-08T18:01:32Z

I believe this can be solved inside the attribute package.

jmacd · 2022-09-08T21:36:02Z

I can't prioritize working on this right away. In case someone else wants to pick this up, here's roughly how the solution looks for just one type of slice (out of 5).

https://github.com/jmacd/opentelemetry-go/pull/new/jmacd/sampleslicesupport

diff --git a/attribute/value.go b/attribute/value.go
index 57899f68..1de57075 100644
--- a/attribute/value.go
+++ b/attribute/value.go
@@ -17,6 +17,7 @@ package attribute // import "go.opentelemetry.io/otel/attribute"
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"strconv"
 
 	"go.opentelemetry.io/otel/internal"
@@ -66,11 +67,11 @@ func BoolValue(v bool) Value {
 
 // BoolSliceValue creates a BOOLSLICE Value.
 func BoolSliceValue(v []bool) Value {
-	cp := make([]bool, len(v))
-	copy(cp, v)
+	cp := reflect.New(reflect.ArrayOf(len(v), reflect.TypeOf(false)))
+	copy(cp.Elem().Slice(0, len(v)).Interface().([]bool), v)
 	return Value{
 		vtype: BOOLSLICE,
-		slice: &cp,
+		slice: cp.Elem().Interface(), // No longer a pointer
 	}
 }
 
@@ -159,10 +160,22 @@ func (v Value) AsBool() bool {
 // AsBoolSlice returns the []bool value. Make sure that the Value's type is
 // BOOLSLICE.
 func (v Value) AsBoolSlice() []bool {
-	if s, ok := v.slice.(*[]bool); ok {
-		return *s
+	rv := reflect.ValueOf(v.slice)
+	if rv.Type().Kind() != reflect.Array {
+		// it's not a slice
+		return nil
 	}
-	return nil
+	correctLen := rv.Len()
+	correctType := reflect.ArrayOf(correctLen, reflect.TypeOf(false))
+	if rv.Type() != correctType {
+		// it's a slice of the wrong kind
+		return nil
+	}
+	// we can't slice it yet, it's not addressable (by design,
+	// otherwise it wouldn't satisfy the requirements of Set).
+	cpy := reflect.New(correctType)
+	_ = reflect.Copy(cpy.Elem(), rv)
+	return cpy.Elem().Slice(0, correctLen).Interface().([]bool)
 }
 
 // AsInt64 returns the int64 value. Make sure that the Value's type is

As you can see, this is difficult to read. Potentially a generics-based solution would be easier to accept?

jmacd · 2022-09-08T21:38:37Z

Performance note: implied by the above, we have to copy the slice to read it. This isn't great. We could offer a different API to access the i'th element of a slice-valued attribute instead, that would allow the consumer of a Value to avoid copying.

MrAlias · 2022-09-08T22:35:05Z

I can't prioritize working on this right away. In case someone else wants to pick this up, here's roughly how the solution looks for just one type of slice (out of 5).

https://github.com/jmacd/opentelemetry-go/pull/new/jmacd/sampleslicesupport

diff --git a/attribute/value.go b/attribute/value.go
index 57899f68..1de57075 100644
--- a/attribute/value.go
+++ b/attribute/value.go
@@ -17,6 +17,7 @@ package attribute // import "go.opentelemetry.io/otel/attribute"
 import (
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"strconv"
 
 	"go.opentelemetry.io/otel/internal"
@@ -66,11 +67,11 @@ func BoolValue(v bool) Value {
 
 // BoolSliceValue creates a BOOLSLICE Value.
 func BoolSliceValue(v []bool) Value {
-	cp := make([]bool, len(v))
-	copy(cp, v)
+	cp := reflect.New(reflect.ArrayOf(len(v), reflect.TypeOf(false)))
+	copy(cp.Elem().Slice(0, len(v)).Interface().([]bool), v)
 	return Value{
 		vtype: BOOLSLICE,
-		slice: &cp,
+		slice: cp.Elem().Interface(), // No longer a pointer
 	}
 }
 
@@ -159,10 +160,22 @@ func (v Value) AsBool() bool {
 // AsBoolSlice returns the []bool value. Make sure that the Value's type is
 // BOOLSLICE.
 func (v Value) AsBoolSlice() []bool {
-	if s, ok := v.slice.(*[]bool); ok {
-		return *s
+	rv := reflect.ValueOf(v.slice)
+	if rv.Type().Kind() != reflect.Array {
+		// it's not a slice
+		return nil
 	}
-	return nil
+	correctLen := rv.Len()
+	correctType := reflect.ArrayOf(correctLen, reflect.TypeOf(false))
+	if rv.Type() != correctType {
+		// it's a slice of the wrong kind
+		return nil
+	}
+	// we can't slice it yet, it's not addressable (by design,
+	// otherwise it wouldn't satisfy the requirements of Set).
+	cpy := reflect.New(correctType)
+	_ = reflect.Copy(cpy.Elem(), rv)
+	return cpy.Elem().Slice(0, correctLen).Interface().([]bool)
 }
 
 // AsInt64 returns the int64 value. Make sure that the Value's type is

As you can see, this is difficult to read. Potentially a generics-based solution would be easier to accept?

I was able to validate this passes the above test. 👍

fatsheep9146 · 2022-09-21T11:59:44Z

can i take this issue ? @jmacd @MrAlias

jmacd · 2022-09-29T17:09:12Z

@fatsheep9146 yes, please!

…presentation change, see open-telemetry/opentelemetry-go#3108

* wip, progress so far * more similar structure to otel, fixing imports * fix for attributesEqual() following otel-go slice-valued attribute representation change, see open-telemetry/opentelemetry-go#3108 * more metric->view pkg fixes * remove otlpconfig pkg * export NewClient() func!! * moved some things around * fix exporter, pipeline imports * go.mod updates * fix shutdown deadlock * upgrade versions to v1.11.2 and v0.34.0 * fix example I missed * work on instrumentation unit tests * repair the tests * fixed commented out unit tests * fix/add comments, stop calling the otel skd "old" * fix go.mod Co-authored-by: Joshua MacDonald <[email protected]>

mmidzik added the bug Something isn't working label Aug 24, 2022

MrAlias added this to the Metric SDK: Beta milestone Sep 8, 2022

MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Sep 8, 2022

MrAlias assigned jmacd Sep 8, 2022

MrAlias added the help wanted Extra attention is needed label Sep 8, 2022

jmacd assigned fatsheep9146 Sep 29, 2022

MrAlias modified the milestones: Metric SDK: Beta, Metric v0.32.2 Sep 29, 2022

fatsheep9146 mentioned this issue Oct 2, 2022

attribute: fix slice related function bug #3252

Merged

MadVikingGod modified the milestones: Metric v0.32.2, v0.32.3 Oct 11, 2022

MadVikingGod closed this as completed in #3252 Oct 13, 2022

MrAlias mentioned this issue Oct 19, 2022

Release v1.11.1/v0.33.0 #3367

Merged

jmacd pushed a commit to lightstep/otel-launcher-go that referenced this issue Nov 29, 2022

fix for attributesEqual() following otel-go slice-valued attribute re…

15f1785

…presentation change, see open-telemetry/opentelemetry-go#3108

kristinapathak pushed a commit to lightstep/otel-launcher-go that referenced this issue Nov 30, 2022

fix for attributesEqual() following otel-go slice-valued attribute re…

0edfbbd

…presentation change, see open-telemetry/opentelemetry-go#3108

pellared added this to Go: Triage Nov 2, 2023

pellared moved this to Closed in Go: Triage Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

otlpmetric with StringSlice produces duplicate metrics #3108

otlpmetric with StringSlice produces duplicate metrics #3108

mmidzik commented Aug 24, 2022 •

edited

Loading

MrAlias commented Sep 8, 2022

MrAlias commented Sep 8, 2022

MrAlias commented Sep 8, 2022 •

edited

Loading

jmacd commented Sep 8, 2022

jmacd commented Sep 8, 2022 •

edited

Loading

jmacd commented Sep 8, 2022

MrAlias commented Sep 8, 2022

fatsheep9146 commented Sep 21, 2022

jmacd commented Sep 29, 2022

otlpmetric with StringSlice produces duplicate metrics #3108

otlpmetric with StringSlice produces duplicate metrics #3108

Comments

mmidzik commented Aug 24, 2022 • edited Loading

Description

Environment

Steps To Reproduce

Expected behavior

MrAlias commented Sep 8, 2022

MrAlias commented Sep 8, 2022

MrAlias commented Sep 8, 2022 • edited Loading

jmacd commented Sep 8, 2022

jmacd commented Sep 8, 2022 • edited Loading

jmacd commented Sep 8, 2022

MrAlias commented Sep 8, 2022

fatsheep9146 commented Sep 21, 2022

jmacd commented Sep 29, 2022

mmidzik commented Aug 24, 2022 •

edited

Loading

MrAlias commented Sep 8, 2022 •

edited

Loading

jmacd commented Sep 8, 2022 •

edited

Loading