Restarts Prometheus subprocess when it crashes #663
Conversation
| prometheusConfigFilename: string, prometheusEndpoint: string, prometheusTsdbFilename: string, | ||
| prometheusLocation: string, configJson: {}, verbose: boolean) { | ||
| await writePrometheusConfigToDisk(prometheusConfigFilename, configJson); | ||
| const processArgs = [ |
There was a problem hiding this comment.
This library is intended to be a generic Prometheus client, but these are all application-specific parameters. Could you change it back to take a generic list of args?
There was a problem hiding this comment.
Thanks Vini. That's interesting insight!
I usually recommend the opposite and put all the "opinions" into the client. This guarantees a consistent experience for all users of the client. And only allow optional customizations to be overridden. I like to think of it as analogous to a builder pattern with all required params without going full builder.
On a side note, I put the logic back into the caller.
There was a problem hiding this comment.
I agree with you when we talk about the domain objects. But I see everything under infrastructure/ as a different bounded context, independent of our app, with their own models.
| mkdirp.sync(path.dirname(configFilename)); | ||
| export async function startPrometheus( | ||
| configFilename: string, configJson: {}, processArgs: string[], endpoint: string) { | ||
| await writePrometheusConfigToDisk(configFilename, configJson); |
There was a problem hiding this comment.
I wonder if it makes sense to make these two calls in main instead. It feels odd to pass both a config filename and the config contents to a method. The configJson is not really used by spawnPrometheusSubprocess.
Anyway, not important, up to you.
There was a problem hiding this comment.
I don't feel strongly either way about it.
The configFilename is used in the args, so there's kind of a duplication going on.
I would personally keep it as one method (even if the signature isn't that great) so that users don't forget to call writeConfigFile before spawnPromProcess.
Adds restart logic for the Prometheus subprocess by listening for process exit events.
Manually tested using the following:
$ yarn do shadowbox/docker/run
$ pgrep prometheus | xargs sudo kill
TODO: Can we write an integration test?
TODO: Can we publish an event somewhere to track the number of crashes we see?