tally icon indicating copy to clipboard operation
tally copied to clipboard

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

Open prateek opened this issue 7 years ago • 3 comments

Ported from https://github.com/m3db/m3x/pull/186

prateek avatar Aug 15 '18 23:08 prateek

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.

CLAassistant avatar Aug 15 '18 23:08 CLAassistant

Coverage Status

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

coveralls avatar Aug 15 '18 23:08 coveralls

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.

jeromefroe avatar Aug 17 '18 12:08 jeromefroe