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

feat: push prometheus metrics to pushgateway on-demand #1404

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ekleog
Copy link

@Ekleog Ekleog commented Jan 20, 2025

Fixes #1403

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.. From my side this is almost ready for merging, I just left a view comments.

General question: What do you think about which env variables are used? The "standard" is to use RUSTIC_XXX env variable for a xxx option. We have the pushgateway_url option and we have (only) env options for PUSHGATEWAY_USER and PUSHGATEWAY_PASS - I personally would have the later 2 also in the config and then we should decide about the env names. Or use RUSTIC_PUSHGATEWAY_* (by clap) if present and the PUSHGATEWAY_* as alternatives if they are commonly used variables for the pushgateway?

src/commands/backup.rs Outdated Show resolved Hide resolved
src/prometheus.rs Outdated Show resolved Hide resolved
src/prometheus.rs Outdated Show resolved Hide resolved
@Ekleog
Copy link
Author

Ekleog commented Jan 27, 2025

Thank you for your review!

About the authentication environment variables, I actually thought about it. My argument against having CLI flags with user/pass is, it's essentially insecure to use this way, because any process on your machine can then read the CLI flags, and thus bypass auth.

Especially when Pushgateway is likely often deployed on the same host it's keeping metrics for (and firewalled out of the internet), this seems to me like having its credentials in the CLI would basically completely kill any security the credentials would bring.

I just renamed them RUSTIC_* for consistency with clap.

Also, I would not at all be opposed to having these as a parameter in the config file, but TBH I have absolutely no idea how to actually do it without also exposing a clap argument — I haven't dived deep enough in rustic's config handling yet.

WDYT?

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.

JSON output as prometheus metric format?
2 participants