promlog: add support for json logger

[alexander-yu]

Note: this changes the signature of promlog.New, so any repos that use this method in prometheus will need to be changed and will need to include the log.format flag.

As far as I'm aware, functions whose signatures are affected (i.e. promlog.New and flag.AddFlags) are used in prometheus/cmd/prometheus/main.go, alertmanager/cmd/alertmanager/main.go, and blackbox_exporter/main.go, so these will need to be updated at some point, at least when the vendor directories are updated.

Fixes issue: prometheus/prometheus#3219

Should I include some sort of test for this feature (and maybe the log level flag as well)?

@krasi-georgiev

[brian-brazil]

This shouldn't require an API change, this should always default to what we already do.

[alexander-yu]

Fair enough; would it be better to add a new method like promlog.NewJSON? Also, should flag.AddFlags remain modified, or should there also be a separate method for adding the format flag?

[brian-brazil]

The whole point of this design is that changes to the log library should only require a revendoring, no code changes.

[alexander-yu]

Yes, that makes sense; I was only asking about flag.AddFlags because its name (and its commented description) seems to imply semantically that it should be for adding any flags used in the package, but its signature right now is fixed at only supporting a single flag (i.e. the log level).

[brian-brazil]

The function allows you to change the default flag value for that one flag. There's no need for the others to have defaults.

[alexander-yu]

@brian-brazil any thoughts on the PR as it is? It's been modified so that an API change isn't necessary.

[ntindall]

@krasi-georgiev @brian-brazil would love to see this into the prometheus mainline - I think this change is sufficient to facilitate some minor changes to cmd.

[ntindall]

@alexander-yu any idea why the CI is failing?

[alexander-yu]

It's due to an earlier merged PR -- #134 is a fix for it, I believe.

[simonpasquier]

I might be missing something obvious but I don't see how this is going to work for Prometheus. How will the main() know about the flag's value?

[brian-brazil]

It shouldn't need to.

[ntindall]

@brian-brazil what do we need to do to get this in?

[brian-brazil]

This shouldn't be here. This is something for the user to configure, not the developer.

[alexander-yu]

Would you recommend that this function instead exist wherever users within prometheus are setting up their loggers? It made sense to put this in the same place as New, since these are doing equivalent things, except with different loggers.

[brian-brazil]

This function shouldn't exist at all.

[alexander-yu]

@brian-brazil Removed the NewJSON function, also rebased.

[brian-brazil]

How does this code actually set the level? It just seems to be creating a flag.

[alexander-yu]

As a flag? That should be set already by AddFlags (and then by the user of this parsing command args). If you mean setting the log level to the JSON logger, that's what the NewJSON function (now removed) was doing, which is functionality that (now) should be done by the user of the flag.

The problem with the current PR is that the user doesn't really have a way to access this flag; could be done by modifying the signature of AddFlags to take a *string as an argument. It would require changes on users within prometheus that call this method, though, which is a concern that you already brought up earlier. Here for example, it would want to pass in a pointer to a string var that has this flag, and then sets up the JSON logger if necessary, probably around here.

Alternatively, do you prefer that any part of prometheus that wants to have support for a JSON logger manually do the setup themselves, rather than providing for that functionality here? If so, I can instead make PRs in the corresponding places.

[brian-brazil]

The various flags, current and future, should be opaque to any users of this library. Can you suggest something that'd achieve that, even if it requires one-off changes to existing users?

[alexander-yu]

Sure; so thinking about it some more, we could do something that mirrors how the log level is currently set; we could have a struct AllowedFormat that looks like this:

type AllowedFormat struct {
	s string
}

func (l *AllowedFormat) String() string {
	return l.s
}

func (l *AllowedLevel) Set(s string) error {
	switch s {
	case "logfmt", "json":
		l.s = s
	default:
		return errors.Errorf("unrecognized log format %q", s)
	}
	return nil
}

which makes sure that we can only have the two specified formats. We'd then have AddFlags look like this:

func AddFlags(a *kingpin.Application, logLevel *promlog.AllowedLevel, logFormat *promlog.AllowedFormat) {
	a.Flag(LevelFlagName, LevelFlagHelp).
		Default("info").SetValue(logLevel)
	a.Flag(FormatFlagName, FormatFlagHelp).
		Default("logfmt").SetValue(logFormat)
}

so it'll set the value of the log format for whatever user is calling this method.

If we wanted to keep the exact internal values of the AllowedFormat flag isolated from users, then we could modify the New logger method to look like:

func New(al AllowedLevel, af AllowedFormat) log.Logger {
	var l log.Logger
	if af.s == "logfmt" {
		l = log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr))
	} else {
		l = log.NewJSONLogger(log.NewSyncWriter(os.Stderr))
	}
	
	l = level.NewFilter(l, al.o)
	l = log.With(l, "ts", log.DefaultTimestampUTC, "caller", log.DefaultCaller)
	return l
}

then on the client side, we might have something like this:

promlogflag.AddFlags(a, &cfg.logLevel, &cfg.logFormat)
...
logger := promlog.New(cfg.logLevel, cfg.logFormat)

So on the client side, this means that there are three things they'd need to do to support this, which could easily be done in a single PR:

• Provide some promlog.AllowedFormat var (potentially as an additional config field, or just as a one-off var) to store the flag value
• Modify the call to AddFlags
• Modify the call to New

[brian-brazil]

When the next setting comes along, that'd require every user of this code to change their code - not just their vendoring.

[brian-brazil]

We probably need either an opaque struct, or that the initialisation function looks at the flag globals.

[alexander-yu]

Sorry I deleted my comment; was writing this up instead.

Right, that makes sense; I was thinking we change AddFlags and New to look like:

func AddFlags(a *kingpin.Application, config *promlog.Config) {
    ...
}

func New(config *promlog.Config) {
    ...
}

where the config struct looks like:

type Config struct {
    level  *AllowedLevel
    format *AllowedFormat
}

This way the clients only need to change the calls once; if we want to add more settings we simply add fields to the struct, and clients can choose whether or not to instantiate these fields whenever they want; revendoring only means that the new fields aren't populated (and probably set to some default when AddFlags is called).

[brian-brazil]

That seems fine to me. Though the fields are private so can't be changed by users, which seems fine too.

[alexander-yu]

Sounds great; I incorporated the changes, though I had to make the fields public/exported, since flag.go needs access to those fields.

[carlpett]

Are there any blockers for merging this still?

[brian-brazil]

That looks about right, but we should wait for 2.5 to be out before merging.

[carlpett]

So, that would be about now then? :)
Sorry for the nagging

[brian-brazil]

Thanks!

Next you'll need to update the repos using this.

[ntindall]

🎉 !