diff --git a/CHANGELOG.md b/CHANGELOG.md index 78e00b637..1e276e200 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cli/main.go b/cli/main.go index a93899f11..2ce910fd1 100644 --- a/cli/main.go +++ b/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 diff --git a/instrumentation.go b/instrumentation.go index 451846f29..193fd45b8 100644 --- a/instrumentation.go +++ b/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 +} diff --git a/instrumentation_test.go b/instrumentation_test.go index c520c5258..b92d3e3fd 100644 --- a/instrumentation_test.go +++ b/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 + } }