Allow OTel metrics to work with postgres versions greater than 17. If…

internal/controller/postgrescluster/pgmonitor.go

@@ -43,6 +43,7 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 	monitoringSecret *corev1.Secret) error {
 
 	var (
+		err              error
 		writableInstance *Instance
 		writablePod      *corev1.Pod
 		setup            string
@@ -64,23 +65,11 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 	// that function against an updated and running pod.
 
 	if pgmonitor.ExporterEnabled(ctx, cluster) || collector.OpenTelemetryMetricsEnabled(ctx, cluster) {
-		sql, err := os.ReadFile(fmt.Sprintf("%s/pg%d/setup.sql", pgmonitor.GetQueriesConfigDir(ctx), cluster.Spec.PostgresVersion))
+		setup, err = r.reconcileExporterSqlSetup(ctx, cluster)
 		if err != nil {
 			return err
 		}
 
-		if collector.OpenTelemetryMetricsEnabled(ctx, cluster) {
-			setup = metricsSetupForOTelCollector
-		} else {
-			// TODO: Revisit how pgbackrest_info.sh is used with pgMonitor.
-			// pgMonitor queries expect a path to a script that runs pgBackRest
-			// info and provides json output. In the queries yaml for pgBackRest
-			// the default path is `/usr/bin/pgbackrest-info.sh`. We update
-			// the path to point to the script in our database image.
-			setup = strings.ReplaceAll(string(sql), "/usr/bin/pgbackrest-info.sh",
-				"/opt/crunchy/bin/postgres/pgbackrest_info.sh")
-		}
-
 		for _, containerStatus := range writablePod.Status.ContainerStatuses {
 			if containerStatus.Name == naming.ContainerDatabase {
 				pgImageSHA = containerStatus.ImageID
@@ -145,6 +134,47 @@ func (r *Reconciler) reconcilePGMonitorExporter(ctx context.Context,
 	return err
 }
 
+// reconcileExporterSqlSetup generates the setup.sql string based on
+// whether the OTel metrics feature is enabled or not and the postgres
+// version being used. This function assumes that at least one of
+// OTel metrics or postgres_exporter are enabled.
+func (r *Reconciler) reconcileExporterSqlSetup(ctx context.Context,
+	cluster *v1beta1.PostgresCluster) (string, error) {
+
+	// If OTel Metrics is enabled we always want to use it. Otherwise,
+	// we can assume that postgres_exporter is enabled and we should
+	// use that
+	if collector.OpenTelemetryMetricsEnabled(ctx, cluster) {
+		return metricsSetupForOTelCollector, nil
+	}
+
+	// pgMonitor will not be adding support for postgres_exporter for postgres
+	// versions past 17. If using postgres 18 or later with the postgres_exporter,
+	// create a warning event and set the sql setup to an empty string
+	pgVersion := cluster.Spec.PostgresVersion
+	if pgVersion > 17 {
+		r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "ExporterNotSupportedForPostgresVersion",
+			"postgres_exporter not supported for pg%d; use OTel for postgres 18 and later",
+			pgVersion)
+		return "", nil
+	}
+
+	// OTel Metrics is not enabled and postgres is version 17 or less,
+	// go ahead and read the appropriate sql file, format the string,
+	// and return it
+	sql, err := os.ReadFile(fmt.Sprintf("%s/pg%d/setup.sql", pgmonitor.GetQueriesConfigDir(ctx), pgVersion))
+	if err != nil {
+		return "", err
+	}
+	// TODO: Revisit how pgbackrest_info.sh is used with pgMonitor.
+	// pgMonitor queries expect a path to a script that runs pgBackRest
+	// info and provides json output. In the queries yaml for pgBackRest
+	// the default path is `/usr/bin/pgbackrest-info.sh`. We update
+	// the path to point to the script in our database image.
+	return strings.ReplaceAll(string(sql), "/usr/bin/pgbackrest-info.sh",
+		"/opt/crunchy/bin/postgres/pgbackrest_info.sh"), nil
+}
+
 // reconcileMonitoringSecret reconciles the secret containing authentication
 // for monitoring tools
 func (r *Reconciler) reconcileMonitoringSecret(

internal/controller/postgrescluster/pgmonitor_test.go

@@ -20,10 +20,12 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/feature"
 	"github.com/crunchydata/postgres-operator/internal/initialize"
 	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
+	"github.com/crunchydata/postgres-operator/internal/testing/events"
 	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -551,8 +553,7 @@ func TestReconcilePGMonitorExporter(t *testing.T) {
 		observed := &observedInstances{forCluster: instances}
 
 		called = false
-		assert.NilError(t, reconciler.reconcilePGMonitorExporter(ctx,
-			cluster, observed, nil))
+		assert.NilError(t, reconciler.reconcilePGMonitorExporter(ctx, cluster, observed, nil))
 		assert.Assert(t, called, "PodExec was not called.")
 		assert.Assert(t, cluster.Status.Monitoring.ExporterConfiguration != "", "ExporterConfiguration was empty.")
 	})
@@ -836,6 +837,147 @@ func TestReconcileExporterQueriesConfig(t *testing.T) {
 			actual, err = reconciler.reconcileExporterQueriesConfig(ctx, cluster)
 			assert.NilError(t, err)
 			assert.Assert(t, actual.Data["defaultQueries.yml"] == existing.Data["defaultQueries.yml"], "Data does not align.")
+			assert.Assert(t, actual.Data["defaultQueries.yml"] != "", "Data should not be empty.")
+		})
+
+		t.Run("Pg>17", func(t *testing.T) {
+			cluster.Spec.PostgresVersion = 18
+			actual, err = reconciler.reconcileExporterQueriesConfig(ctx, cluster)
+			assert.NilError(t, err)
+			assert.Assert(t, actual.Data["defaultQueries.yml"] == "", "Data should be empty")
 		})
 	})
 }
+
+// TestReconcileExporterSqlSetup checks that the setup script returned
+// by reconcileExporterSqlSetup is either empty or not depending on
+// which exporter is enabled and what the postgres version is.
+func TestReconcileExporterSqlSetup(t *testing.T) {
+	ctx := context.Background()
+
+	monitoringSpec := &v1beta1.MonitoringSpec{
+		PGMonitor: &v1beta1.PGMonitorSpec{
+			Exporter: &v1beta1.ExporterSpec{
+				Image: "image",
+			},
+		},
+	}
+
+	instrumentationSpec := &v1beta1.InstrumentationSpec{
+		Image: "image",
+	}
+
+	testCases := []struct {
+		tcName             string
+		postgresVersion    int32
+		exporterEnabled    bool
+		otelMetricsEnabled bool
+		errorPresent       bool
+		setupEmpty         bool
+		expectedNumEvents  int
+		expectedEvent      string
+	}{{
+		tcName:             "ExporterEnabledOtelDisabled",
+		postgresVersion:    17,
+		exporterEnabled:    true,
+		otelMetricsEnabled: false,
+		errorPresent:       false,
+		setupEmpty:         false,
+		expectedNumEvents:  0,
+		expectedEvent:      "",
+	}, {
+		tcName:             "ExporterDisabledOtelEnabled",
+		postgresVersion:    17,
+		exporterEnabled:    false,
+		otelMetricsEnabled: true,
+		errorPresent:       false,
+		setupEmpty:         false,
+		expectedNumEvents:  0,
+		expectedEvent:      "",
+	}, {
+		tcName:             "BothEnabled",
+		postgresVersion:    17,
+		exporterEnabled:    true,
+		otelMetricsEnabled: true,
+		errorPresent:       false,
+		setupEmpty:         false,
+		expectedNumEvents:  0,
+		expectedEvent:      "",
+	}, {
+		tcName:             "ExporterEnabledOtelDisabledPostgres18",
+		postgresVersion:    18,
+		exporterEnabled:    true,
+		otelMetricsEnabled: false,
+		errorPresent:       false,
+		setupEmpty:         true,
+		expectedNumEvents:  1,
+		expectedEvent:      "postgres_exporter not supported for pg18; use OTel for postgres 18 and later",
+	}, {
+		tcName:             "ExporterDisabledOtelEnabledPostgres18",
+		postgresVersion:    18,
+		exporterEnabled:    false,
+		otelMetricsEnabled: true,
+		errorPresent:       false,
+		setupEmpty:         false,
+		expectedNumEvents:  0,
+		expectedEvent:      "",
+	}, {
+		tcName:             "BothEnabledPostgres18",
+		postgresVersion:    18,
+		exporterEnabled:    true,
+		otelMetricsEnabled: true,
+		errorPresent:       false,
+		setupEmpty:         false,
+		expectedNumEvents:  0,
+		expectedEvent:      "",
+	}, {
+		tcName:             "ExporterEnabledOtelDisabledBadPostgresVersion",
+		postgresVersion:    1,
+		exporterEnabled:    true,
+		otelMetricsEnabled: false,
+		errorPresent:       true,
+		setupEmpty:         true,
+		expectedNumEvents:  0,
+		expectedEvent:      "",
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.tcName, func(t *testing.T) {
+			cluster := testCluster()
+			cluster.Spec.PostgresVersion = tc.postgresVersion
+
+			recorder := events.NewRecorder(t, runtime.Scheme)
+			r := &Reconciler{Recorder: recorder}
+
+			gate := feature.NewGate()
+			assert.NilError(t, gate.SetFromMap(map[string]bool{
+				feature.OpenTelemetryMetrics: tc.otelMetricsEnabled,
+			}))
+			ctx := feature.NewContext(ctx, gate)
+
+			if tc.otelMetricsEnabled {
+				cluster.Spec.Instrumentation = instrumentationSpec
+			}
+
+			if tc.exporterEnabled {
+				cluster.Spec.Monitoring = monitoringSpec
+			}
+
+			setup, err := r.reconcileExporterSqlSetup(ctx, cluster)
+			if tc.errorPresent {
+				assert.Assert(t, err != nil)
+			} else {
+				assert.NilError(t, err)
+			}
+			assert.Equal(t, setup == "", tc.setupEmpty)
+
+			assert.Equal(t, len(recorder.Events), tc.expectedNumEvents)
+			if tc.expectedNumEvents == 1 {
+				assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
+				assert.Equal(t, recorder.Events[0].Reason, "ExporterNotSupportedForPostgresVersion")
+				assert.Equal(t, recorder.Events[0].Note, tc.expectedEvent)
+				assert.Equal(t, recorder.Events[0].Type, corev1.EventTypeWarning)
+			}
+		})
+	}
+}

internal/pgmonitor/exporter.go

@@ -66,6 +66,12 @@ func GenerateDefaultExporterQueries(ctx context.Context, cluster *v1beta1.Postgr
 		queries += string(queriesContents) + "\n"
 	}
 
+	// pgMonitor will not be adding support for postgres_exporter for postgres
+	// versions past 17. If pg version is greater than 17, return an empty string.
+	if cluster.Spec.PostgresVersion > 17 {
+		return ""
+	}
+
 	// Add general queries for specific postgres version
 	queriesGeneral, err := os.ReadFile(fmt.Sprintf("%s/pg%d/queries_general.yml", queriesConfigDir, cluster.Spec.PostgresVersion))
 	if err != nil {

internal/pgmonitor/exporter_test.go

@@ -38,6 +38,12 @@ func TestGenerateDefaultExporterQueries(t *testing.T) {
 		assert.Assert(t, strings.Contains(queries, "ccp_pg_stat_statements_reset"),
 			"Queries do not contain 'ccp_pg_stat_statements_reset' query when they should.")
 	})
+
+	t.Run("PG>17", func(t *testing.T) {
+		cluster.Spec.PostgresVersion = 18
+		queries := GenerateDefaultExporterQueries(ctx, cluster)
+		assert.Equal(t, queries, "")
+	})
 }
 
 func TestExporterStartCommand(t *testing.T) {