From 830a0f74dff5bca7323bb329d8c6798cbf9cbf66 Mon Sep 17 00:00:00 2001 From: Luiz Lelis Date: Tue, 5 Mar 2024 12:55:21 -0300 Subject: [PATCH] chore: update `dotnet` instr. to command-line (#2690) * chore: update `dotnet` instr. to command-line * fix: `.NET` pkg instrumentation upgrade * fix: remove `debug` logs * fix: pod mutator tests for `.NET` instrumentations * fix: using capital N for `DotNet` --- .../dotnet-instrumentation-command-flag.yaml | 16 ++++++++ internal/config/main.go | 8 ++++ internal/config/options.go | 6 +++ main.go | 6 ++- pkg/constants/env.go | 1 + pkg/featuregate/featuregate.go | 6 --- pkg/instrumentation/podmutator.go | 2 +- pkg/instrumentation/podmutator_test.go | 38 ++++++++++--------- pkg/instrumentation/upgrade/upgrade.go | 14 +++---- pkg/instrumentation/upgrade/upgrade_test.go | 2 + 10 files changed, 66 insertions(+), 33 deletions(-) create mode 100644 .chloggen/dotnet-instrumentation-command-flag.yaml diff --git a/.chloggen/dotnet-instrumentation-command-flag.yaml b/.chloggen/dotnet-instrumentation-command-flag.yaml new file mode 100644 index 0000000000..b9e20c6e08 --- /dev/null +++ b/.chloggen/dotnet-instrumentation-command-flag.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: change dotnet instrumentation feature gate into command line flag --enable-dotnet-instrumentation + +# One or more tracking issues related to the change +issues: [2582, 2671] + +# (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: diff --git a/internal/config/main.go b/internal/config/main.go index b336ed4bce..7454200ab0 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -45,6 +45,7 @@ type Config struct { createRBACPermissions bool enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool + enableDotNetInstrumentation bool autoInstrumentationDotNetImage string autoInstrumentationGoImage string autoInstrumentationApacheHttpdImage string @@ -79,6 +80,7 @@ func New(opts ...Option) Config { createRBACPermissions: o.createRBACPermissions, enableMultiInstrumentation: o.enableMultiInstrumentation, enableApacheHttpdInstrumentation: o.enableApacheHttpdInstrumentation, + enableDotNetInstrumentation: o.enableDotNetInstrumentation, targetAllocatorImage: o.targetAllocatorImage, operatorOpAMPBridgeImage: o.operatorOpAMPBridgeImage, targetAllocatorConfigMapEntry: o.targetAllocatorConfigMapEntry, @@ -118,10 +120,16 @@ func (c *Config) EnableMultiInstrumentation() bool { return c.enableMultiInstrumentation } +// EnableApacheHttpdAutoInstrumentation is true when the operator supports ApacheHttpd auto instrumentation. func (c *Config) EnableApacheHttpdAutoInstrumentation() bool { return c.enableApacheHttpdInstrumentation } +// EnableDotNetAutoInstrumentation is true when the operator supports dotnet auto instrumentation. +func (c *Config) EnableDotNetAutoInstrumentation() bool { + return c.enableDotNetInstrumentation +} + // CollectorConfigMapEntry represents the configuration file name for the collector. Immutable. func (c *Config) CollectorConfigMapEntry() string { return c.collectorConfigMapEntry diff --git a/internal/config/options.go b/internal/config/options.go index a87385d1f4..e698c84a5b 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -44,6 +44,7 @@ type options struct { createRBACPermissions bool enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool + enableDotNetInstrumentation bool targetAllocatorConfigMapEntry string operatorOpAMPBridgeConfigMapEntry string targetAllocatorImage string @@ -92,6 +93,11 @@ func WithEnableApacheHttpdInstrumentation(s bool) Option { o.enableApacheHttpdInstrumentation = s } } +func WithEnableDotNetInstrumentation(s bool) Option { + return func(o *options) { + o.enableDotNetInstrumentation = s + } +} func WithTargetAllocatorConfigMapEntry(s string) Option { return func(o *options) { o.targetAllocatorConfigMapEntry = s diff --git a/main.go b/main.go index 1711f4810e..2f0d0d9b61 100644 --- a/main.go +++ b/main.go @@ -109,6 +109,7 @@ func main() { createRBACPermissions bool enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool + enableDotNetInstrumentation bool collectorImage string targetAllocatorImage string operatorOpAMPBridgeImage string @@ -132,7 +133,8 @@ func main() { "Enabling this will ensure there is only one active controller manager.") pflag.BoolVar(&createRBACPermissions, "create-rbac-permissions", false, "Automatically create RBAC permissions needed by the processors") pflag.BoolVar(&enableMultiInstrumentation, "enable-multi-instrumentation", false, "Controls whether the operator supports multi instrumentation") - pflag.BoolVar(&enableApacheHttpdInstrumentation, constants.FlagApacheHttpd, true, "controls whether the operator supports Apache HTTPD auto-instrumentation") + pflag.BoolVar(&enableApacheHttpdInstrumentation, constants.FlagApacheHttpd, true, "Controls whether the operator supports Apache HTTPD auto-instrumentation") + pflag.BoolVar(&enableDotNetInstrumentation, constants.FlagDotNet, true, "Controls whether the operator supports dotnet auto-instrumentation") stringFlagOrEnv(&collectorImage, "collector-image", "RELATED_IMAGE_COLLECTOR", fmt.Sprintf("ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector:%s", v.OpenTelemetryCollector), "The default OpenTelemetry collector image. This image is used when no image is specified in the CustomResource.") stringFlagOrEnv(&targetAllocatorImage, "target-allocator-image", "RELATED_IMAGE_TARGET_ALLOCATOR", fmt.Sprintf("ghcr.io/open-telemetry/opentelemetry-operator/target-allocator:%s", v.TargetAllocator), "The default OpenTelemetry target allocator image. This image is used when no image is specified in the CustomResource.") stringFlagOrEnv(&operatorOpAMPBridgeImage, "operator-opamp-bridge-image", "RELATED_IMAGE_OPERATOR_OPAMP_BRIDGE", fmt.Sprintf("ghcr.io/open-telemetry/opentelemetry-operator/operator-opamp-bridge:%s", v.OperatorOpAMPBridge), "The default OpenTelemetry Operator OpAMP Bridge image. This image is used when no image is specified in the CustomResource.") @@ -172,6 +174,7 @@ func main() { "labels-filter", labelsFilter, "enable-multi-instrumentation", enableMultiInstrumentation, "enable-apache-httpd-instrumentation", enableApacheHttpdInstrumentation, + "enable-dotnet-instrumentation", enableDotNetInstrumentation, ) restConfig := ctrl.GetConfigOrDie() @@ -190,6 +193,7 @@ func main() { config.WithCreateRBACPermissions(createRBACPermissions), config.WithEnableMultiInstrumentation(enableMultiInstrumentation), config.WithEnableApacheHttpdInstrumentation(enableApacheHttpdInstrumentation), + config.WithEnableDotNetInstrumentation(enableDotNetInstrumentation), config.WithTargetAllocatorImage(targetAllocatorImage), config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage), config.WithAutoInstrumentationJavaImage(autoInstrumentationJava), diff --git a/pkg/constants/env.go b/pkg/constants/env.go index 510adef6cb..64efe59aa2 100644 --- a/pkg/constants/env.go +++ b/pkg/constants/env.go @@ -36,4 +36,5 @@ const ( EnvNodeName = "OTEL_RESOURCE_ATTRIBUTES_NODE_NAME" FlagApacheHttpd = "enable-apache-httpd-instrumentation" + FlagDotNet = "enable-dotnet-instrumentation" ) diff --git a/pkg/featuregate/featuregate.go b/pkg/featuregate/featuregate.go index f12fb985b6..1269188c4f 100644 --- a/pkg/featuregate/featuregate.go +++ b/pkg/featuregate/featuregate.go @@ -25,12 +25,6 @@ const ( ) var ( - EnableDotnetAutoInstrumentationSupport = featuregate.GlobalRegistry().MustRegister( - "operator.autoinstrumentation.dotnet", - featuregate.StageBeta, - featuregate.WithRegisterDescription("controls whether the operator supports .NET auto-instrumentation"), - featuregate.WithRegisterFromVersion("v0.76.1"), - ) EnablePythonAutoInstrumentationSupport = featuregate.GlobalRegistry().MustRegister( "operator.autoinstrumentation.python", featuregate.StageBeta, diff --git a/pkg/instrumentation/podmutator.go b/pkg/instrumentation/podmutator.go index 28da01ac6a..004e8239dd 100644 --- a/pkg/instrumentation/podmutator.go +++ b/pkg/instrumentation/podmutator.go @@ -265,7 +265,7 @@ func (pm *instPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod c logger.Error(err, "failed to select an OpenTelemetry Instrumentation instance for this pod") return pod, err } - if featuregate.EnableDotnetAutoInstrumentationSupport.IsEnabled() || inst == nil { + if pm.config.EnableDotNetAutoInstrumentation() || inst == nil { insts.DotNet.Instrumentation = inst insts.DotNet.AdditionalAnnotations = map[string]string{annotationDotNetRuntime: annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationDotNetRuntime)} } else { diff --git a/pkg/instrumentation/podmutator_test.go b/pkg/instrumentation/podmutator_test.go index ffe5327a03..4fa270d8dc 100644 --- a/pkg/instrumentation/podmutator_test.go +++ b/pkg/instrumentation/podmutator_test.go @@ -1811,6 +1811,7 @@ func TestMutatePod(t *testing.T) { }, }, }, + config: config.New(config.WithEnableDotNetInstrumentation(true)), }, { name: "dotnet injection, by namespace annotations", @@ -1990,6 +1991,7 @@ func TestMutatePod(t *testing.T) { }, }, }, + config: config.New(config.WithEnableDotNetInstrumentation(true)), }, { name: "dotnet injection multiple containers, true", @@ -2265,6 +2267,7 @@ func TestMutatePod(t *testing.T) { }, }, }, + config: config.New(config.WithEnableDotNetInstrumentation(true)), }, { name: "dotnet injection feature gate disabled", @@ -2343,13 +2346,7 @@ func TestMutatePod(t *testing.T) { }, }, }, - setFeatureGates: func(t *testing.T) { - originalVal := featuregate.EnableDotnetAutoInstrumentationSupport.IsEnabled() - require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableDotnetAutoInstrumentationSupport.ID(), false)) - t.Cleanup(func() { - require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableDotnetAutoInstrumentationSupport.ID(), originalVal)) - }) - }, + config: config.New(config.WithEnableDotNetInstrumentation(false)), }, { name: "go injection, true", @@ -3845,7 +3842,10 @@ func TestMutatePod(t *testing.T) { }, }, }, - config: config.New(config.WithEnableMultiInstrumentation(true)), + config: config.New( + config.WithEnableMultiInstrumentation(true), + config.WithEnableDotNetInstrumentation(true), + ), }, { name: "multi instrumentation for multiple containers feature gate enabled, container-names not used", @@ -4503,7 +4503,10 @@ func TestMutatePod(t *testing.T) { }, }, }, - config: config.New(config.WithEnableMultiInstrumentation(true)), + config: config.New( + config.WithEnableMultiInstrumentation(true), + config.WithEnableDotNetInstrumentation(true), + ), }, { name: "multi instrumentation for multiple containers feature gate disabled, multiple instrumentation annotations set", @@ -4980,7 +4983,10 @@ func TestMutatePod(t *testing.T) { }, }, }, - config: config.New(config.WithEnableMultiInstrumentation(true)), + config: config.New( + config.WithEnableMultiInstrumentation(true), + config.WithEnableDotNetInstrumentation(true), + ), }, { name: "multi instrumentation feature gate disabled, instrumentation feature gate disabled and annotation set, multiple specific containers set", @@ -5080,14 +5086,10 @@ func TestMutatePod(t *testing.T) { }, }, }, - config: config.New(config.WithEnableMultiInstrumentation(true)), - setFeatureGates: func(t *testing.T) { - originalValDotNetInstr := featuregate.EnableDotnetAutoInstrumentationSupport.IsEnabled() - require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableDotnetAutoInstrumentationSupport.ID(), false)) - t.Cleanup(func() { - require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableDotnetAutoInstrumentationSupport.ID(), originalValDotNetInstr)) - }) - }, + config: config.New( + config.WithEnableMultiInstrumentation(true), + config.WithEnableDotNetInstrumentation(false), + ), }, } diff --git a/pkg/instrumentation/upgrade/upgrade.go b/pkg/instrumentation/upgrade/upgrade.go index 0788ac3537..831271c2a8 100644 --- a/pkg/instrumentation/upgrade/upgrade.go +++ b/pkg/instrumentation/upgrade/upgrade.go @@ -35,7 +35,6 @@ var ( constants.AnnotationDefaultAutoInstrumentationJava: featuregate.EnableJavaAutoInstrumentationSupport, constants.AnnotationDefaultAutoInstrumentationNodeJS: featuregate.EnableNodeJSAutoInstrumentationSupport, constants.AnnotationDefaultAutoInstrumentationPython: featuregate.EnablePythonAutoInstrumentationSupport, - constants.AnnotationDefaultAutoInstrumentationDotNet: featuregate.EnableDotnetAutoInstrumentationSupport, constants.AnnotationDefaultAutoInstrumentationGo: featuregate.EnableGoAutoInstrumentationSupport, constants.AnnotationDefaultAutoInstrumentationNginx: featuregate.EnableNginxAutoInstrumentationSupport, } @@ -62,7 +61,8 @@ type InstrumentationUpgrade struct { func NewInstrumentationUpgrade(client client.Client, logger logr.Logger, recorder record.EventRecorder, cfg config.Config) *InstrumentationUpgrade { defaultAnnotationToConfig := map[string]autoInstConfig{ - constants.AnnotationDefaultAutoInstrumentationApacheHttpd: autoInstConfig{constants.FlagApacheHttpd, cfg.EnableApacheHttpdAutoInstrumentation()}, + constants.AnnotationDefaultAutoInstrumentationApacheHttpd: {constants.FlagApacheHttpd, cfg.EnableApacheHttpdAutoInstrumentation()}, + constants.AnnotationDefaultAutoInstrumentationDotNet: {constants.FlagDotNet, cfg.EnableDotNetAutoInstrumentation()}, } return &InstrumentationUpgrade{ @@ -127,6 +127,11 @@ func (u *InstrumentationUpgrade) upgrade(_ context.Context, inst v1alpha1.Instru upgraded.Spec.ApacheHttpd.Image = u.DefaultAutoInstApacheHttpd upgraded.Annotations[annotation] = u.DefaultAutoInstApacheHttpd } + case constants.AnnotationDefaultAutoInstrumentationDotNet: + if inst.Spec.DotNet.Image == autoInst { + upgraded.Spec.DotNet.Image = u.DefaultAutoInstDotNet + upgraded.Annotations[annotation] = u.DefaultAutoInstDotNet + } } } else { u.Logger.Error(nil, "autoinstrumentation not enabled for this language", "flag", config.id) @@ -155,11 +160,6 @@ func (u *InstrumentationUpgrade) upgrade(_ context.Context, inst v1alpha1.Instru upgraded.Spec.Python.Image = u.DefaultAutoInstPython upgraded.Annotations[annotation] = u.DefaultAutoInstPython } - case constants.AnnotationDefaultAutoInstrumentationDotNet: - if inst.Spec.DotNet.Image == autoInst { - upgraded.Spec.DotNet.Image = u.DefaultAutoInstDotNet - upgraded.Annotations[annotation] = u.DefaultAutoInstDotNet - } case constants.AnnotationDefaultAutoInstrumentationGo: if inst.Spec.Go.Image == autoInst { upgraded.Spec.Go.Image = u.DefaultAutoInstGo diff --git a/pkg/instrumentation/upgrade/upgrade_test.go b/pkg/instrumentation/upgrade/upgrade_test.go index 7a7ea8dc4b..9e9aa7547f 100644 --- a/pkg/instrumentation/upgrade/upgrade_test.go +++ b/pkg/instrumentation/upgrade/upgrade_test.go @@ -79,6 +79,7 @@ func TestUpgrade(t *testing.T) { config.WithAutoInstrumentationApacheHttpdImage("apache-httpd:1"), config.WithAutoInstrumentationNginxImage("nginx:1"), config.WithEnableApacheHttpdInstrumentation(true), + config.WithEnableDotNetInstrumentation(true), ), ).Default(context.Background(), inst) assert.Nil(t, err) @@ -101,6 +102,7 @@ func TestUpgrade(t *testing.T) { config.WithAutoInstrumentationApacheHttpdImage("apache-httpd:2"), config.WithAutoInstrumentationNginxImage("nginx:2"), config.WithEnableApacheHttpdInstrumentation(true), + config.WithEnableDotNetInstrumentation(true), ) up := NewInstrumentationUpgrade(k8sClient, ctrl.Log.WithName("instrumentation-upgrade"), &record.FakeRecorder{}, cfg)