CLI/flag package choice and standardization

[fabxc]

The proposed amtool (prometheus/alertmanager#636) uses a different CLI/flag package than our other CLI components. This causes friction when using our different tools.

This tool uses cobra/viper, which is the de-facto standard for CLIs in Go nowadays I suppose. It allows nice UX with subcommands and flags specified at those subcommand levels. The flag format is also what one expects in general.
Prometheus, promtool and Alertmanager OTOH are using Go's standard flag package, which is weird at best and limited in functionality. People probably got used to it by now but we probably shouldn't introduce tools with different flag formats etc.

So A) move amtool to stdlib flag (and hack something around for proper subcommands) or B) eventually move Alertmanager, promtool, and Prometheus (2.0+) to cobra.
It would be somewhat disruptive but is probably a saner solution in the long run. For most people it merely means updating their deployment recipes, which they have to anyway for Prometheus 2.0.

@juliusv @brian-brazil @beorn7 @brancz @grobie

[beorn7]

Given that our old "one true way" to create CLIs hasn't spread a lot (besides promtool, there is only storagetool, but that's just a tiny hack at the moment), I think we can only gain by moving the CLIs to cobra (or just leave it there for the amtool PR).

WRT main binaries, I guess the switch to Prometheus 2 will allow us anything. ;)

[brancz]

Before moving to cobra for each of these I would consider just switching the package from flag to pflag which would solve the main confusion. It's quite literally a drop in replacement, I recently switched a project from the core flag package to pflag and it was just changing the import. I would suggest switching Alertmanager to it for the next release and Prometheus and promtool for the 2.0 release. I don't see the benefit of moving Alertmanager and Prometheus to cobra but certainly see it for the pflags package.

[fabxc]

Sure, but that's literally the only visible part of it.
Cobra really only affects internal code structure and allows for some proper semantics around subcommands.

But isn't pflag using -- whereas flag uses -. I recall haveing tested it some longer time ago where pflag was not able to process - in a backwards compatible way. Do I remember that incorrectly?

I'd also at least think about the . separated flag structure. It has something going for it but also goes against the grain of any other tool out there I know. Not feeling too strongly but something we should also consider while we are thinking about moving to a more standard way.

[brancz]

If moving to cobra the -- vs - compatibility issue is something we have to think about either way as cobra requires the use of pflag internally.

[grobie]

I think it makes sense using a well established package to write CLI tools instead of building yet another one. We should focus on writing a metrics system instead.

My only issue with cobra/viper is the extensive use of globals and init() functions which make it quite hard to test and separate different concerns (I just ran into an issue in promu where one subcommand would need a different init() structure). I don't have enough experience to know whether that's an inherent problem of these libraries or just the common usage.

[fabxc]

I share your concerns around some general design decisions of that package. Yet I couldn't find another one that works as well and/or doesn't have even weirder edges.
The upside is, one can largely move this at the very edge and have the actual command functions separated out for better testing.

[sdurrheimer]

@grobie It is not mandatory to use init() and globals with cobra/viper. For example, docker doesn't do it like we did with promu.

I was keeping promu simple, but we can refactor it to use less init() and globals.

[juliusv]

Cobra (or even the other options) all sound agreeable to me.

[matthiasr]

I'm not a huge fan of Cobra but it's the least bad way as far as I can see.

I agree that we cannot break flags before 2.0, but we could allow additional forms if that can be done.

[fabxc]

So @gouthamve moved prometheus to Cobra while getting our 2.0 flags sorted out: #2844
Shortly after Kingpin was pointed out as a better alternative from a library perspective. Both has the same effects for the end user largely.

I tried out Kingpin in #2866 and I agree that it's really nice. Would be good to get more input on this. Especially since we'd switch over our other CLIs in the long run.

[gouthamve]

tbh, I don't see a huge gain given our CLIs are pretty simple ones.

In prometheus we don't have any commands at all.
In promtool, we have only commands and no sub-commands. But none of the commands use flags, making the structure of kingpin and cobra almost similar.

That said, I do agree that kingpin is better, but it comes with a non-standard flag system.

[grobie]

I've never liked cobra and the code structure it imposes. Happy to try something else.

[fabxc]

I'd like to release an alpha3 right now. I'll merge #2866 for that but if concerns come up I'm happy to paddle back for subsequent releases.

The flag package is indeed not a drop-in replacement anymore. But when using Cobra, every flag line had to be touched regardless.

[brian-brazil]

We still need help converting all of our other packages to this.

[carlpett]

I'd also at least think about the . separated flag structure. It has something going for it but also goes against the grain of any other tool out there I know. Not feeling too strongly but something we should also consider while we are thinking about moving to a more standard way.

Has this ship sailed, or is it still open for discussion? I'd like to add another point beside it being different from anything else. Windows users need to quote the flags when they have dots in them, such as "-log.level" debug. I appreciate that Windows is not a main platform in any way, but if it is not important to anyone, it could reduce a little bit of friction for those users.

Sorry if this derails the discussion a bit - I'm willing to atone by doing a few package conversions :) Is this just internal packages, or should it go for the "official" exporters as well?

[brian-brazil]

This would cover all the Go code under the Prometheus Github organisation.

[carlpett]

Good. Should the exporters aim to release this change at the same time as Prometheus 2.0 so that the user experience is somewhat unified, or just release each as soon as they are done? For libraries in prometheus/common, I guess everything is vendored everywhere, so those are simple to just change? Looks like it is only logging and possibly model/time.go that are affected there, though.

[brian-brazil]

Unless someone want to put in a good bit of effort, I don't think a unified release is likely. I'd say get the code all updated, and each exporter is released as it's released.

[carlpett]

Yup, that seems most realistic. But then the question of the the dots. Are there good reasons for keeping them? The two (or 1.5, maybe) arguments against are

1. Different from just about everything else (@fabxc)
2. Flag parsing errors on Windows

Since all users will have to change their flags as part of the 2.0 upgrade anyway, there's no backwards compatibility concerns.

[juliusv]

I love the way the dots allow us to have clear namespacing in flag names, a thing that we have sometimes missed from metric names, ironically. I'm not sure whether (1) is a disadvantage since if people just copy the flag names, it shouldn't matter much whether they're different from anything else. (2) is a concern of course. I'm not sure if it's enough to sway my opinion the other way, but then again I rarely ever deal with Windows. So I'll go with what @brian-brazil or @fabxc or so decide. I will still personally cry about lack of dots though in case the decision is to remove them :)

[juliusv]

To elaborate: without dots, we just have dashes as separators. But dashes don't make clear whether they separate namespaces or just words. So the boundaries of groupings would not be clearly defined anymore.

[brian-brazil]

I've a mild preference for dots.

[grobie]

I'm also in favor of keeping them.

[carlpett]

Agree that they do have a nice namespacing purpose. So, looks like there is a solid assembly behind keeping them, then.
Not sure how we'll handle this in wmi_exporter, since the UX gets pretty poor (the main issue is our usage of common/log, our own flags are easy to change), but we'll find some good way though, I'm sure.

I'll go ahead and convert a few packages, then!

[grobie]

I don't think there is a requirement to use the same flag style on all components. If dots are problematic on Windows, it'd make sense to me to use something else for the wmi exporter.

…

On Mon, Jul 24, 2017 at 8:54 PM Calle Pettersson ***@***.***> wrote: Agree that they do have a nice namespacing purpose. So, looks like there is a solid assembly behind keeping them, then. Not sure how we'll handle this in wmi_exporter, since the UX gets pretty poor (the main issue is our usage of common/log, our own flags are easy to change), but we'll find some good way though, I'm sure. I'll go ahead and convert a few packages, then! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2455 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaInRyUseKm_LOO8E9Zy0OZj0GPJSks5sROhYgaJpZM4MOSAX> .

[carlpett]

@grobie Yeah, the issue with just changing the flags in wmi_exporter is that common/log sets flags itself (log.level etc), which we inherit. Otherwise a change in the exporter would have been very simple.

[gouthamve]

@carlpett, so we moved away from that now. We are not using the stdlib for the flags but rather use kingpin for the flags.

See here where we explicitly register the logging flags: https://github.com/prometheus/prometheus/blob/dev-2.0/cmd/prometheus/main.go#L95-L101 I think its best if you move to kingpin too as that is what the rest of the exporters will move to soon-ish.

[carlpett]

@gouthamve Thanks! I was aware of the move to Kingpin, but I hadn't seen that the log package no longer will register its own flags. That removes that complication for us :)

I had a branch on wmi_exporter already where I switched to Kingpin, so this will make that very easy.

[gouthamve]

We now moved to kingpin for CLI/flags. Closing this now.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.