Add a way for hooks to mutate/write entry Data fields

[epelc]

Hooks should have a way to safely(re goroutine safe) write/update/add fields to entries. There are several issues stemming from the data field not having a lock and hooks not having any way to mutate fields.

One option is to switch from using a hook to using a formatter but you cannot easily wrap formatters while safely modifying fields. ie you cannot directly modify Data due to race conditions, you cannot use entry.WithFields then pass to say json formatter because that screws up entry release/locks/buffer usage, etc.

Use Case

Our use case is a simple hook which adds a hostname to every log entry to help differentiate which server a log came from.

Potential solutions

All current solutions require forks/minor changes if you are using the JSON formatter due to internal fields. They do not all touch this though, only the use a formatter option below(2nd option).

Improve Hooks API(best)

Adding a way for hooks to return a new entry to replace the current one would allow easily modifying/adding new fields. This would probably have the lowest api impact and be the easiest to implement. Hooks could optionally implement a MutatbleHook interface which would take precident over the existing Fire. ie something like FireAndMutate(*Entry) (*Entry, error) where the returned entry is passed onto the next hook and used if non-nil. If any hook returned a new entry then it could be used in place of the current one.

Use a formatter instead(alternative, also very good if formatters could be wrapped easily while modifying entries)

The only real current option around is to modify the json formatter or your target formatter to directly include your new/modified/updated field logic. Even doing this hardly works and requires a fork of the repo due to the Entry.err field being private.

The main issue with this is that Entry.WithField does not create a full copy(ie the error, buffer, and potentially other fields are not copied). It also has issues with releasing entries when calling from within a formatter. I did not fully investigate this although a simple option is to modify the underlying formatter(ie JSON) itself to simply add the field before sending to json.Encoder.Encode. This side steps all the issues/perf with creating entry copies/modifications.

Add Data locks

Another option is to add a mutex around the Data field so that hooks could safely modify them.

See the following related issues.
#1046

#953
vmware-tanzu/velero#599

[epelc]

I've added a PR for a solution which we are using. It happens to be the simplest to implement and also the most performant but requires updating each formatter to support it. Luckily I believe most people use the JSON formatter.

If you are sending logs elsewhere then you can implement a similar feature by copying data fields within a Hook then send the log entry to a third party. Some people use JSON + a log collector though instead of directly having your application ship logs.

[dgsb]

Hello @epelc, your analysis is quite complete.
I think your fix would solve your issue, but as you've noticed it won't solve issues on other formatter.
I'd rather not merge that as logrus code is kind of already bloated of several minor features only a few users are using.
I'm currently working on per entry rwlock fix, the lock can then activated at logger configuration level in order to not have a performance drawback for users not having modifying hook.

Just curious, as another work around, why don't you just set up a new entry as the default logger for your application instead of using a hook for setting a value (the hostname) which never changes throughout the life of your application.

defaultLogger := logrus.WithField("hostname", hostname)

You just need then to cascade the logger through your application stack.

[epelc]

@dgsb Thank you for the response! Also very excited to hear about the fixes for lock support.

I agree this may be too specific a use case to accept. The lock support should resolve this without bloat.

The only real thing I'd note is that it'd be nice to be able to implement a modified JSON formatter without forking. ie make the internal APIs it uses public in some way. This fork has been working very well for us for the past month though.

I think the main issue with the defaultLogger suggestion is that not all code has a global logger and it'd be a lot of work to update everything to use it. It would work though if you started a fresh project.

[dgsb]

I have a fix which doesn't add any interface changes nor a mutex usage. You may want to check the next release of logrus if you're interested.

[dgsb]

@epelc can we close this issue ?

[epelc]

@dgsb yes the locking you added should fix it.