Allow suppressing service.instance.id and service.version

Also added proper tests for all cases.

config/moved_service.go

@@ -36,9 +36,15 @@ type Service struct {
 // ServiceTelemetry defines the configurable settings for service telemetry.
 // Deprecated: [v0.52.0] Use service.ConfigServiceTelemetry
 type ServiceTelemetry struct {
-	Logs     ServiceTelemetryLogs    `mapstructure:"logs"`
-	Metrics  ServiceTelemetryMetrics `mapstructure:"metrics"`
-	Resource map[string]string       `mapstructure:"resource"`
+	Logs    ServiceTelemetryLogs    `mapstructure:"logs"`
+	Metrics ServiceTelemetryMetrics `mapstructure:"metrics"`
+
+	// Resource specifies user-defined attributes to include with all emitted telemetry.
+	// For metrics the attributes become Prometheus labels.
+	// Note that some attributes are added automatically (e.g. service.version) even
+	// if they are not specified here. In order to suppress such attributes the
+	// attribute must be specified in this map with empty string value.
+	Resource map[string]string `mapstructure:"resource"`
 }
 
 // ServiceTelemetryLogs defines the configurable settings for service telemetry logs.

service/collector_test.go

@@ -35,7 +35,6 @@ import (
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config"
-	"go.opentelemetry.io/collector/config/mapconverter/overwritepropertiesmapconverter"
 	"go.opentelemetry.io/collector/internal/testcomponents"
 	"go.opentelemetry.io/collector/internal/testutil"
 	"go.opentelemetry.io/collector/internal/version"
@@ -179,7 +178,105 @@ func TestCollectorFailedShutdown(t *testing.T) {
 	assert.Equal(t, Closed, col.GetState())
 }
 
-func testCollectorStartHelper(t *testing.T, telemetry collectorTelemetryExporter) {
+// mapConverter applies extraMap of config settings. Useful for overriding the config
+// for testing purposes. Keys must use "::" delimiter between levels.
+func mapConverter(extraMap map[string]string) config.MapConverterFunc {
+	return func(ctx context.Context, cfgMap *config.Map) error {
+		for k, v := range extraMap {
+			cfgMap.Set(k, v)
+		}
+		return nil
+	}
+}
+
+type labelState int
+
+const (
+	labelNotPresent labelState = iota
+	labelSpecificValue
+	labelAnyValue
+)
+
+type labelValue struct {
+	label string
+	state labelState
+}
+
+type ownMetricsTestCase struct {
+	name                string
+	userDefinedResource map[string]string
+	expectedLabels      map[string]labelValue
+}
+
+var ownMetricsTestCases = []ownMetricsTestCase{
+	{
+		name:                "no resource",
+		userDefinedResource: nil,
+		// All labels added to all collector metrics by default are listed below.
+		// These labels are hard coded here in order to avoid inadvertent changes:
+		// at this point changing labels should be treated as a breaking changing
+		// and requires a good justification. The reason is that changes to metric
+		// names or labels can break alerting, dashboards, etc that are used to
+		// monitor the Collector in production deployments.
+		expectedLabels: map[string]labelValue{
+			"service_instance_id": {state: labelAnyValue},
+			"service_version":     {label: version.Version, state: labelSpecificValue},
+		},
+	},
+	{
+		name: "resource with custom attr",
+		userDefinedResource: map[string]string{
+			"custom_resource_attr": "resource_attr_test_value",
+		},
+		expectedLabels: map[string]labelValue{
+			"service_instance_id":  {state: labelAnyValue},
+			"service_version":      {label: version.Version, state: labelSpecificValue},
+			"custom_resource_attr": {label: "resource_attr_test_value", state: labelSpecificValue},
+		},
+	},
+	{
+		name: "override service.instance.id",
+		userDefinedResource: map[string]string{
+			"service.instance.id": "my_instance_id",
+		},
+		expectedLabels: map[string]labelValue{
+			"service_instance_id": {label: "my_instance_id", state: labelSpecificValue},
+			"service_version":     {label: version.Version, state: labelSpecificValue},
+		},
+	},
+	{
+		name: "suppress service.instance.id",
+		userDefinedResource: map[string]string{
+			"service.instance.id": "",
+		},
+		expectedLabels: map[string]labelValue{
+			"service_instance_id": {state: labelNotPresent},
+			"service_version":     {label: version.Version, state: labelSpecificValue},
+		},
+	},
+	{
+		name: "override service.version",
+		userDefinedResource: map[string]string{
+			"service.version": "2022-05-20",
+		},
+		expectedLabels: map[string]labelValue{
+			"service_instance_id": {state: labelAnyValue},
+			"service_version":     {label: "2022-05-20", state: labelSpecificValue},
+		},
+	},
+	{
+		name: "suppress service.version",
+		userDefinedResource: map[string]string{
+			"service.version": "",
+		},
+		expectedLabels: map[string]labelValue{
+			"service_instance_id": {state: labelAnyValue},
+			"service_version":     {state: labelNotPresent},
+		},
+	},
+}
+
+func testCollectorStartHelper(t *testing.T, telemetry collectorTelemetryExporter, tc ownMetricsTestCase) {
 	factories, err := testcomponents.NewDefaultFactories()
 	require.NoError(t, err)
 	var once sync.Once
@@ -196,24 +293,18 @@ func testCollectorStartHelper(t *testing.T, telemetry collectorTelemetryExporter
 		filepath.Join("testdata", "otelcol-config.yaml"),
 	})
 
-	// Define some Resource attributes.
-	userDefinedResource := map[string]string{
-		"custom_resource_attr": "resource_attr_test_value",
-	}
-
 	// Prepare config properties to be merged with the main config.
-	extraCfgAsProps := []string{
+	extraCfgAsProps := map[string]string{
 		// Set the metrics address to expose own metrics on.
-		"service.telemetry.metrics.address=" + metricsAddr,
+		"service::telemetry::metrics::address": metricsAddr,
 	}
 	// Also include resource attributes under the service.telemetry.resource key.
-	for k, v := range userDefinedResource {
-		extraCfgAsProps = append(extraCfgAsProps,
-			"service.telemetry.resource."+k+"="+v)
+	for k, v := range tc.userDefinedResource {
+		extraCfgAsProps["service::telemetry::resource::"+k] = v
 	}
 
 	cfgSet.MapConverters = append([]config.MapConverterFunc{
-		overwritepropertiesmapconverter.New(extraCfgAsProps)},
+		mapConverter(extraCfgAsProps)},
 		cfgSet.MapConverters...,
 	)
 	cfgProvider, err := NewConfigProvider(cfgSet)
@@ -236,25 +327,7 @@ func testCollectorStartHelper(t *testing.T, telemetry collectorTelemetryExporter
 	assert.Equal(t, col.telemetry.Logger, col.GetLogger())
 	assert.True(t, loggingHookCalled)
 
-	// All labels added to all collector metrics by default are listed below.
-	// These labels are hard coded here in order to avoid inadvertent changes:
-	// at this point changing labels should be treated as a breaking changing
-	// and requires a good justification. The reason is that changes to metric
-	// names or labels can break alerting, dashboards, etc that are used to
-	// monitor the Collector in production deployments.
-	mandatoryLabels := map[string]string{
-		"service_instance_id": "", // Empty string means the value will not be asserted.
-	}
-	if AddCollectorVersionTag {
-		mandatoryLabels["service_version"] = version.Version
-	}
-
-	// Attributes of user defined Resource must also be included as labels of metrics.
-	for k, v := range userDefinedResource {
-		mandatoryLabels[k] = v
-	}
-
-	assertMetrics(t, metricsAddr, mandatoryLabels)
+	assertMetrics(t, metricsAddr, tc.expectedLabels)
 
 	assertZPages(t)
 	col.signalsChannel <- syscall.SIGTERM
@@ -264,15 +337,23 @@ func testCollectorStartHelper(t *testing.T, telemetry collectorTelemetryExporter
 }
 
 func TestCollectorStartWithOpenCensusMetrics(t *testing.T) {
-	testCollectorStartHelper(t, newColTelemetry(featuregate.NewRegistry()))
+	for _, tc := range ownMetricsTestCases {
+		t.Run(tc.name, func(t *testing.T) {
+			testCollectorStartHelper(t, newColTelemetry(featuregate.NewRegistry()), tc)
+		})
+	}
 }
 
 func TestCollectorStartWithOpenTelemetryMetrics(t *testing.T) {
-	colTel := newColTelemetry(featuregate.NewRegistry())
-	colTel.registry.Apply(map[string]bool{
-		useOtelForInternalMetricsfeatureGateID: true,
-	})
-	testCollectorStartHelper(t, colTel)
+	for _, tc := range ownMetricsTestCases {
+		t.Run(tc.name, func(t *testing.T) {
+			colTel := newColTelemetry(featuregate.NewRegistry())
+			colTel.registry.Apply(map[string]bool{
+				useOtelForInternalMetricsfeatureGateID: true,
+			})
+			testCollectorStartHelper(t, colTel, tc)
+		})
+	}
 }
 
 func TestCollectorShutdownBeforeRun(t *testing.T) {
@@ -341,7 +422,7 @@ func (tel *mockColTelemetry) shutdown() error {
 	return errors.New("err1")
 }
 
-func assertMetrics(t *testing.T, metricsAddr string, mandatoryLabels map[string]string) {
+func assertMetrics(t *testing.T, metricsAddr string, expectedLabels map[string]labelValue) {
 	client := &http.Client{}
 	resp, err := client.Get("http://" + metricsAddr + "/metrics")
 	require.NoError(t, err)
@@ -371,11 +452,15 @@ func assertMetrics(t *testing.T, metricsAddr string, mandatoryLabels map[string]
 				labelMap[*labelPair.Name] = *labelPair.Value
 			}
 
-			for k, v := range mandatoryLabels {
-				// require is used here so test fails with a single message.
-				require.NotEmpty(t, labelMap[k], "mandatory label %q not present", k)
-				if v != "" {
-					require.Equal(t, v, labelMap[k], "mandatory label %q value mismatch", k)
+			for k, v := range expectedLabels {
+				switch v.state {
+				case labelNotPresent:
+					_, present := labelMap[k]
+					assert.False(t, present, "label %q must not be present", k)
+				case labelSpecificValue:
+					require.Equal(t, v.label, labelMap[k], "mandatory label %q value mismatch", k)
+				case labelAnyValue:
+					assert.NotEmpty(t, labelMap[k], "mandatory label %q not present", k)
 				}
 			}
 		}

service/telemetry.go

@@ -114,17 +114,28 @@ func (tel *colTelemetry) initOnce(col *Collector) error {
 		resource = map[string]string{}
 	}
 
-	var instanceID string
-	instanceID = resource[semconv.AttributeServiceInstanceID]
-	if instanceID == "" {
+	if instanceID, ok := resource[semconv.AttributeServiceInstanceID]; !ok {
 		// AttributeServiceInstanceID is not specified in the config. Auto-generate one.
 		instanceUUID, _ := uuid.NewRandom()
 		instanceID = instanceUUID.String()
 		resource[semconv.AttributeServiceInstanceID] = instanceID
+	} else {
 	}
 
 	if AddCollectorVersionTag {
-		resource[semconv.AttributeServiceVersion] = version.Version
+		// AttributeServiceVersion is not specified in the config. Use the actual
+		// build version.
+		if _, ok := resource[semconv.AttributeServiceVersion]; !ok {
+			resource[semconv.AttributeServiceVersion] = version.Version
+		}
+	}
+
+	// Empty Resource attributes are an indication by the user to suppress and stop
+	// emitting the particular attribute.
+	for k, v := range resource {
+		if v == "" {
+			delete(resource, k)
+		}
 	}
 
 	var pe http.Handler