Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

otlpmetric with StringSlice produces duplicate metrics #3108

Closed
mmidzik opened this issue Aug 24, 2022 · 9 comments · Fixed by #3252
Closed

otlpmetric with StringSlice produces duplicate metrics #3108

mmidzik opened this issue Aug 24, 2022 · 9 comments · Fixed by #3252
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working help wanted Extra attention is needed pkg:SDK Related to an SDK package
Projects

Comments

@mmidzik
Copy link

mmidzik commented Aug 24, 2022

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.

@mmidzik mmidzik added the bug Something isn't working label Aug 24, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Sep 8, 2022

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.

ordered: rec.attrs.Equivalent(),

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

b, ok := s.values[attr]

@MrAlias MrAlias added this to the Metric SDK: Beta milestone Sep 8, 2022
@MrAlias MrAlias added this to Needs triage in Bugs via automation Sep 8, 2022
@MrAlias MrAlias moved this from Needs triage to High priority in Bugs Sep 8, 2022
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Sep 8, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Sep 8, 2022

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:

descriptor: &b.descriptor,

or maybe the sort slice being reused?

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

More investigation is needed.

@MrAlias
Copy link
Contributor

MrAlias commented Sep 8, 2022

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
Copy link
Contributor

jmacd commented Sep 8, 2022

I believe this can be solved inside the attribute package.

@jmacd
Copy link
Contributor

jmacd commented Sep 8, 2022

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
Copy link
Contributor

jmacd commented Sep 8, 2022

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
Copy link
Contributor

MrAlias commented Sep 8, 2022

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. 👍

@MrAlias MrAlias added the help wanted Extra attention is needed label Sep 8, 2022
@fatsheep9146
Copy link
Contributor

can i take this issue ? @jmacd @MrAlias

@jmacd
Copy link
Contributor

jmacd commented Sep 29, 2022

@fatsheep9146 yes, please!

@MadVikingGod MadVikingGod modified the milestones: Metric v0.32.2, v0.32.3 Oct 11, 2022
Bugs automation moved this from High priority to Closed Oct 13, 2022
jmacd added a commit to lightstep/otel-launcher-go that referenced this issue Nov 29, 2022
kristinapathak pushed a commit to lightstep/otel-launcher-go that referenced this issue Nov 30, 2022
kristinapathak added a commit to lightstep/otel-launcher-go that referenced this issue Dec 9, 2022
* 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 <jmacd@lightstep.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working help wanted Extra attention is needed pkg:SDK Related to an SDK package
Projects
Archived in project
Bugs
  
Closed
Development

Successfully merging a pull request may close this issue.

5 participants