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

[prometheus-reporter] Add ability to infer listen port based on env var #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prateek
Copy link
Collaborator

@prateek prateek commented Aug 15, 2018

Ported from m3db/m3x#186

@CLAassistant
Copy link

CLAassistant commented Aug 15, 2018

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.7%) to 92.613% when pulling e54f421 on prateek:prateek/tally-prometheus-listen-address into d0a004a on uber-go:master.


const (
// ConfigResolver resolves port using a value provided in config
ConfigResolver ListenAddressResolver = "config"

Choose a reason for hiding this comment

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

Would it be more clear to name this PortType with static and dynamic being the two values? Static ports should always be loaded from config, and dynamic ports should always be resolved from env vars.

// DynamicListenAddress if specified will be used instead of just registering the
// handler on the default HTTP serve mux without listening.
// Note: if DynamicListenAddress is specified, ListenAddress is ignored.
DynamicListenAddress *ListenAddressConfiguration `yaml:"dynamicListenAddress"`

Choose a reason for hiding this comment

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

Hm this is kinda confusing, e.g., you can have a dynamicListenAddress that defines its port in the config file.

@@ -143,3 +153,22 @@ func (c Configuration) NewReporter(

return reporter, nil
}

func (c Configuration) resolveListenAddress() (addr string, resolved bool, err error) {

Choose a reason for hiding this comment

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

Technically you don't really need resolved since addr == "" && !resolved is always false.

// Resolve returns the resolved listen address given the configuration.
func (c ListenAddressConfiguration) Resolve() (string, error) {
if c.Port == nil {
return "", errors.New("missing port config")

Choose a reason for hiding this comment

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

nit: make the err errMissingPort.

Hostname string `yaml:"hostname" validate:"nonzero"`

// port specifies the port to listen on
Port *PortConfiguration `yaml:"port" validate:"nonzero"`

Choose a reason for hiding this comment

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

Hm if this is always nonzero, why not make it a non-pointer?

string(p.PortType))
}

return fmt.Sprintf("%s:%d", c.Hostname, port), nil

Choose a reason for hiding this comment

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

nit: net.JoinHostPort might be more portable.

@jeromefroe
Copy link
Contributor

I thought about this some more, instead of adding another configuration option, how would you feel if we modify the prometheus configuration in the service itself? For example, we could change:

server.Run(server.RunOptions{
		ConfigFile: *configFile,
})

to

var cfg config.Configuration
if err := xconfig.LoadFile(&cfg, *configFile, xconfig.Options{}); err != nil {
			fmt.Fprintf(os.Stderr, "unable to load %s: %v", runOpts.ConfigFile, err)
			os.Exit(1)
}

// update config.Configuration.Metrics.PrometheusReporter based on env variable

server.Run(server.RunOptions{
		Config: config,
})

I think this approach would address both of our concerns: we wouldn't need to munge the config files at runtime and we wouldn't need to include an additional configuration option in tally.

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.

None yet

5 participants