-
Notifications
You must be signed in to change notification settings - Fork 118
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
base: master
Are you sure you want to change the base?
[prometheus-reporter] Add ability to infer listen port based on env var #86
Conversation
|
|
||
const ( | ||
// ConfigResolver resolves port using a value provided in config | ||
ConfigResolver ListenAddressResolver = "config" |
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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:
to
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. |
Ported from m3db/m3x#186