Fix failed label does not work leads to servicegraph metrics error

Co-authored-by: Tomas Pica <[email protected]>

.chloggen/Fix-failed-label-does-not-work-leads-to-servicegraph-metrics-error.yaml

@@ -0,0 +1,27 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
+component: servicegraphconnector
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix failed label does not work leads to servicegraph metrics error
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [32018]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
+
+# If your change doesn't affect end users or the exported elements of any package,
+# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: []

connector/servicegraphconnector/connector.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -367,7 +368,7 @@ func (p *serviceGraphConnector) onExpire(e *store.Edge) {
 }
 
 func (p *serviceGraphConnector) aggregateMetricsForEdge(e *store.Edge) {
-	metricKey := p.buildMetricKey(e.ClientService, e.ServerService, string(e.ConnectionType), e.Dimensions)
+	metricKey := p.buildMetricKey(e.ClientService, e.ServerService, string(e.ConnectionType), strconv.FormatBool(e.Failed), e.Dimensions)
 	dimensions := buildDimensions(e)
 
 	if p.config.VirtualNodeExtraLabel {
@@ -583,9 +584,9 @@ func (p *serviceGraphConnector) collectServerLatencyMetrics(ilm pmetric.ScopeMet
 	return nil
 }
 
-func (p *serviceGraphConnector) buildMetricKey(clientName, serverName, connectionType string, edgeDimensions map[string]string) string {
+func (p *serviceGraphConnector) buildMetricKey(clientName, serverName, connectionType, failed string, edgeDimensions map[string]string) string {
 	var metricKey strings.Builder
-	metricKey.WriteString(clientName + metricKeySeparator + serverName + metricKeySeparator + connectionType)
+	metricKey.WriteString(clientName + metricKeySeparator + serverName + metricKeySeparator + connectionType + metricKeySeparator + failed)
 
 	for _, dimName := range p.config.Dimensions {
 		dim, ok := edgeDimensions[dimName]

connector/servicegraphconnector/connector_test.go

@@ -66,31 +66,68 @@ func TestConnectorShutdown(t *testing.T) {
 }
 
 func TestConnectorConsume(t *testing.T) {
-	// Prepare
-	cfg := &Config{
-		Dimensions: []string{"some-attribute", "non-existing-attribute"},
-		Store:      StoreConfig{MaxItems: 10},
-	}
-
-	set := componenttest.NewNopTelemetrySettings()
-	set.Logger = zaptest.NewLogger(t)
-	conn, err := newConnector(set, cfg, newMockMetricsExporter())
-	require.NoError(t, err)
-	assert.NoError(t, conn.Start(context.Background(), componenttest.NewNopHost()))
-
-	// Test & verify
-	td := buildSampleTrace(t, "val")
-	// The assertion is part of verifyHappyCaseMetrics func.
-	assert.NoError(t, conn.ConsumeTraces(context.Background(), td))
-
-	// Force collection
-	conn.store.Expire()
-	md, err := conn.buildMetrics()
-	assert.NoError(t, err)
-	verifyHappyCaseMetrics(t, md)
-
-	// Shutdown the connector
-	assert.NoError(t, conn.Shutdown(context.Background()))
+	t.Run("test common case", func(t *testing.T) {
+		// Prepare
+		cfg := &Config{
+			Dimensions: []string{"some-attribute", "non-existing-attribute"},
+			Store:      StoreConfig{MaxItems: 10},
+		}
+
+		set := componenttest.NewNopTelemetrySettings()
+		set.Logger = zaptest.NewLogger(t)
+		conn, err := newConnector(set, cfg, newMockMetricsExporter())
+		require.NoError(t, err)
+		assert.NoError(t, conn.Start(context.Background(), componenttest.NewNopHost()))
+
+		// Test & verify
+		td := buildSampleTrace(t, "val")
+		// The assertion is part of verifyHappyCaseMetrics func.
+		assert.NoError(t, conn.ConsumeTraces(context.Background(), td))
+
+		// Force collection
+		conn.store.Expire()
+		md, err := conn.buildMetrics()
+		assert.NoError(t, err)
+		verifyHappyCaseMetrics(t, md)
+
+		// Shutdown the connector
+		assert.NoError(t, conn.Shutdown(context.Background()))
+	})
+	t.Run("test fix failed label not work", func(t *testing.T) {
+		cfg := &Config{
+			Store: StoreConfig{MaxItems: 10},
+		}
+		set := componenttest.NewNopTelemetrySettings()
+		set.Logger = zaptest.NewLogger(t)
+		conn, err := newConnector(set, cfg, newMockMetricsExporter())
+		require.NoError(t, err)
+
+		assert.NoError(t, conn.Start(context.Background(), componenttest.NewNopHost()))
+		defer require.NoError(t, conn.Shutdown(context.Background()))
+
+		// this trace simulate two services' trace: foo, bar
+		// foo called bar three times, two success, one failed
+		td, err := golden.ReadTraces("testdata/failed-label-not-work-simple-trace.yaml")
+		assert.NoError(t, err)
+		assert.NoError(t, conn.ConsumeTraces(context.Background(), td))
+
+		// Force collection
+		conn.store.Expire()
+		actualMetrics, err := conn.buildMetrics()
+		assert.NoError(t, err)
+
+		// Verify
+		expectedMetrics, err := golden.ReadMetrics("testdata/failed-label-not-work-expect-metrics.yaml")
+		assert.NoError(t, err)
+
+		err = pmetrictest.CompareMetrics(expectedMetrics, actualMetrics,
+			pmetrictest.IgnoreMetricsOrder(),
+			pmetrictest.IgnoreMetricDataPointsOrder(),
+			pmetrictest.IgnoreStartTimestamp(),
+			pmetrictest.IgnoreTimestamp(),
+		)
+		require.NoError(t, err)
+	})
 }
 
 func verifyHappyCaseMetrics(t *testing.T, md pmetric.Metrics) {
@@ -262,7 +299,7 @@ func TestUpdateDurationMetrics(t *testing.T) {
 			Dimensions: []string{},
 		},
 	}
-	metricKey := p.buildMetricKey("foo", "bar", "", map[string]string{})
+	metricKey := p.buildMetricKey("foo", "bar", "", "false", map[string]string{})
 
 	testCases := []struct {
 		caseStr  string

connector/servicegraphconnector/go.mod

@@ -110,6 +110,6 @@ retract (
 
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/golden => ../../pkg/golden
 
-replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil => ../../pkg/pdatautil
-
 replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest => ../../pkg/pdatatest
+
+replace github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatautil => ../../pkg/pdatautil