Skip to content

Conversation

@dsessler7
Copy link
Contributor

… postgres_exporter is used with postgres versions greater than 17, create a warning event and set the setup.sql blank.

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature
  • Bug fix
  • Documentation
  • Testing enhancement
  • Other

What is the current behavior (link to any open issues here)?

Currently, if you are using either metrics exporter (OTel or postgres_exporter) with a postgres version greater than 17, you will get a reconciler error and metrics will not work.

What is the new behavior (if this is a feature change)?

  • Breaking change (fix or feature that would cause existing functionality to change)

You can now get metrics with pg > 17 if you use OTel as your exporter. We will not support postgres_exporter with pg versions greater than 17. If you attempt to use postgres_exporter with pg > 17, you will see a warning event and the metrics will essentially be turned off.

Other Information:

Copy link
Collaborator

@jmckulk jmckulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One naming ⛏️but functionally looks good

// OTel metrics or postgres_exporter are enabled.
func (r *Reconciler) reconcileExporterSqlSetup(ctx context.Context,
cluster *v1beta1.PostgresCluster) (string, error) {
if !collector.OpenTelemetryMetricsEnabled(ctx, cluster) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if exporter would make more sense here since we are only talking about exporter specific things (comments, warnings, setup) ⛏️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow... In this function the "Exporter" in reconcileExporterSqlSetup refers to both/either OTel and/or postgres_exporter. If the OTel metrics feature is enabled, it takes precedence (even if postgres_exporter is enabled). If OTel metrics isn't enabled, we get the setup for postgres_exporter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. I should have said if postgres_exporter... something like this:

if pgmonitor.ExporterEnabled(ctx, cluster) {
  # do some postgres_exporter specific things
  return metrics for postgres_exporter
}

# default to otel
return metrics for otel

It feels weird to me to have

if !otel {
  # do postgres_exporter things
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's just the thing, if OTel metrics is enabled it will always be used, even if postgres_exporter is also enabled. Also, pgmonitor.ExporterEnabled checks that at least one of OTel metrics or postgres_exporter are enabled (not only postgres_exporter).

I guess I could change it to more of a guard clause for OTel and make postgres_exporter the "default":

if otel {
  return metricsSetupForOTelCollector, nil
}

# do postgres_exporter stuff

But then we're kinda splitting hairs here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely splitting hairs 👍

… postgres_exporter is used with postgres versions greater than 17, create a warning event and set the setup.sql blank.
@dsessler7 dsessler7 merged commit 50fd5ef into CrunchyData:main Nov 6, 2025
18 checks passed
@dsessler7 dsessler7 deleted the pg18-exporter-fix branch November 6, 2025 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants