Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework of configuration dump #28

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Rework of configuration dump #28

merged 1 commit into from
Jun 2, 2022

Conversation

mjf
Copy link
Contributor

@mjf mjf commented Jun 1, 2022

UTF-8 characters were causing issues on certain systems (daemon could
not start and crashed). This patch completely reworks the formatting of
the configuration dump given by --dry-run. This originates in #11.

Care was taken to ensure that the config dump gives something expectable
in the comments. There was far too things redundant with the actual YAML
configuration in the original format.

Signed-off-by: Matouš Jan Fialka mjf@mjf.cz

UTF-8 characters were causing issues on certain systems (daemon could
not start and crashed). This patch completely reworks the formatting of
the configuration dump given by `--dry-run`. This originates in in #11.

Care was taken to ensure that the config dump gives something expectable
in the comments. There was far too things redundant with the actual YAML
configuration in the original format.

Signed-off-by: Matouš Jan Fialka <mjf@mjf.cz>
@mjf
Copy link
Contributor Author

mjf commented Jun 1, 2022

Hi @Vonng
This is first try. I do not know your preferences, so please let me know if you like it or not. Once we get this right, we can use pg_exporter -c ... --dry-run to re-generate all the files use to generate the pg_exporter.yaml (except the 000-... one, which needs to be somewhat refactored (make it a MD file?)

@Vonng Vonng merged commit 520f2e1 into Vonng:master Jun 2, 2022
@Vonng
Copy link
Owner

Vonng commented Jun 2, 2022

Thanks for your contribution~~

Since's collectors are highly structured config.
I was consider using a postgresql table to store all the collector configs and add some util script to manipulate them.

@mjf
Copy link
Contributor Author

mjf commented Jun 2, 2022

I was consider using a postgresql table to store all the collector configs and add some util script to manipulate them.

Please, don't do that. I think YAML files are good-enough for this and also storing configuration in extra database may lay extra burdens on operators. And, most probably, you would need to maintain extra Postgres cluster per project or per customer (not only because of security concerns, but also because of locality, local law conditions, etc.) and storing configuration data for monitoring a database cluster in that very database cluster makes no sense at all.

If it makes sense in your use cases, make this strictly optional and just extend the --config flag to accept some postgres://... (or the libpq's connection string) and read .pgpass etc. or make it part of some top-level .yaml config or environmental variables or whatever but keep the possibility to maintain the whole structure in YAML files and, the best, in a folder hierarchy. Does this make sense to you? 🙂


I even think it would be best to provide the pg_exporter configs, which are highly valuable (!) by the way, completely apart from the pg_exporter source code tree in separated one (eg. Vonng/pg_exporter_pigsty_configs would be yours and I will create my own in my namespace - either I can start with forking yours, or I will maintain my own elsewhere...) The configs are quite complex, as you stated, and there is literally no one-to-fit-all solution. I like the idea people could easily just discuss, fork etc. the configurations, which is mostly about SQL queries for DBAs and monitoring, rather than forking the whole project. The exporter as such is one thing, the SQL queries it runs are pretty different thing, IMHO, which also depends on user style of writting SQL etc. (one likes joins, other would use CTEs, whatever, for the same results).

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.

2 participants