consider making cmd/prometheus config defaults as package-level vars

[mischief]

in ./cmd/prometheus/config.go, the default directories for assets such as consoles are inline in the cli flag declarations.

if these defaults were moved out into package level string vars, they could be set with go's -ldflags -X, which would allow a packaging system to set the defaults appropriate to where these files will be installed.

[fabxc]

That seems like a good idea and in a main package having that inlined and unexported struct probably has little value.

Which extend do you imagine with this? Should all flags point to an exported package variable?

[brian-brazil]

I don't like the idea of making it harder to figure out what the defaults are from the code in the general case.

[fabxc]

Mh, but the defaults would still be clear in the help text.

[brian-brazil]

Thinking a bit more, I'm against this. Flags should be spread liberally throughout the codebase, and adding a 2nd way to configure our flags that all flags usage had to follow would be an additional burden that'd result in fewer flags in a codebase that's already hardcoding a bit much.

[fabxc]

No, flags should not be spread throughout the code base. This was
consciously removed long time ago and we had this discussions multiple
times over the last few months. Flags are fine, they are passed down as
configuration options but are all defined in one place. Regardless of other
arguments against this issue, this is not one of them.

On Tue, Sep 20, 2016 at 7:49 PM Brian Brazil notifications@github.com
wrote:

Thinking a bit more, I'm against this. Flags should be spread liberally
throughout the codebase, and adding a 2nd way to configure our flags that
all flags usage had to follow would be an additional burden that'd result
in fewer flags in a codebase that's already hardcoding a bit much.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#1976 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEuA8rXQyRWTir5qSXwCK3lPJvnzZdTQks5qsByygaJpZM4J52cL
.

[brian-brazil]

This is an argument against it, as I continue to believe we're using flags in an incorrect fashion (basically we're pre-maturely optimising for every single flag being relevant to users using Prometheus as a series of modules; rather than the more common case of them being rarely altered values by operators).

[beorn7]

Without implying anything for what's suggested in this issue:

In our given situation (and even more so now that more and more people use packages from the Prometheus codebase as libraries), I am firmly in the camp of restricting flags to the main package and pass down configuration options as parameters of type constructors. (Option structs are a well established way to provide reasonable defaults on the package level and thus avoid parameter sprawl.) Reasons have been discussed elsewhere, I'm not repeating them here. I just wanted to cast my vote, in case there were any doubts.

[brian-brazil]

If we're going to discuss this particular issue, why should we accept this additional method of configuring flags and not environment variables, files or via http which we have all previously rejected?

[fabxc]

It would be different as it doesn't configure directly but just adjusts the
defaults and this at build time.

But I think we should have a more direct use case than thinking that it
would be useful.

On Thu, Sep 22, 2016, 12:41 PM Brian Brazil notifications@github.com
wrote:

If we're going to discuss this particular issue, why should we accept this
additional method of configuring flags and not environment variables, files
or via http which we have all previously rejected?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#1976 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEuA8lctrd4mP3WAwxSs0scwarrNYLqsks5qsltpgaJpZM4J52cL
.

[brian-brazil]

That brings many of the same challenges as the others: it's an additional way to do things, and it's no longer sufficient to look at the command line to know what is being passed which will make debugging and support harder (imagine someone getting one of these binaries and being confused when we tell them e.g. the default retention is 15 days if you haven't changed anything).

This use case could be handled by a shell script wrapper, or more likely as this would be for use in a packaging system via a /etc/defaults file.

[discordianfish]

I think setting default as build time is established thing for long time. At least stuff like changing a prefix and defaulting to a path within that prefix to find config files etc.
But I see @brian-brazil's point and spend too much time figuring out which config/directories a binary uses.
All in all I agree with @fabxc, we should wait for a specific requirement and then can see how to solve that.

[brian-brazil]

It's established in C-ish binaries built via automake/autoconfigure, it's not established in Go.

[juliusv]

Voting 👍 on having all flags in main only, unsure about the build-time default value injection though.

@TheTincho - is this something that would be useful for Debian packages, for examples?

[TheTincho]

If you move the flags definitions somewhere else, without being configurable via linker options, does not change much for me. I still need to patch the source to conform to Debian standards (only it is simpler to keep track of them if they are in a single file).

Now, if these were configurable via -ldflags, then I could drop those patches. No big deal, but it makes things tidier and simpler for me.

[discordianfish]

@TheTincho So you will/need to change the defaults anyway, right? In that case I think something settable via ldflags is preferable over downstream having to ship patched versions..

[brian-brazil]

@discordianfish That argument can be applied to every other way we've been asked to support configuring these.

[discordianfish]

@brian-brazil That's right, the question is do we want to support configuring the defaults or not. If we don't, people who need to change this will patch Prometheus which I would say is worse.

[brian-brazil]

It's their choice to patch, rather than adding a wrapper which is what everyone else seems to do.

[discordianfish]

@TheTincho Do you know what Debian is patching those things instead of using wrapper scripts (or just specify flags on startup)?

[TheTincho]

We do both:

• Patching the source to set defaults for paths that match what is installed. This is useful for example if the user runs prometheus without the system wrapper.
• Adding a wrapper to allow the user to change the defaults: paths and anything else.

[brian-brazil]

Doesn't look like we'll be changing this.

[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.