more viewstate tests; actual fixes

open-telemetry · Apr 29, 2022 · cbbcec6 · cbbcec6
1 parent 445f47a
commit cbbcec6
Show file tree

Hide file tree

Showing 5 changed files with 477 additions and 72 deletions.
diff --git a/sdk/metric/internal/viewstate/errors.go b/sdk/metric/internal/viewstate/errors.go
@@ -38,17 +38,17 @@ func (vc ViewConflictsError) Error() string {
 		total += len(l)
 	}
 	// These are almost always duplicative, so we print only examples for one Config.
-	for exp, conflictsByExporter := range vc {
-		if len(conflictsByExporter) == 0 {
+	for rd, conflictsByReader := range vc {
+		if len(conflictsByReader) == 0 {
 			break
 		}
 		if len(vc) == 1 {
-			if len(conflictsByExporter) == 1 {
-				return fmt.Sprintf("%v: %v", exp, conflictsByExporter[0].Error())
+			if len(conflictsByReader) == 1 {
+				return fmt.Sprintf("%v: %v", rd, conflictsByReader[0].Error())
 			}
-			return fmt.Sprintf("%d conflicts, e.g. %v: %v", total, exp, conflictsByExporter[0])
+			return fmt.Sprintf("%d conflicts, e.g. %v: %v", total, rd, conflictsByReader[0])
 		}
-		return fmt.Sprintf("%d conflicts in %d readers, e.g. %v: %v", total, len(vc), exp, conflictsByExporter[0])
+		return fmt.Sprintf("%d conflicts in %d readers, e.g. %v: %v", total, len(vc), rd, conflictsByReader[0])
 	}
 	return noConflictsString
 }
@@ -58,12 +58,12 @@ func (ViewConflictsError) Is(err error) bool {
 	return ok
 }
 
-func (vc *ViewConflictsBuilder) Add(exp reader.Reader, c Conflict) {
+func (vc *ViewConflictsBuilder) Add(rd reader.Reader, c Conflict) {
 	if *vc == nil {
 		*vc = ViewConflictsBuilder{}
 	}
 
-	(*vc)[exp] = append((*vc)[exp], c)
+	(*vc)[rd] = append((*vc)[rd], c)
 }
 
 func (vc *ViewConflictsBuilder) Combine(other ViewConflictsBuilder) {

diff --git a/sdk/metric/internal/viewstate/errors_test.go b/sdk/metric/internal/viewstate/errors_test.go
@@ -55,37 +55,53 @@ func TestViewConflictsBuilder(t *testing.T) {
 	// enough to test the ViewConflicts logic.
 	oneError := oneConflict.Semantic.Error()
 
-	builder = ViewConflictsBuilder{
-		rd1: []Conflict{
-			oneConflict,
-		},
-	}
+	builder = ViewConflictsBuilder{}
+	builder.Add(rd0, oneConflict)
 	err = builder.AsError()
 	require.True(t, strings.HasSuffix(err.Error(), oneError), err)
 
-	builder = ViewConflictsBuilder{
-		rd1: []Conflict{
-			oneConflict,
-			oneConflict,
-		},
-	}
+	builder = ViewConflictsBuilder{}
+	builder.Add(rd0, oneConflict)
+	builder.Add(rd0, oneConflict)
 	err = builder.AsError()
 	require.True(t, strings.HasSuffix(err.Error(), oneError), err)
 	require.True(t, strings.HasPrefix(err.Error(), "2 conflicts, e.g. "), err)
 
-	builder = ViewConflictsBuilder{
-		rd0: []Conflict{
-			oneConflict,
-		},
-		rd1: []Conflict{
-			oneConflict,
-		},
-	}
+	builder = ViewConflictsBuilder{}
+	builder.Add(rd0, oneConflict)
+	builder.Add(rd1, oneConflict)
 	err = builder.AsError()
 	require.True(t, strings.HasSuffix(err.Error(), oneError), err)
 	require.True(t, strings.HasPrefix(err.Error(), "2 conflicts in 2 readers, e.g. "), err)
 }
 
+func TestConflictCombine(t *testing.T) {
+	pipelines := export.NewPipelines(
+		export.NewPipeline(reader.NewManualReader("test0")),
+		export.NewPipeline(reader.NewManualReader("test1")),
+	)
+	rd0 := pipelines.At(0).Reader
+	rd1 := pipelines.At(1).Reader
+
+	builder1 := ViewConflictsBuilder{}
+	builder1.Add(rd0, oneConflict)
+
+	builder2 := ViewConflictsBuilder{}
+	builder2.Add(rd1, oneConflict)
+
+	builder1.Combine(builder2)
+	err1 := builder1.AsError()
+	require.True(t, strings.HasSuffix(err1.Error(), oneConflict.Semantic.Error()), err1)
+	require.True(t, strings.HasPrefix(err1.Error(), "2 conflicts in 2 readers, e.g. "), err1)
+
+	var builder3 ViewConflictsBuilder
+	builder3.Combine(ViewConflictsBuilder{}) // empty builder has no effect
+	builder3.Combine(builder1)
+	err3 := builder3.AsError()
+
+	require.Equal(t, err1, err3)
+}
+
 // TestConflictError tests that both semantic errors and duplicate
 // conflicts are printed.  Note this uses the real library to generate
 // the conflict, to avoid creating a relatively large test-only type.

diff --git a/sdk/metric/internal/viewstate/viewstate.go b/sdk/metric/internal/viewstate/viewstate.go
@@ -438,11 +438,10 @@ func newSyncView[
 ](config configuredBehavior) leafInstrument {
 	tempo := config.reader.DefaultTemporality(config.desc.Kind)
 	metric := baseMetric[N, Storage, Methods]{
-		fromName: config.fromName,
-		desc:     config.desc,
-		acfg:     config.acfg,
-		data:     map[attribute.Set]*Storage{},
-
+		fromName:   config.fromName,
+		desc:       config.desc,
+		acfg:       config.acfg,
+		data:       map[attribute.Set]*Storage{},
 		keysSet:    config.keysSet,
 		keysFilter: config.keysFilter,
 	}
@@ -467,11 +466,12 @@ func newAsyncView[
 ](config configuredBehavior) leafInstrument {
 	tempo := config.reader.DefaultTemporality(config.desc.Kind)
 	metric := baseMetric[N, Storage, Methods]{
-		fromName: config.fromName,
-		desc:     config.desc,
-		acfg:     config.acfg,
-		data:     map[attribute.Set]*Storage{},
-		keysSet:  config.keysSet,
+		fromName:   config.fromName,
+		desc:       config.desc,
+		acfg:       config.acfg,
+		data:       map[attribute.Set]*Storage{},
+		keysSet:    config.keysSet,
+		keysFilter: config.keysFilter,
 	}
 	instrument := compiledAsyncInstrument[N, Storage, Methods]{
 		baseMetric: metric,