Add the WithEnv option

CHANGELOG.md

@@ -8,9 +8,18 @@ OpenTelemetry Go Automatic Instrumentation adheres to [Semantic Versioning](http
 
 ## [Unreleased]
 
+### Added
+
+- Add the `WithEnv` `InstrumentationOption` to configure `Instrumentation` to parse the environment.
+  The `Instrumentation` will no longer by default parse the environment.
+  This option needs to be used to enable environment parsing, and the order it is passed influences the environment precedence.
+  All options passed before this one will be overridden if there are conflicts, and those passed after will override the environment. ([#417](https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/417))
+
 ### Changed
 
 - Documentation no longer says that `SYS_PTRACE` capabilty is needed. ([#388](https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/388))
+- The `NewInstrumentation` no longer parses environment variables by default.
+  Use the new `WithEnv` option to enable environment parsing. ([#417](https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/417))
 
 ### Fixed
 

cli/main.go

@@ -49,7 +49,7 @@ func main() {
 	logger := newLogger().WithName("go.opentelemetry.io/auto")
 
 	logger.Info("building OpenTelemetry Go instrumentation ...")
-	inst, err := auto.NewInstrumentation()
+	inst, err := auto.NewInstrumentation(auto.WithEnv())
 	if err != nil {
 		logger.Error(err, "failed to create instrumentation")
 		return

instrumentation.go

@@ -42,8 +42,6 @@ const (
 	// envResourceAttrKey is the key for the environment variable value containing
 	// OpenTelemetry Resource attributes.
 	envResourceAttrKey = "OTEL_RESOURCE_ATTRIBUTES"
-	// serviceNameDefault is the default service name prefix used if a user does not provide one.
-	serviceNameDefault = "unknown_service"
 )
 
 // Instrumentation manages and controls all OpenTelemetry Go
@@ -74,6 +72,9 @@ func newLogger() logr.Logger {
 
 // NewInstrumentation returns a new [Instrumentation] configured with the
 // provided opts.
+//
+// If conflicting or duplicate options are provided, the last one will have
+// precedence and be used.
 func NewInstrumentation(opts ...InstrumentationOption) (*Instrumentation, error) {
 	// TODO: pass this in as an option.
 	//
@@ -89,7 +90,7 @@ func NewInstrumentation(opts ...InstrumentationOption) (*Instrumentation, error)
 	}
 
 	pa := process.NewAnalyzer(logger)
-	pid, err := pa.DiscoverProcessID(c.target)
+	pid, err := pa.DiscoverProcessID(&c.target)
 	if err != nil {
 		return nil, err
 	}
@@ -150,71 +151,37 @@ type InstrumentationOption interface {
 }
 
 type instConfig struct {
-	target      *process.TargetArgs
+	target      process.TargetArgs
 	serviceName string
 }
 
 func newInstConfig(opts []InstrumentationOption) instConfig {
-	c := instConfig{target: &process.TargetArgs{}}
+	var c instConfig
 	for _, opt := range opts {
 		if opt != nil {
 			c = opt.apply(c)
 		}
 	}
-	c = c.applyEnv()
-	return c
-}
 
-func (c instConfig) applyEnv() instConfig {
-	if v, ok := os.LookupEnv(envTargetExeKey); ok {
-		c.target.ExePath = v
-		// The environment variable takes precedence over a passed PID option.
-		c.target.Pid = 0
-	}
-	if v, ok := os.LookupEnv(envServiceNameKey); ok {
-		c.serviceName = v
-	} else {
-		c = c.applyResourceAtrrEnv()
-		if c.serviceName == "" {
-			c = c.setDefualtServiceName()
-		}
+	// Defaults.
+	if c.serviceName == "" {
+		c.serviceName = c.defualtServiceName()
 	}
-	return c
-}
 
-func (c instConfig) setDefualtServiceName() instConfig {
-	if c.target.ExePath != "" {
-		c.serviceName = fmt.Sprintf("%s:%s", serviceNameDefault, filepath.Base(c.target.ExePath))
-	} else {
-		c.serviceName = serviceNameDefault
-	}
 	return c
 }
 
-func (c instConfig) applyResourceAtrrEnv() instConfig {
-	attrs := strings.TrimSpace(os.Getenv(envResourceAttrKey))
-
-	if attrs == "" {
-		return c
-	}
-
-	pairs := strings.Split(attrs, ",")
-	for _, p := range pairs {
-		k, v, found := strings.Cut(p, "=")
-		if !found {
-			continue
-		}
-		key := strings.TrimSpace(k)
-		if key == string(semconv.ServiceNameKey) {
-			c.serviceName = strings.TrimSpace(v)
-		}
+func (c instConfig) defualtServiceName() string {
+	name := "unknown_service"
+	if c.target.ExePath != "" {
+		name = fmt.Sprintf("%s:%s", name, filepath.Base(c.target.ExePath))
 	}
-
-	return c
+	return name
 }
 
 func (c instConfig) validate() error {
-	if c.target == nil {
+	var zero process.TargetArgs
+	if c.target == zero {
 		return errUndefinedTarget
 	}
 	return c.target.Validate()
@@ -233,11 +200,12 @@ func (o fnOpt) apply(c instConfig) instConfig { return o(c) }
 // If multiple of these options are provided to an [Instrumentation], the last
 // one will be used.
 //
-// If OTEL_GO_AUTO_TARGET_EXE is defined it will take precedence over any value
-// passed here.
+// If OTEL_GO_AUTO_TARGET_EXE is defined, this option will conflict with
+// [WithEnv]. If both are used, the last one provided to an [Instrumentation]
+// will be used.
 func WithTarget(path string) InstrumentationOption {
 	return fnOpt(func(c instConfig) instConfig {
-		c.target.ExePath = path
+		c.target = process.TargetArgs{ExePath: path}
 		return c
 	})
 }
@@ -247,8 +215,9 @@ func WithTarget(path string) InstrumentationOption {
 // If multiple of these options are provided to an [Instrumentation], the last
 // one will be used.
 //
-// If OTEL_SERVICE_NAME is defined it will take precedence over any value
-// passed here.
+// If OTEL_SERVICE_NAME is defined or the service name is defined in
+// OTEL_RESOURCE_ATTRIBUTES, this option will conflict with [WithEnv]. If both
+// are used, the last one provided to an [Instrumentation] will be used.
 func WithServiceName(serviceName string) InstrumentationOption {
 	return fnOpt(func(c instConfig) instConfig {
 		c.serviceName = serviceName
@@ -265,11 +234,62 @@ func WithServiceName(serviceName string) InstrumentationOption {
 // If multiple of these options are provided to an [Instrumentation], the last
 // one will be used.
 //
-// If OTEL_GO_AUTO_TARGET_EXE is defined it will take precedence over any value
-// passed here.
+// If OTEL_GO_AUTO_TARGET_EXE is defined, this option will conflict with
+// [WithEnv]. If both are used, the last one provided to an [Instrumentation]
+// will be used.
 func WithPID(pid int) InstrumentationOption {
 	return fnOpt(func(c instConfig) instConfig {
-		c.target.Pid = pid
+		c.target = process.TargetArgs{Pid: pid}
 		return c
 	})
 }
+
+var lookupEnv = os.LookupEnv
+
+// WithEnv returns an [InstrumentationOption] that will configure
+// [Instrumentation] using the values defined by the following environment
+// variables:
+//
+//   - OTEL_GO_AUTO_TARGET_EXE: sets the target binary
+//   - OTEL_SERVICE_NAME (or OTEL_RESOURCE_ATTRIBUTES): sets the service name
+//
+// This option may conflict with [WithTarget], [WithPID], and [WithServiceName]
+// if their respective environment variable is defined. If more than one of
+// these options are used, the last one provided to an [Instrumentation] will
+// be used.
+func WithEnv() InstrumentationOption {
+	return fnOpt(func(c instConfig) instConfig {
+		if v, ok := lookupEnv(envTargetExeKey); ok {
+			c.target = process.TargetArgs{ExePath: v}
+		}
+		if v, ok := lookupServiceName(); ok {
+			c.serviceName = v
+		}
+		return c
+	})
+}
+
+func lookupServiceName() (string, bool) {
+	// Prioritize OTEL_SERVICE_NAME over OTEL_RESOURCE_ATTRIBUTES value.
+	if v, ok := lookupEnv(envServiceNameKey); ok {
+		return v, ok
+	}
+
+	v, ok := lookupEnv(envResourceAttrKey)
+	if !ok {
+		return "", false
+	}
+
+	for _, keyval := range strings.Split(strings.TrimSpace(v), ",") {
+		key, val, found := strings.Cut(keyval, "=")
+		if !found {
+			continue
+		}
+		key = strings.TrimSpace(key)
+		if key == string(semconv.ServiceNameKey) {
+			return strings.TrimSpace(val), true
+		}
+	}
+
+	return "", false
+}

instrumentation_test.go

@@ -16,11 +16,9 @@ package auto
 
 import (
 	"fmt"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-
 	semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
 )
 
@@ -33,38 +31,94 @@ func TestWithServiceName(t *testing.T) {
 
 	// No service name provided - check for default value
 	c = newInstConfig([]InstrumentationOption{})
-	assert.Equal(t, serviceNameDefault, c.serviceName)
-
-	// OTEL_RESOURCE_ATTRIBUTES
-	resServiceName := "resValue"
-	err := os.Setenv(envResourceAttrKey, fmt.Sprintf("key1=val1,%s=%s", string(semconv.ServiceNameKey), resServiceName))
-	if err != nil {
-		t.Error(err)
-	}
-	c = newInstConfig([]InstrumentationOption{WithServiceName((testServiceName))})
-	assert.Equal(t, resServiceName, c.serviceName)
-
-	// Add env var to take precedence
-	envServiceName := "env_serviceName"
-	err = os.Setenv(envServiceNameKey, envServiceName)
-	if err != nil {
-		t.Error(err)
-	}
-	c = newInstConfig([]InstrumentationOption{WithServiceName((testServiceName))})
-	assert.Equal(t, envServiceName, c.serviceName)
+	assert.Equal(t, c.defualtServiceName(), c.serviceName)
 }
 
 func TestWithPID(t *testing.T) {
-	// Current PID
-	currPID := os.Getpid()
-	c := newInstConfig([]InstrumentationOption{WithPID(currPID)})
-	currExe, err := os.Executable()
-	if err != nil {
-		t.Error(err)
-	}
-	assert.Equal(t, currPID, c.target.Pid)
+	c := newInstConfig([]InstrumentationOption{WithPID(1)})
+	assert.Equal(t, 1, c.target.Pid)
 
+	const exe = "./test/path/program/run.go"
 	// PID should override valid target exe
-	c = newInstConfig([]InstrumentationOption{WithPID(currPID), WithTarget(currExe)})
-	assert.Equal(t, currPID, c.target.Pid)
+	c = newInstConfig([]InstrumentationOption{WithTarget(exe), WithPID(1)})
+	assert.Equal(t, 1, c.target.Pid)
+	assert.Equal(t, "", c.target.ExePath)
+}
+
+func TestWithEnv(t *testing.T) {
+	t.Run("OTEL_GO_AUTO_TARGET_EXE", func(t *testing.T) {
+		const path = "./test/path/program/run.go"
+		mockEnv(t, map[string]string{"OTEL_GO_AUTO_TARGET_EXE": path})
+		c := newInstConfig([]InstrumentationOption{WithEnv()})
+		assert.Equal(t, path, c.target.ExePath)
+		assert.Equal(t, 0, c.target.Pid)
+	})
+
+	t.Run("OTEL_SERVICE_NAME", func(t *testing.T) {
+		const name = "test_service"
+		mockEnv(t, map[string]string{"OTEL_SERVICE_NAME": name})
+		c := newInstConfig([]InstrumentationOption{WithEnv()})
+		assert.Equal(t, name, c.serviceName)
+	})
+
+	t.Run("OTEL_RESOURCE_ATTRIBUTES", func(t *testing.T) {
+		const name = "test_service"
+		val := fmt.Sprintf("a=b,fubar,%s=%s,foo=bar", semconv.ServiceNameKey, name)
+		mockEnv(t, map[string]string{"OTEL_RESOURCE_ATTRIBUTES": val})
+		c := newInstConfig([]InstrumentationOption{WithEnv()})
+		assert.Equal(t, name, c.serviceName)
+	})
+}
+
+func TestOptionPrecedence(t *testing.T) {
+	const (
+		path = "./test/path/program/run.go"
+		name = "test_service"
+	)
+
+	t.Run("Env", func(t *testing.T) {
+		mockEnv(t, map[string]string{
+			"OTEL_GO_AUTO_TARGET_EXE": path,
+			"OTEL_SERVICE_NAME":       name,
+		})
+
+		// WithEnv passed last, it should have precedence.
+		opts := []InstrumentationOption{
+			WithPID(1),
+			WithServiceName("wrong"),
+			WithEnv(),
+		}
+		c := newInstConfig(opts)
+		assert.Equal(t, path, c.target.ExePath)
+		assert.Equal(t, 0, c.target.Pid)
+		assert.Equal(t, name, c.serviceName)
+	})
+
+	t.Run("Options", func(t *testing.T) {
+		mockEnv(t, map[string]string{
+			"OTEL_GO_AUTO_TARGET_EXE": path,
+			"OTEL_SERVICE_NAME":       "wrong",
+		})
+
+		// WithEnv passed first, it should be overridden.
+		opts := []InstrumentationOption{
+			WithEnv(),
+			WithPID(1),
+			WithServiceName(name),
+		}
+		c := newInstConfig(opts)
+		assert.Equal(t, "", c.target.ExePath)
+		assert.Equal(t, 1, c.target.Pid)
+		assert.Equal(t, name, c.serviceName)
+	})
+}
+
+func mockEnv(t *testing.T, env map[string]string) {
+	orig := lookupEnv
+	t.Cleanup(func() { lookupEnv = orig })
+
+	lookupEnv = func(key string) (string, bool) {
+		v, ok := env[key]
+		return v, ok
+	}
 }