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

Change flags to be Windows friendlier #96

Closed
carlpett opened this issue Jul 19, 2017 · 8 comments · Fixed by #112
Closed

Change flags to be Windows friendlier #96

carlpett opened this issue Jul 19, 2017 · 8 comments · Fixed by #112

Comments

@carlpett
Copy link
Collaborator

The dotted flags we currently use (-collectors.enabled etc) don't play nice with how Powershell tokenizes stuff, so users need to quote the flags ("-collectors.enabled"). (See also #91)
Since starting the exporter from Powershell is a pretty standard use-case, platform coherence and compatibility seems more important than imitating other (primarily Linux-focused) parts of the ecosystem.

One stumbling block here is that the logging library we use (github.com/prometheus/common/log) defines its own flags as part of initialization. So just changing our flags would lead to a mix, which would be worse.

I'm not aware of any way to change the flags already set by another package, but maybe it is possible? Another possibility would be to have a patch on the log library, but I'm not too keen on diverging there.

@martinlindhe
Copy link
Collaborator

I have not tired, but maybe we can just overwrite flag.CommandLine, as set here:
https://github.com/prometheus/common/blob/master/log/log.go#L72

documented here: https://golang.org/pkg/flag/#pkg-variables
CommandLine is the default set of command-line flags, parsed from os.Args.

@martinlindhe
Copy link
Collaborator

Otherwise I also think it's better to not do changes in prometheus.
If we cannot solve this, we should open a bug on prometheus/common about our use case.

I also noted their logger had notes about syslogging not working on windows, something that would be beneficial if we could get wired up, too.

@carlpett
Copy link
Collaborator Author

Just read this on the Prometheus blog for the 2.0 series:

First, we moved to a new flag library, which uses the more common double-dash -- prefix for flags instead of the single dash Prometheus used so far.

So, this seems we can just move along with the rest then :)

@carlpett
Copy link
Collaborator Author

I've been converting a few of the exporters in the Prometheus org to use Kingpin, so I'll do it here as well. With the double dashes, Powershell does not have the same parsing behaviour, so we'll be clear of the problems in #91 as well.
2.0 seems a bit aggressive, though, maybe 1.0? :)

@martinlindhe
Copy link
Collaborator

Hehe, you are right. I was thinking 2.0 as in "prometheus 2.0", didn't reflect of our version numbering which would made more sense.

This is great news!

@martinlindhe
Copy link
Collaborator

By the way, I haven't looked at if we need to do updates to be compatible with prometheus 2.0 (as in the exporter working with it), or if it is just api related internal changes.
I was thinking we should bump versioning to maybe 1.0 (or 2.0 to cause less confusion for end users?) when we are updated for prometheus 2.0

@carlpett
Copy link
Collaborator Author

The exposition format is identical afaik, so we do not need to do anything there. I also do not think any other exporters plan any versioning bumps to indicate v2 compatibility.
I do think that we can consider ourselves stable enough for a 1.0 release pretty soon, though. After this flag change (which is a breaking change), I'm not aware of any other large changes we need to do. Agree?

@martinlindhe
Copy link
Collaborator

The flags are breaking and warrants a major bump i think, yes. Okay, lets aim for a 1.0. Fixing the label

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 a pull request may close this issue.

2 participants