NodeJS instrumentation featuregates into cli (#2834)

* NodeJS instrumentation featuregates into cli Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Added complement to featuregate Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Fixed Linters Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Fixed Linters Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Added e2e parameters Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Fixed e2e test Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Fixed e2e test Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Fixed e2e test Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Fixed e2e test Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Removed feature flags Signed-off-by: Yuri Sa <yurimsa@gmail.com> * Removed feature flags Signed-off-by: Yuri Sa <yurimsa@gmail.com> --------- Signed-off-by: Yuri Sa <yurimsa@gmail.com>
open-telemetry · Apr 30, 2024 · 48bc207 · 48bc207
1 parent 69e84a8
commit 48bc207
Show file tree

Hide file tree

Showing 15 changed files with 61 additions and 71 deletions.
diff --git a/.chloggen/change-instrumentation-feature-gates.yaml b/.chloggen/change-instrumentation-feature-gates.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. collector, target allocator, auto-instrumentation, opamp, github action)
+component: 'operator'
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: remove featuregate `operator.autoinstrumentation.nodejs`. Use command line flag `--enable-nodejs-instrumentation` instead
+
+# One or more tracking issues related to the change
+issues: [2674]
+
+# (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/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml
@@ -35,9 +35,9 @@ jobs:
          - e2e-metadata-filters
        include:
          - group: e2e-instrumentation
-           setup: "add-operator-arg OPERATOR_ARG=--enable-go-instrumentation prepare-e2e"
+           setup: "add-instrumentation-params prepare-e2e"
          - group: e2e-multi-instrumentation
-           setup: "add-operator-arg OPERATOR_ARG=--enable-multi-instrumentation prepare-e2e"
+           setup: "add-multi-instrumentation-params prepare-e2e"
          - group: e2e-metadata-filters
            setup: "add-operator-arg OPERATOR_ARG='--annotations-filter=*filter.out --labels=*filter.out' prepare-e2e"
 

diff --git a/Makefile b/Makefile
@@ -145,6 +145,14 @@ add-operator-arg: manifests kustomize
 add-image-targetallocator:
 	@$(MAKE) add-operator-arg OPERATOR_ARG=--target-allocator-image=$(TARGETALLOCATOR_IMG)
 
+.PHONY: add-instrumentation-params
+add-instrumentation-params:
+	@$(MAKE) add-operator-arg OPERATOR_ARG=--enable-go-instrumentation=true
+
+.PHONY: add-multi-instrumentation-params
+add-multi-instrumentation-params:
+	@$(MAKE) add-operator-arg OPERATOR_ARG=--enable-multi-instrumentation
+
 .PHONY: add-image-opampbridge
 add-image-opampbridge:
 	@$(MAKE) add-operator-arg OPERATOR_ARG=--operator-opamp-bridge-image=$(OPERATOROPAMPBRIDGE_IMG)

diff --git a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml
@@ -498,7 +498,6 @@ spec:
                 - --enable-leader-election
                 - --zap-log-level=info
                 - --zap-time-encoding=rfc3339nano
-                - --feature-gates=+operator.autoinstrumentation.go
                 - --enable-nginx-instrumentation=true
                 image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.98.0
                 livenessProbe:

diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml
@@ -32,7 +32,6 @@ spec:
         - "--enable-leader-election"
         - "--zap-log-level=info"
         - "--zap-time-encoding=rfc3339nano"
-        - "--feature-gates=+operator.autoinstrumentation.go"
         - "--enable-nginx-instrumentation=true"
         image: controller
         name: manager

diff --git a/internal/config/main.go b/internal/config/main.go
@@ -50,6 +50,7 @@ type Config struct {
 	enableGoInstrumentation             bool
 	enableNginxInstrumentation          bool
 	enablePythonInstrumentation         bool
+	enableNodeJSInstrumentation         bool
 	enableJavaInstrumentation           bool
 	autoInstrumentationDotNetImage      string
 	autoInstrumentationGoImage          string
@@ -94,6 +95,7 @@ func New(opts ...Option) Config {
 		enableGoInstrumentation:             o.enableGoInstrumentation,
 		enableNginxInstrumentation:          o.enableNginxInstrumentation,
 		enablePythonInstrumentation:         o.enablePythonInstrumentation,
+		enableNodeJSInstrumentation:         o.enableNodeJSInstrumentation,
 		enableJavaInstrumentation:           o.enableJavaInstrumentation,
 		targetAllocatorImage:                o.targetAllocatorImage,
 		operatorOpAMPBridgeImage:            o.operatorOpAMPBridgeImage,
@@ -172,6 +174,11 @@ func (c *Config) EnablePythonAutoInstrumentation() bool {
 	return c.enablePythonInstrumentation
 }
 
+// EnableNodeJSAutoInstrumentation is true when the operator supports dotnet auto instrumentation.
+func (c *Config) EnableNodeJSAutoInstrumentation() bool {
+	return c.enableNodeJSInstrumentation
+}
+
 // 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
@@ -49,6 +49,7 @@ type options struct {
 	enableGoInstrumentation             bool
 	enableNginxInstrumentation          bool
 	enablePythonInstrumentation         bool
+	enableNodeJSInstrumentation         bool
 	enableJavaInstrumentation           bool
 	targetAllocatorConfigMapEntry       string
 	operatorOpAMPBridgeConfigMapEntry   string
@@ -125,6 +126,11 @@ func WithEnablePythonInstrumentation(s bool) Option {
 		o.enablePythonInstrumentation = s
 	}
 }
+func WithEnableNodeJSInstrumentation(s bool) Option {
+	return func(o *options) {
+		o.enableNodeJSInstrumentation = s
+	}
+}
 func WithTargetAllocatorConfigMapEntry(s string) Option {
 	return func(o *options) {
 		o.targetAllocatorConfigMapEntry = s

diff --git a/main.go b/main.go
@@ -115,6 +115,7 @@ func main() {
 		enableGoInstrumentation          bool
 		enablePythonInstrumentation      bool
 		enableNginxInstrumentation       bool
+		enableNodeJSInstrumentation      bool
 		enableJavaInstrumentation        bool
 		collectorImage                   string
 		targetAllocatorImage             string
@@ -145,6 +146,7 @@ func main() {
 	pflag.BoolVar(&enableGoInstrumentation, constants.FlagGo, false, "Controls whether the operator supports Go auto-instrumentation")
 	pflag.BoolVar(&enablePythonInstrumentation, constants.FlagPython, true, "Controls whether the operator supports python auto-instrumentation")
 	pflag.BoolVar(&enableNginxInstrumentation, constants.FlagNginx, false, "Controls whether the operator supports nginx auto-instrumentation")
+	pflag.BoolVar(&enableNodeJSInstrumentation, constants.FlagNodeJS, true, "Controls whether the operator supports nodejs auto-instrumentation")
 	pflag.BoolVar(&enableJavaInstrumentation, constants.FlagJava, true, "Controls whether the operator supports java 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.")
@@ -191,6 +193,7 @@ func main() {
 		"enable-go-instrumentation", enableGoInstrumentation,
 		"enable-python-instrumentation", enablePythonInstrumentation,
 		"enable-nginx-instrumentation", enableNginxInstrumentation,
+		"enable-nodejs-instrumentation", enableNodeJSInstrumentation,
 		"enable-java-instrumentation", enableJavaInstrumentation,
 	)
 
@@ -214,6 +217,7 @@ func main() {
 		config.WithEnableGoInstrumentation(enableGoInstrumentation),
 		config.WithEnableNginxInstrumentation(enableNginxInstrumentation),
 		config.WithEnablePythonInstrumentation(enablePythonInstrumentation),
+		config.WithEnableNodeJSInstrumentation(enableNodeJSInstrumentation),
 		config.WithEnableJavaInstrumentation(enableJavaInstrumentation),
 		config.WithTargetAllocatorImage(targetAllocatorImage),
 		config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage),

diff --git a/pkg/constants/env.go b/pkg/constants/env.go
@@ -42,5 +42,6 @@ const (
 	FlagGo          = "enable-go-instrumentation"
 	FlagPython      = "enable-python-instrumentation"
 	FlagNginx       = "enable-nginx-instrumentation"
+	FlagNodeJS      = "enable-nodejs-instrumentation"
 	FlagJava        = "enable-java-instrumentation"
 )
diff --git a/pkg/featuregate/featuregate.go b/pkg/featuregate/featuregate.go
@@ -25,25 +25,6 @@ const (
 )
 
 var (
-	EnableNodeJSAutoInstrumentationSupport = featuregate.GlobalRegistry().MustRegister(
-		"operator.autoinstrumentation.nodejs",
-		featuregate.StageBeta,
-		featuregate.WithRegisterDescription("controls whether the operator supports NodeJS auto-instrumentation"),
-		featuregate.WithRegisterFromVersion("v0.76.1"),
-	)
-	EnableNginxAutoInstrumentationSupport = featuregate.GlobalRegistry().MustRegister(
-		"operator.autoinstrumentation.nginx",
-		featuregate.StageAlpha,
-		featuregate.WithRegisterDescription("controls whether the operator supports Nginx auto-instrumentation"),
-		featuregate.WithRegisterFromVersion("v0.86.0"),
-	)
-	EnableGoAutoInstrumentationSupport = featuregate.GlobalRegistry().MustRegister(
-		"operator.autoinstrumentation.go",
-		featuregate.StageAlpha,
-		featuregate.WithRegisterDescription("controls whether the operator supports Golang auto-instrumentation"),
-		featuregate.WithRegisterFromVersion("v0.77.0"),
-	)
-
 	// PrometheusOperatorIsAvailable is the feature gate that enables features associated to the Prometheus Operator.
 	PrometheusOperatorIsAvailable = featuregate.GlobalRegistry().MustRegister(
 		"operator.observability.prometheus",

diff --git a/pkg/instrumentation/podmutator.go b/pkg/instrumentation/podmutator.go
@@ -29,7 +29,6 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
 var (
@@ -241,7 +240,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.EnableNodeJSAutoInstrumentationSupport.IsEnabled() || inst == nil {
+	if pm.config.EnableNodeJSAutoInstrumentation() || inst == nil {
 		insts.NodeJS.Instrumentation = inst
 	} else {
 		logger.Error(nil, "support for NodeJS auto instrumentation is not enabled")

diff --git a/pkg/instrumentation/podmutator_test.go b/pkg/instrumentation/podmutator_test.go
@@ -22,14 +22,12 @@ import (
 	"github.com/go-logr/logr"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	colfeaturegate "go.opentelemetry.io/collector/featuregate"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/tools/record"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
 func TestMutatePod(t *testing.T) {
@@ -815,6 +813,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
+			config: config.New(config.WithEnableNodeJSInstrumentation(true)),
 		},
 		{
 			name: "nodejs injection multiple containers, true",
@@ -1086,6 +1085,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
+			config: config.New(config.WithEnableNodeJSInstrumentation(true)),
 		},
 		{
 			name: "nodejs injection feature gate disabled",
@@ -1168,13 +1168,6 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableNodeJSAutoInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableNodeJSAutoInstrumentationSupport.ID(), false))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableNodeJSAutoInstrumentationSupport.ID(), originalVal))
-				})
-			},
 		},
 		{
 			name: "python injection, true",
@@ -4201,6 +4194,7 @@ func TestMutatePod(t *testing.T) {
 				config.WithEnableMultiInstrumentation(true),
 				config.WithEnablePythonInstrumentation(true),
 				config.WithEnableDotNetInstrumentation(true),
+				config.WithEnableNodeJSInstrumentation(true),
 			),
 		},
 		{
@@ -4991,6 +4985,7 @@ func TestMutatePod(t *testing.T) {
 				config.WithEnableMultiInstrumentation(true),
 				config.WithEnableDotNetInstrumentation(true),
 				config.WithEnablePythonInstrumentation(true),
+				config.WithEnableNodeJSInstrumentation(true),
 			),
 		},
 		{
@@ -5145,7 +5140,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			config: config.New(config.WithEnableMultiInstrumentation(false)),
+			config: config.New(config.WithEnableMultiInstrumentation(false), config.WithEnableJavaInstrumentation(false)),
 		},
 		{
 			name: "multi instrumentation feature gate enabled, multiple instrumentation annotations set, no containers",
@@ -5289,7 +5284,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			config: config.New(config.WithEnableMultiInstrumentation(true)),
+			config: config.New(config.WithEnableMultiInstrumentation(true), config.WithEnableJavaInstrumentation(false)),
 		},
 		{
 			name: "multi instrumentation feature gate enabled, single instrumentation annotation set, no containers",
@@ -5590,6 +5585,7 @@ func TestMutatePod(t *testing.T) {
 			config: config.New(
 				config.WithEnableMultiInstrumentation(true),
 				config.WithEnableDotNetInstrumentation(false),
+				config.WithEnableNodeJSInstrumentation(false),
 			),
 		},
 	}

diff --git a/pkg/instrumentation/upgrade/upgrade.go b/pkg/instrumentation/upgrade/upgrade.go
@@ -20,20 +20,12 @@ import (
 	"reflect"
 
 	"github.com/go-logr/logr"
-	featuregate2 "go.opentelemetry.io/collector/featuregate"
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/constants"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
-)
-
-var (
-	defaultAnnotationToGate = map[string]*featuregate2.Gate{
-		constants.AnnotationDefaultAutoInstrumentationNodeJS: featuregate.EnableNodeJSAutoInstrumentationSupport,
-	}
 )
 
 type autoInstConfig struct {
@@ -62,6 +54,7 @@ func NewInstrumentationUpgrade(client client.Client, logger logr.Logger, recorde
 		constants.AnnotationDefaultAutoInstrumentationGo:          {constants.FlagGo, cfg.EnableGoAutoInstrumentation()},
 		constants.AnnotationDefaultAutoInstrumentationNginx:       {constants.FlagNginx, cfg.EnableNginxAutoInstrumentation()},
 		constants.AnnotationDefaultAutoInstrumentationPython:      {constants.FlagPython, cfg.EnablePythonAutoInstrumentation()},
+		constants.AnnotationDefaultAutoInstrumentationNodeJS:      {constants.FlagNodeJS, cfg.EnableNodeJSAutoInstrumentation()},
 		constants.AnnotationDefaultAutoInstrumentationJava:        {constants.FlagJava, cfg.EnableJavaAutoInstrumentation()},
 	}
 
@@ -147,43 +140,23 @@ func (u *InstrumentationUpgrade) upgrade(_ context.Context, inst v1alpha1.Instru
 						upgraded.Spec.Python.Image = u.DefaultAutoInstPython
 						upgraded.Annotations[annotation] = u.DefaultAutoInstPython
 					}
+				case constants.AnnotationDefaultAutoInstrumentationNodeJS:
+					if inst.Spec.NodeJS.Image == autoInst {
+						upgraded.Spec.NodeJS.Image = u.DefaultAutoInstNodeJS
+						upgraded.Annotations[annotation] = u.DefaultAutoInstNodeJS
+					}
 				case constants.AnnotationDefaultAutoInstrumentationJava:
 					if inst.Spec.Java.Image == autoInst {
 						upgraded.Spec.Java.Image = u.DefaultAutoInstJava
 						upgraded.Annotations[annotation] = u.DefaultAutoInstJava
 					}
 				}
+
 			} else {
 				u.Logger.V(4).Info("autoinstrumentation not enabled for this language", "flag", instCfg.id)
 			}
 		}
 	}
 
-	for annotation, gate := range defaultAnnotationToGate {
-		autoInst := upgraded.Annotations[annotation]
-		if autoInst != "" {
-			if gate.IsEnabled() {
-				switch annotation {
-				case constants.AnnotationDefaultAutoInstrumentationNodeJS:
-					if inst.Spec.NodeJS.Image == autoInst {
-						upgraded.Spec.NodeJS.Image = u.DefaultAutoInstNodeJS
-						upgraded.Annotations[annotation] = u.DefaultAutoInstNodeJS
-					}
-				case constants.AnnotationDefaultAutoInstrumentationPython:
-					if inst.Spec.Python.Image == autoInst {
-						upgraded.Spec.Python.Image = u.DefaultAutoInstPython
-						upgraded.Annotations[annotation] = u.DefaultAutoInstPython
-					}
-				case constants.AnnotationDefaultAutoInstrumentationGo:
-					if inst.Spec.Go.Image == autoInst {
-						upgraded.Spec.Go.Image = u.DefaultAutoInstGo
-						upgraded.Annotations[annotation] = u.DefaultAutoInstGo
-					}
-				}
-			} else {
-				u.Logger.V(4).Info("autoinstrumentation not enabled for this language", "flag", gate.ID())
-			}
-		}
-	}
 	return upgraded
 }
diff --git a/pkg/instrumentation/upgrade/upgrade_test.go b/pkg/instrumentation/upgrade/upgrade_test.go
@@ -69,6 +69,7 @@ func TestUpgrade(t *testing.T) {
 			config.WithEnableGoInstrumentation(true),
 			config.WithEnableNginxInstrumentation(true),
 			config.WithEnablePythonInstrumentation(true),
+			config.WithEnableNodeJSInstrumentation(true),
 			config.WithEnableJavaInstrumentation(true),
 		),
 	).Default(context.Background(), inst)
@@ -96,6 +97,7 @@ func TestUpgrade(t *testing.T) {
 		config.WithEnableGoInstrumentation(true),
 		config.WithEnableNginxInstrumentation(true),
 		config.WithEnablePythonInstrumentation(true),
+		config.WithEnableNodeJSInstrumentation(true),
 		config.WithEnableJavaInstrumentation(true),
 	)
 	up := NewInstrumentationUpgrade(k8sClient, ctrl.Log.WithName("instrumentation-upgrade"), &record.FakeRecorder{}, cfg)

diff --git a/tests/e2e-upgrade/upgrade-test/opentelemetry-operator-v0.86.0.yaml b/tests/e2e-upgrade/upgrade-test/opentelemetry-operator-v0.86.0.yaml
@@ -8317,7 +8317,6 @@ spec:
             - --enable-leader-election
             - --zap-log-level=info
             - --zap-time-encoding=rfc3339nano
-            - --feature-gates=+operator.autoinstrumentation.go
           image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.86.0
           livenessProbe:
             httpGet: