prometheus-community / postgres_exporter

A PostgreSQL metric exporter for Prometheus
Apache License 2.0
2.66k stars 710 forks source link

Only run certain queries on some databases #443

Open pvanderlinden opened 3 years ago

pvanderlinden commented 3 years ago

I was wondering if it is possible to run some queries on some of the databases? The postgres exporter auto discovers the databases, and runs all queries on all databases. Which currently logs errors in both the exporter and postgres logs. How can I restrict certain queries to only some/one of the databases (for example only run pg_stat_statements on the "main" database)? Or is that a new feature?

uspen commented 3 years ago

master: true

pvanderlinden commented 3 years ago

@uspen that only says it should only run on the master database server as far as I understand. Otherwise I don't understand how to specify what the main DB is (and would be severely limiting)

pvanderlinden commented 3 years ago

Is this project dead @wrouesnel ?

wrouesnel commented 3 years ago

I haven't had a lot of time to work on it over the last year, but more significantly I simply don't work with monitoring postgres databases all that much professionally anymore. Pull requests are merged as I can get around to it (like now).

adriendrien commented 3 years ago

Hi,

Is there any known workaround so we can avoid logging errors ?

itshikanov commented 3 years ago

any ideas?

pvanderlinden commented 3 years ago

I have looked at the code, the main challenge I see is that there is no database name available when looping through servers/queries: https://github.com/prometheus-community/postgres_exporter/blob/cbc6ae3c595db51a2bbec91448226c00b37fa844/cmd/postgres_exporter/postgres_exporter.go#L1460

SuperQ commented 3 years ago

Prometheus exporters are supposed to be designed to be 1:1 with their targets. The other option is to use the multi-target exporter pattern.

The current multi-database server feature is actually a mistake and needs to be removed. As this bug has shown, it's one of those unintended consequences of trying to do things that don't follow best practices.

pvanderlinden commented 3 years ago

This is actually not about multiple servers, but about scraping multiple databases within in single server (or would that be seen as a separate target as well)? How would you approach this @SuperQ , because a lot of the metrics exported within this exporter will be server wide metrics, others are database specific metrics (but most will export the metrics for all databases within that server)?

SuperQ commented 3 years ago

Oh, sorry, I was confused by name of the feature.

Is this about filters for which databases are included in table stats?

pvanderlinden commented 3 years ago

It is mainly about filtering which user queries will run on which databases. There are also a few server wide tables with database stats, which this is not about. The main scenario is to run certain queries on certain database(s) to prevent a constant error stream

pvanderlinden commented 3 years ago

For example pg_stat_statements is an extension, which only needs to be activated on one database to get the info you need.

SuperQ commented 3 years ago

I see part of this problem is that the user-provided query system allows the exporter to be abused as a general use SQL exporter. IMO we should probably eliminate this feature, and integrate all the useful postgresql server metrics queries into the exporter as collectors . This is how we avoid this class of problems in the mysqld_exporter.

pvanderlinden commented 3 years ago

How would you handle this with extensions? Postgresql has a ton of extensions, which usually provide their own statistics. The statement statistics table is one of them (not installed/activated by default)

SuperQ commented 3 years ago

In the mysqld_exporter, we have a variety of collector modules, each setup to gather data for various plugins/extensions/features. Some we enable by default, others like the MySQL equivalent of pg_stat_statements we leave off by default for cardinality. But it's a simple flag-flip with some tunables to enable it.

This style of things makes it a lot easier for people less familiar with the database internals to use. We can program in Prometheus-carinality-safe defaults.

pvanderlinden commented 3 years ago

That makes sense. Maybe this ticket should be about a collector instead in that case. Unfortunately I don't have enough experience with golang to make such a big change

keithf4 commented 3 years ago

I see part of this problem is that the user-provided query system allows the exporter to be abused as a general use SQL exporter. IMO we should probably eliminate this feature, and integrate all the useful postgresql server metrics queries into the exporter as collectors . This is how we avoid this class of problems in the mysqld_exporter.

Please do not eliminate this feature. This is primarily how we, and I believe many users, have been using this exporter. What you may consider the useful postgresql metrics others may not. We actually completely turn off all the included metrics precisely because we prefer using our own and eliminate collecting many that we do not want to collect.

keithf4 commented 3 years ago

We actually run two postgres_exporters to sort of solve the primary issue brought up here

We do this because if you connect to multiple databases when collecting the "global" metrics, you end up collecting duplicates for the same metric. Doesn't cause series errors because each has a different dbname label, but in a cluster with many databases, this causes A LOT of needless duplication. In order to collect individual database metrics, you must connect to that specific database. And since this is a different set of queries, we have to run a separate exporter for it since you can only provide a single custom query file to the exporter.

We've customized our postgres_exporter service to build a single custom query file defined by an environment variable that the user can set with as many query files as they'd like.

Ideally, it would be nice to be able to just define which database a query should run on with a custom query file. Being able to connect to multiple databases and designate which one a given custom query runs on would seem to solve a lot of these issues.

https://access.crunchydata.com/documentation/pgmonitor/latest/exporter/#setup-on-rhel-or-centos-7

pvanderlinden commented 3 years ago

@keithf4 I think what @SuperQ is saying that you might as well use this exporter: https://github.com/free/sql_exporter (disclaimer: haven't looked at the docs in depth yet). My question however is, what would be the use of this postgresql exporter in that case? Maybe it is more useful to have a maintained collection of yaml files with standard queries for postgresql (common, per version, per extension) instead, which are usable with the sql exporter?

SuperQ commented 3 years ago

@keithf4

Please do not eliminate this feature. This is primarily how we, and I believe many users, have been using this exporter. What you may consider the useful postgresql metrics others may not. We actually completely turn off all the included metrics precisely because we prefer using our own and eliminate collecting many that we do not want to collect.

Funny enough, that's exactly one of the reasons I want to eliminate this feature.

By having bad options and defaults in the exporter, everyone has to roll their own. This makes every install different, which makes it harder for us to share experience with what makes for good PostgreSQL monitoring.

This leads to a feedback loop of no contributions of good monitoring practices to the exporter. This also leads to tribal copy-pasta of the queries.yml.

By having every install be different, we can't reliably provide a Monitoring Mixin to make alerting and dashboards easier to use.

The better scenario would be for you to contribute your best practices back to the exporter as collectors, with flags to adjust the behavior based on individual needs. File issues as to why you disable all the default collectors if they don't work as you need them to.

keithf4 commented 3 years ago

I'd be happy to help contribute to some query standardization here. However, that could just be for the standard ones that are included and turned on by default the way things are now. Yes, if those are good and even more modular than it is now, that would be great and cause fewer people to try and roll their own. But I don't think it will provide the right solution most are looking for and could make maintaining this for the community pretty challenging. Been following the usage and support of this extension for a long time and users custom queries themselves never seemed to be the problem, it was more complaints about what this issue was originally opened for or simply just trying to get the custom queries working in the first place.

I think one thing you'll find about the PG community is that there are so many third party, optional extensions both built in and throughout the community that trying to have all of these collectors maintained upstream would be incredibly challenging both for the maintainers of this extension and for users of it. By removing any sort of user customization feature I can almost guarantee you'd either see a new flood of requests to support everyone's addon tool or a migration away to another tool that allows that. Just for example, in our usage we require the ability to monitor backups from pgBackRest and metrics from pgBouncer. And in the case of pgBouncer (currently one of the most popular poolers available), I don't see how you'd ever be able to support that built in due to the odd way its metrics are queried (this was our solution - https://github.com/CrunchyData/pgbouncer_fdw).

If there'd be someway to allow add custom collector by the user, while maintaining a good standard set, that would be ideal.

And back to the original topic of this post, it would be nice if the exporter can still maintain the ability to connect to multiple databases in a single PG instance from a single exporter. Otherwise it's back to the way it was before 0.5.0 where we had to run a single exporter per-database for a single pg instance, which is not ideal from either a maintenance standpoint or resource usage. An instance with 20-30 user databases would require 20-30 exporters running.

I hadn't seen the sql_exporter before, thanks for sharing! But initially looking at it, it seems to suffer from the same problems as this issue as well... you'd have to run one for each database you want to monitor in a single instance. But it seems this exporter allows custom collections which is really nice. Would that sort of feature be able to be incorporated into postgres_exporter?

SuperQ commented 3 years ago

I welcome the flood of requests to add features. This is the intended outcome.

Of course not everything will fit in the use case. Solving everything with one tool is antithetical to the design of Prometheus. Monitoring the contents of databases is a non-goal of the postgres_exporter. This exporter is only intended to monitor the PostgreSQL server itself. Not any "user" or "business" data, that's what the sql_exporter is for.

Your example of pgBackRest is a good one. If it's something that lives inside the server as a plugin, I see no reason to reject including it.

Another example, there's a separate pgbouncer_exporter.

I've talked about this with some PostgreSQL developers before, but long-term I would love to see this entire tool eliminated.

Having an exporter built in as a PostgreSQL extension, and having the ability for other extensions to self-register is the end game here. The original Prometheus design was intended to be "Agentless". Every piece of software is able to expose a monitoring endpoint directly, with no 3rd party process/agent. For example, if crunch-proxy were a viable pgbouncer replacement, it would be able to expose Prometheus metrics directly.

We did this with HAProxy, it now has built-in metrics, and the exporter is only useful for legacy installs.

pvanderlinden commented 3 years ago

@SuperQ The approach will also mean it will break many installations, and means people will just stick with an older version before it broke basic monitoring. The ideal situation sound good, but don't we need something which works in between?

SuperQ commented 3 years ago

Yes, of course. I'm not planning to just throw the feature out the window without a migration plan for users. It would have to implement it slowly, likely over several releases.

I can write up a design doc issue to plan out what I think we should do. And I'm not going to do things without community feeback or support. But unless I can get my $dayjob to support the development, it will need to be a community effort.

keithf4 commented 3 years ago

Great to hear the plans going forward for this. Look forward to helping where I can.

If anything comes out of this, though, I really do think there needs to be some sort of option to allow custom queries. The mere fact that this is monitoring a database just begs for it. And while I appreciate your enthusiasm to welcome those requests to just build those into core, I just don't think it is a realistic design goal for long term maintenance. Especially something that can be solved with the simple feature of allowing a user to run the queries they want to run. I can guarantee the custom query request will be, by far, the most requested feature you see if were ever removed.

I will say an an exporter that can be installed into postgres as an extension would be fantastic!

SuperQ commented 3 years ago

It's been exactly this way in the mysqld_exporter since the beginning. It has a wide variety of collectors for pretty much every useful monitoring feature in MySQL.

pvanderlinden commented 3 years ago

I had 90% of a solution done when you mentioned you didn't think this was the correct way forward. I finished it of this morning. What do you think of allowing this as a temporary fix while the plan is made to revamp this? My branch: https://github.com/prometheus-community/postgres_exporter/compare/master...pvanderlinden:databases-filter?expand=1

pvanderlinden commented 3 years ago

@SuperQ Have you seen my question ^?

jjoooosseepp commented 2 years ago

Does anyone know what are the plans with this? Can one expect the changes made by @pvanderlinden to be merged or should anyone interested in this just compile their own version with that change?

sebglon commented 1 year ago

Can we have a review on the PR above please?

MichaelDBA commented 1 year ago

This is also very troubling since you cannot exclude databases at the present time with the current version. See #735, #750.

@keithf4, are you able to overcome this problem with excluding databases?