Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Metrics Push Gateway Options #678

Merged
merged 6 commits into from
Jan 29, 2019
Merged

Metrics Push Gateway Options #678

merged 6 commits into from
Jan 29, 2019

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Jan 28, 2019

PR description

Don't overload --metrics-host and --metrics-port for push mode,
instead use --metrics-push-host and --metrics-push-port. The former
is inbound, the latter is outbound.

dont' overload `--metrics-host` and `--metrics-port` for push mode,
instead use `--metrics-push-host` and `--metrics-push-port`.  The former
is inbound, the latter is outbound.
allow problematic shutdown.
@@ -674,6 +692,8 @@ MetricsConfiguration metricsConfiguration() {
metricsConfiguration.setMode(metricsMode);
metricsConfiguration.setHost(metricsHost.toString());
metricsConfiguration.setPort(metricsPort);
metricsConfiguration.setPushHost(metricsPushHost.toString());
metricsConfiguration.setPushPort(metricsPushPort);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to throw an error if the user configures a set of host / port values that don't correspond to the chosen mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

picoCLI doesn't let us see what CLI flags there are, just the resulting values and a default is indistinguishable from a flag with a default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, no. Not with our current CLI/TOML library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will be possible using what's in #679

Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

LGTM but will probably require a rework after merge with #679

shemnon and others added 3 commits January 28, 2019 15:58
CLI change - go to --metrics-push-* instead and get reid of --metrics-mode.
add error if both metrics and metrics-push are enabled.
@NicolasMassart
Copy link
Contributor

NicolasMassart commented Jan 28, 2019

I can't tell if using push enabled instead of a mode option is better for users. Otherwise the code looks correct given this initial choice.

@shemnon shemnon merged commit 3c32f27 into PegaSysEng:master Jan 29, 2019
@shemnon
Copy link
Contributor Author

shemnon commented Jan 29, 2019

Adding new rules to #679

vinistevam pushed a commit to vinistevam/pantheon that referenced this pull request Jan 29, 2019
* Don't overload `--metrics-host` and `--metrics-port` for push mode, instead add '--metrics-push-enabled' , `--metrics-push-host` and `--metrics-push-port`, rename `--metrics-prometheus-job` to `--metrics-push-prometheus-job` and remove `--metrics-mode` 
* Show an error if both metrics and metrics-push are enabled.
* Allow shutdown if we cannot communicate with the Push Gateway.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants