Add util func for adding collector logic (#4128)

Check feature gates and check spec

In our original execution, we had a mix of logic to enable OTel: some logic required
just the feature gate, some logic required the feature gates AND the instrumentation
spec. This PR regularizes the logic: every check require both gates and spec to indicate
the user wants instrumentation; specific checks for logs/metrics within larger checks 
can be left as is.

Note: This PR also removes the instrumentation check from ExporterEnabled. We may 
want to re-add logic like that and be clear about which takes precedence.

Author: benjaminjb

internal/collector/instance.go

@@ -50,9 +50,7 @@ func AddToPod(
 	includeLogrotate bool,
 	thisPodServesMetrics bool,
 ) {
-	if spec == nil ||
-		!(feature.Enabled(ctx, feature.OpenTelemetryLogs) ||
-			feature.Enabled(ctx, feature.OpenTelemetryMetrics)) {
+	if !OpenTelemetryLogsOrMetricsEnabled(ctx, spec) {
 		return
 	}
 

internal/collector/naming.go

@@ -15,6 +15,7 @@ const PGBouncerMetrics = "metrics/pgbouncer"
 const PostgresMetrics = "metrics/postgres"
 const PatroniMetrics = "metrics/patroni"
 const ResourceDetectionProcessor = "resourcedetection"
+const MonitoringUser = "ccp_monitoring"
 
 const SqlQuery = "sqlquery"
 

internal/collector/patroni.go

@@ -9,7 +9,6 @@ import (
 	"slices"
 	"strconv"
 
-	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -23,7 +22,7 @@ func EnablePatroniLogging(ctx context.Context,
 		spec = inCluster.Spec.Instrumentation.Logs
 	}
 
-	if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
+	if OpenTelemetryLogsEnabled(ctx, inCluster) {
 		directory := naming.PatroniPGDataLogPath
 
 		// Keep track of what log records and files have been processed.
@@ -134,7 +133,7 @@ func EnablePatroniMetrics(ctx context.Context,
 	inCluster *v1beta1.PostgresCluster,
 	outConfig *Config,
 ) {
-	if feature.Enabled(ctx, feature.OpenTelemetryMetrics) {
+	if OpenTelemetryMetricsEnabled(ctx, inCluster) {
 		// Add Prometheus exporter
 		outConfig.Exporters[Prometheus] = map[string]any{
 			"endpoint": "0.0.0.0:" + strconv.Itoa(PrometheusPort),

internal/collector/patroni_test.go

@@ -11,6 +11,7 @@ import (
 	"gotest.tools/v3/assert"
 
 	"github.com/crunchydata/postgres-operator/internal/feature"
+	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -23,8 +24,14 @@ func TestEnablePatroniLogging(t *testing.T) {
 		ctx := feature.NewContext(context.Background(), gate)
 
 		config := NewConfig(nil)
+		cluster := new(v1beta1.PostgresCluster)
+		require.UnmarshalInto(t, &cluster.Spec, `{
+			instrumentation: {
+				logs: { retentionPeriod: 5h },
+			},
+		}`)
 
-		EnablePatroniLogging(ctx, new(v1beta1.PostgresCluster), config)
+		EnablePatroniLogging(ctx, cluster, config)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)

internal/collector/pgadmin.go

@@ -10,17 +10,17 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 
-	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
 func EnablePgAdminLogging(ctx context.Context, spec *v1beta1.InstrumentationSpec,
 	configmap *corev1.ConfigMap,
 ) error {
-	if !feature.Enabled(ctx, feature.OpenTelemetryLogs) {
+	if !OpenTelemetryLogsEnabled(ctx, spec) {
 		return nil
 	}
+
 	otelConfig := NewConfig(spec)
 
 	otelConfig.Extensions["file_storage/pgadmin_data_logs"] = map[string]any{
@@ -125,5 +125,6 @@ func EnablePgAdminLogging(ctx context.Context, spec *v1beta1.InstrumentationSpec
 	if err == nil {
 		configmap.Data["collector.yaml"] = otelYAML
 	}
+
 	return err
 }

internal/collector/pgadmin_test.go

@@ -12,7 +12,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 
 	"github.com/crunchydata/postgres-operator/internal/collector"
-	pgadmin "github.com/crunchydata/postgres-operator/internal/controller/standalone_pgadmin"
 	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
@@ -31,7 +30,11 @@ func TestEnablePgAdminLogging(t *testing.T) {
 
 		configmap := new(corev1.ConfigMap)
 		initialize.Map(&configmap.Data)
-		err := collector.EnablePgAdminLogging(ctx, nil, configmap)
+		var instrumentation *v1beta1.InstrumentationSpec
+		require.UnmarshalInto(t, &instrumentation, `{
+			logs: { retentionPeriod: 12h },
+		}`)
+		err := collector.EnablePgAdminLogging(ctx, instrumentation, configmap)
 		assert.NilError(t, err)
 
 		assert.Assert(t, cmp.MarshalMatches(configmap.Data, `
@@ -44,7 +47,7 @@ collector.yaml: |
   extensions:
     file_storage/pgadmin_data_logs:
       create_directory: false
-      directory: `+pgadmin.LogDirectoryAbsolutePath+`/receiver
+      directory: /var/lib/pgadmin/logs/receiver
       fsync: true
   processors:
     batch/1s:
@@ -90,11 +93,11 @@ collector.yaml: |
   receivers:
     filelog/gunicorn:
       include:
-      - `+pgadmin.GunicornLogFileAbsolutePath+`
+      - /var/lib/pgadmin/logs/gunicorn.log
       storage: file_storage/pgadmin_data_logs
     filelog/pgadmin:
       include:
-      - `+pgadmin.LogFileAbsolutePath+`
+      - /var/lib/pgadmin/logs/pgadmin.log
       storage: file_storage/pgadmin_data_logs
   service:
     extensions:
@@ -165,7 +168,7 @@ collector.yaml: |
   extensions:
     file_storage/pgadmin_data_logs:
       create_directory: false
-      directory: `+pgadmin.LogDirectoryAbsolutePath+`/receiver
+      directory: /var/lib/pgadmin/logs/receiver
       fsync: true
   processors:
     batch/1s:
@@ -211,11 +214,11 @@ collector.yaml: |
   receivers:
     filelog/gunicorn:
       include:
-      - `+pgadmin.GunicornLogFileAbsolutePath+`
+      - /var/lib/pgadmin/logs/gunicorn.log
       storage: file_storage/pgadmin_data_logs
     filelog/pgadmin:
       include:
-      - `+pgadmin.LogFileAbsolutePath+`
+      - /var/lib/pgadmin/logs/pgadmin.log
       storage: file_storage/pgadmin_data_logs
   service:
     extensions:

internal/collector/pgbackrest.go

@@ -11,7 +11,6 @@ import (
 	"fmt"
 	"slices"
 
-	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -29,7 +28,7 @@ func NewConfigForPgBackrestRepoHostPod(
 ) *Config {
 	config := NewConfig(spec)
 
-	if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
+	if OpenTelemetryLogsEnabled(ctx, spec) {
 
 		var directory string
 		for _, repo := range repos {

internal/collector/pgbackrest_test.go

@@ -11,6 +11,7 @@ import (
 	"gotest.tools/v3/assert"
 
 	"github.com/crunchydata/postgres-operator/internal/feature"
+	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -27,8 +28,12 @@ func TestNewConfigForPgBackrestRepoHostPod(t *testing.T) {
 				Volume: new(v1beta1.RepoPVC),
 			},
 		}
+		var instrumentation *v1beta1.InstrumentationSpec
+		require.UnmarshalInto(t, &instrumentation, `{
+			logs: { retentionPeriod: 12h },
+		}`)
 
-		config := NewConfigForPgBackrestRepoHostPod(ctx, nil, repos)
+		config := NewConfigForPgBackrestRepoHostPod(ctx, instrumentation, repos)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)

internal/collector/pgbouncer.go

@@ -12,7 +12,6 @@ import (
 	"slices"
 	"strconv"
 
-	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -40,7 +39,7 @@ func NewConfigForPgBouncerPod(
 	config := NewConfig(cluster.Spec.Instrumentation)
 
 	EnablePgBouncerLogging(ctx, cluster, config)
-	EnablePgBouncerMetrics(ctx, config, sqlQueryUsername)
+	EnablePgBouncerMetrics(ctx, cluster, config, sqlQueryUsername)
 
 	return config
 }
@@ -56,7 +55,7 @@ func EnablePgBouncerLogging(ctx context.Context,
 		spec = inCluster.Spec.Instrumentation.Logs
 	}
 
-	if feature.Enabled(ctx, feature.OpenTelemetryLogs) {
+	if OpenTelemetryLogsEnabled(ctx, inCluster) {
 		directory := naming.PGBouncerLogPath
 
 		// Keep track of what log records and files have been processed.
@@ -171,8 +170,10 @@ func EnablePgBouncerLogging(ctx context.Context,
 
 // EnablePgBouncerMetrics adds necessary configuration to the collector config to scrape
 // metrics from pgBouncer when the OpenTelemetryMetrics feature flag is enabled.
-func EnablePgBouncerMetrics(ctx context.Context, config *Config, sqlQueryUsername string) {
-	if feature.Enabled(ctx, feature.OpenTelemetryMetrics) {
+func EnablePgBouncerMetrics(ctx context.Context, inCluster *v1beta1.PostgresCluster,
+	config *Config, sqlQueryUsername string) {
+
+	if OpenTelemetryMetricsEnabled(ctx, inCluster) {
 		// Add Prometheus exporter
 		config.Exporters[Prometheus] = map[string]any{
 			"endpoint": "0.0.0.0:" + strconv.Itoa(PrometheusPort),

internal/collector/pgbouncer_test.go

@@ -11,6 +11,7 @@ import (
 	"gotest.tools/v3/assert"
 
 	"github.com/crunchydata/postgres-operator/internal/feature"
+	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -23,8 +24,13 @@ func TestEnablePgBouncerLogging(t *testing.T) {
 		ctx := feature.NewContext(context.Background(), gate)
 
 		config := NewConfig(nil)
-
-		EnablePgBouncerLogging(ctx, new(v1beta1.PostgresCluster), config)
+		cluster := new(v1beta1.PostgresCluster)
+		require.UnmarshalInto(t, &cluster.Spec, `{
+			instrumentation: {
+				logs: { retentionPeriod: 5h },
+			},
+		}`)
+		EnablePgBouncerLogging(ctx, cluster, config)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)