changes from feedback

Author: zeitlinger

.chloggen/resource-attribute-from-annotations.yaml

@@ -1,13 +1,23 @@
-change_type: breaking
+change_type: enhancement
 
 component: auto-instrumentation
 
-note: Add support for k8s labels such as app.kubernetes.io/name and annotations for resource attributes
+note: Add support for k8s labels such as app.kubernetes.io/name for resource attributes
 
 issues: [3112]
 
 subtext: |
-  It's a breaking change because these annotations are well-known and widely used in the Kubernetes ecosystem. The following labels are supported:
+  You can opt-in as follows:
+  ```yaml
+  apiVersion: opentelemetry.io/v1alpha1
+  kind: Instrumentation
+  metadata:
+    name: my-instrumentation
+  spec:
+    defaults:
+      useLabelsForResourceAttributes: true
+  ```
+  The following labels are supported:
     - `app.kubernetes.io/name` becomes `service.name`
     - `app.kubernetes.io/version` becomes `service.version`
     - `app.kubernetes.io/part-of` becomes `service.namespace`

README.md

@@ -735,7 +735,7 @@ spec:
     image: your-image:tag
 ```
 
-You can also use labels to set resource attributes. 
+You can also use common labels to set resource attributes. 
 
 The following labels are supported:
 - `app.kubernetes.io/name` becomes `service.name`
@@ -754,11 +754,36 @@ metadata:
     app.kubernetes.io/part-of: "shop"
     app.kubernetes.io/instance: "my-service-123"
 spec:
-    containers:
-    - name: main-container
-      image: your-image:tag
+  containers:
+  - name: main-container
+    image: your-image:tag
+```
+
+This requires an explicit opt-in as follows:
+
+```yaml
+apiVersion: opentelemetry.io/v1alpha1
+kind: Instrumentation
+metadata:
+  name: my-instrumentation
+spec:
+  defaults:
+    useLabelsForResourceAttributes: true
 ```
 
+#### Priority for setting resource attributes
+
+The priority for setting resource attributes is as follows (first found wins):
+
+- Resource attributes set via `OTEL_RESOURCE_ATTRIBUTES` and `OTEL_SERVICE_NAME` environment variables
+- Resource attributes calculated from the pod's metadata (e.g. `k8s.pod.name`)
+- Resource attributes set via the `Instrumentation` CR (in the `spec.resource.resourceAttributes` section)
+- Resource attributes set via annotations (with the `resource.opentelemetry.io/` prefix)
+- Resource attributes set via labels (e.g. `app.kubernetes.io/name`)
+
+This priority is applied for each resource attribute separately, so it is possible to set some attributes via 
+annotations and others via labels.
+
 ## Compatibility matrix
 
 ### OpenTelemetry Operator vs. OpenTelemetry Collector

apis/v1alpha1/instrumentation_types.go

@@ -40,6 +40,9 @@ type InstrumentationSpec struct {
 	// +optional
 	Sampler `json:"sampler,omitempty"`
 
+	// Defaults defines default values for the instrumentation.
+	Defaults Defaults `json:"defaults,omitempty"`
+
 	// Env defines common env vars. There are four layers for env vars' definitions and
 	// the precedence order is: `original container env vars` > `language specific env vars` > `common env vars` > `instrument spec configs' vars`.
 	// If the former var had been defined, then the other vars would be ignored.
@@ -114,6 +117,12 @@ type Sampler struct {
 	Argument string `json:"argument,omitempty"`
 }
 
+// Defaults defines default values for the instrumentation.
+type Defaults struct {
+	// UseLabelsForResourceAttributes defines whether to use common labels for resource attributes.
+	UseLabelsForResourceAttributes bool `json:"useLabelsForResourceAttributes,omitempty"`
+}
+
 // Java defines Java SDK and instrumentation configuration.
 type Java struct {
 	// Image is a container image with javaagent auto-instrumentation JAR.

pkg/constants/env.go

@@ -31,10 +31,10 @@ const (
 	AnnotationDefaultAutoInstrumentationApacheHttpd = InstrumentationPrefix + "default-auto-instrumentation-apache-httpd-image"
 	AnnotationDefaultAutoInstrumentationNginx       = InstrumentationPrefix + "default-auto-instrumentation-nginx-image"
 
-	AnnotationAppName     = "app.kubernetes.io/name"
-	AnnotationAppInstance = "app.kubernetes.io/instance"
-	AnnotationAppVersion  = "app.kubernetes.io/version"
-	AnnotationAppPartOf   = "app.kubernetes.io/part-of"
+	LabelAppName     = "app.kubernetes.io/name"
+	LabelAppInstance = "app.kubernetes.io/instance"
+	LabelAppVersion  = "app.kubernetes.io/version"
+	LabelAppPartOf   = "app.kubernetes.io/part-of"
 
 	ResourceAttributeAnnotationPrefix = "resource.opentelemetry.io/"
 

pkg/instrumentation/apachehttpd.go

@@ -59,7 +59,7 @@ const (
 	6) Inject mounting of volumes / files into appropriate directories in application container
 */
 
-func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod corev1.Pod, index int, otlpEndpoint string, resourceMap map[string]string) corev1.Pod {
+func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod corev1.Pod, useLabelsForResourceAttributes bool, index int, otlpEndpoint string, resourceMap map[string]string) corev1.Pod {
 
 	// caller checks if there is at least one container
 	container := &pod.Spec.Containers[index]
@@ -95,7 +95,7 @@ func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod
 			MountPath: apacheAgentConfDirFull,
 		})
 		// remove resource requirements since those are then reserved for the lifetime of a pod
-		// and we definitely do not need them for the init container for cp command
+		// ]and we definitely do not need them for the init container for cp command
 		cloneContainer.Resources = apacheSpec.Resources
 		// remove livenessProbe, readinessProbe, and startupProbe, since not supported on init containers
 		cloneContainer.LivenessProbe = nil
@@ -162,7 +162,7 @@ func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod
 			Env: []corev1.EnvVar{
 				{
 					Name:  apacheAttributesEnvVar,
-					Value: getApacheOtelConfig(pod, apacheSpec, index, otlpEndpoint, resourceMap),
+					Value: getApacheOtelConfig(pod, useLabelsForResourceAttributes, apacheSpec, index, otlpEndpoint, resourceMap),
 				},
 				{Name: apacheServiceInstanceIdEnvVar,
 					ValueFrom: &corev1.EnvVarSource{
@@ -201,7 +201,7 @@ func isApacheInitContainerMissing(pod corev1.Pod, containerName string) bool {
 
 // Calculate Apache HTTPD agent configuration file based on attributes provided by the injection rules
 // and by the pod values.
-func getApacheOtelConfig(pod corev1.Pod, apacheSpec v1alpha1.ApacheHttpd, index int, otelEndpoint string, resourceMap map[string]string) string {
+func getApacheOtelConfig(pod corev1.Pod, useLabelsForResourceAttributes bool, apacheSpec v1alpha1.ApacheHttpd, index int, otelEndpoint string, resourceMap map[string]string) string {
 	template := `
 #Load the Otel Webserver SDK
 LoadFile %[1]s/sdk_lib/lib/libopentelemetry_common.so
@@ -222,7 +222,7 @@ LoadModule otel_apache_module %[1]s/WebServerModule/Apache/libmod_apache_otel%[2
 	if otelEndpoint == "" {
 		otelEndpoint = "http://localhost:4317/"
 	}
-	serviceName := chooseServiceName(pod, resourceMap, index)
+	serviceName := chooseServiceName(pod, useLabelsForResourceAttributes, resourceMap, index)
 	serviceNamespace := pod.GetNamespace()
 	if len(serviceNamespace) == 0 {
 		serviceNamespace = resourceMap[string(semconv.K8SNamespaceNameKey)]

pkg/instrumentation/apachehttpd_test.go

@@ -417,7 +417,7 @@ func TestInjectApacheHttpdagent(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			pod := injectApacheHttpdagent(logr.Discard(), test.ApacheHttpd, test.pod, 0, "http://otlp-endpoint:4317", resourceMap)
+			pod := injectApacheHttpdagent(logr.Discard(), test.ApacheHttpd, test.pod, false, 0, "http://otlp-endpoint:4317", resourceMap)
 			assert.Equal(t, test.expected, pod)
 		})
 	}
@@ -527,7 +527,7 @@ func TestInjectApacheHttpdagentUnknownNamespace(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			pod := injectApacheHttpdagent(logr.Discard(), test.ApacheHttpd, test.pod, 0, "http://otlp-endpoint:4317", resourceMap)
+			pod := injectApacheHttpdagent(logr.Discard(), test.ApacheHttpd, test.pod, false, 0, "http://otlp-endpoint:4317", resourceMap)
 			assert.Equal(t, test.expected, pod)
 		})
 	}

pkg/instrumentation/nginx.go

@@ -62,7 +62,7 @@ const (
 	6) Inject mounting of volumes / files into appropriate directories in the application container
 */
 
-func injectNginxSDK(_ logr.Logger, nginxSpec v1alpha1.Nginx, pod corev1.Pod, index int, otlpEndpoint string, resourceMap map[string]string) corev1.Pod {
+func injectNginxSDK(_ logr.Logger, nginxSpec v1alpha1.Nginx, pod corev1.Pod, useLabelsForResourceAttributes bool, index int, otlpEndpoint string, resourceMap map[string]string) corev1.Pod {
 
 	// caller checks if there is at least one container
 	container := &pod.Spec.Containers[index]
@@ -217,7 +217,7 @@ mv ${NGINX_AGENT_CONF_DIR_FULL}/opentelemetry_agent.conf  ${NGINX_AGENT_CONF_DIR
 			Env: []corev1.EnvVar{
 				{
 					Name:  nginxAttributesEnvVar,
-					Value: getNginxOtelConfig(pod, nginxSpec, index, otlpEndpoint, resourceMap),
+					Value: getNginxOtelConfig(pod, useLabelsForResourceAttributes, nginxSpec, index, otlpEndpoint, resourceMap),
 				},
 				{
 					Name:  "OTEL_NGINX_I13N_SCRIPT",
@@ -277,12 +277,12 @@ func isNginxInitContainerMissing(pod corev1.Pod, containerName string) bool {
 
 // Calculate Nginx agent configuration file based on attributes provided by the injection rules
 // and by the pod values.
-func getNginxOtelConfig(pod corev1.Pod, nginxSpec v1alpha1.Nginx, index int, otelEndpoint string, resourceMap map[string]string) string {
+func getNginxOtelConfig(pod corev1.Pod, useLabelsForResourceAttributes bool, nginxSpec v1alpha1.Nginx, index int, otelEndpoint string, resourceMap map[string]string) string {
 
 	if otelEndpoint == "" {
 		otelEndpoint = "http://localhost:4317/"
 	}
-	serviceName := chooseServiceName(pod, resourceMap, index)
+	serviceName := chooseServiceName(pod, useLabelsForResourceAttributes, resourceMap, index)
 	serviceNamespace := pod.GetNamespace()
 	if len(serviceNamespace) == 0 {
 		serviceNamespace = resourceMap[string(semconv.K8SNamespaceNameKey)]

pkg/instrumentation/nginx_test.go

@@ -477,7 +477,7 @@ func TestInjectNginxSDK(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			pod := injectNginxSDK(logr.Discard(), test.Nginx, test.pod, 0, "http://otlp-endpoint:4317", resourceMap)
+			pod := injectNginxSDK(logr.Discard(), test.Nginx, test.pod, false, 0, "http://otlp-endpoint:4317", resourceMap)
 			assert.Equal(t, test.expected, pod)
 		})
 	}
@@ -600,7 +600,7 @@ func TestInjectNginxUnknownNamespace(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			pod := injectNginxSDK(logr.Discard(), test.Nginx, test.pod, 0, "http://otlp-endpoint:4317", resourceMap)
+			pod := injectNginxSDK(logr.Discard(), test.Nginx, test.pod, false, 0, "http://otlp-endpoint:4317", resourceMap)
 			assert.Equal(t, test.expected, pod)
 		})
 	}