Logrus: Breaking API Change in IsTerminal()

[bbrks]

Hi,

Just raising an issue to let you know #471 seemed to break the API without incrementing the major version number. I'm not asking for a revert or change, just logging an issue to help with people trying to debug if they encounter the same thing.

logrus.IsTerminal() used to take no parameters, now it takes an io.Writer interface.

Compilation error for searchability:
not enough arguments in call to logrus.IsTerminal

[sirupsen]

Yeah, I did consider this—the trade-off here is to fix a bunch of TTY-related bugs for a function that I wouldn't expect people to use publicly. A TTY-package should likely be used for this. Regardless, I think it's a bug to say logrus.IsTerminal(). It's completely opaque what that means. Is the main logger's out a TTY? Is stderr a TTY? It inherently needs to take an object to infer an FD from, IMO.

When doing this work it's weighed between how many people breaking backwards compatibility would affect (in this case, my hunch is few) between how many people the bug is affecting (lots of problems on the Internet about people who've changed their out and are getting coloured logs). It's possible to make this change with complete backwards compatibility, with the cost of cruft, but I didn't think this was going to affect many. We can revisit this if this is affecting many people.

Thanks for logging this issue @bbrks

[bbrks]

No worries @sirupsen, we only ran into the issue as we have a custom formatter for our logs, based on Logrus' own, and it was easily fixed by merging upstream changes :) I did think it's likely not a widely used function.

[sirupsen]

Out of curiosity (I have considered removing formatters in favour of hooks for years), what are you using your formatter for?

[bbrks]

We only have a few changes to suit our particular use. In particular:

• Added DefaultFields to the TextFormatter struct and append them in the Format method. This allows us to share a global logger which has a set of default fields (such as the name of the service, which environment it is running in, etc.)
• Minor changes to the way fields are quoted to suit our logging infrastructure (quote empty fields, quote with single quotes rather than double)

[sirupsen]

Added DefaultFields to the TextFormatter struct and append them in the Format method. This allows us to share a global logger which has a set of default fields (such as the name of the service, which environment it is running in, etc.)

Is there any reason you don't pass a logrus.Entry around? If you do logger.WithFields(..) you can pass that around for an environment-specific logger. I end up doing this a lot.

Minor changes to the way fields are quoted to suit our logging infrastructure (quote empty fields, quote with single quotes rather than double)

These actually seem like useful additions to the upstream TextFormatter

[bbrks]

We've completely missed that you can pass around a logrus.Entry with a set of fields. This makes much more sense and works exactly the same as our default fields thing. Thanks!

I'll put together a PR soon for allowing some configuration in the upstream formatter. Thanks for your work on logrus. Feel free to close this issue whenever.

[sirupsen]

We've completely missed that you can pass around a logrus.Entry with a set of fields.

@bbrks how do you think we can make that use-case more discoverable in the README and documentation? Would you be interested in contributing a patch that would surface this better?

I'll put together a PR soon for allowing some configuration in the upstream formatter. Thanks for your work on logrus. Feel free to close this issue whenever.

Great. I'm keeping this open and searchable since others may run into the same issue.

[bbrks]

@sirupsen It probably got lost as it was right at the bottom of a code block, with only inline comments describing it. I personally only read code blocks in documentation if the surrounding header/text has described what I'm looking for.

Maybe mentioning something about default fields a Ctrl+F would be able to find it.

[sirupsen]

Added to the README as an explicit example https://github.com/sirupsen/logrus/commit/3f603f494d61c73457fb234161d8982b9f0f0b71

[bbrks]

@sirupsen perfect :)

[bbuck]

@sirupsen I originally implemented an io.Writer to process special color codes that my application uses, but since this change I've discovered I can probably handle this fine via hooks. Although I still have issues with this change -- it's breaking a core concept of my application which is multiple log targets. For those I take advantage of io.MultiWriter to group more than one io.Writer which can't be cast as an *os.File to be determined to be a "terminal." It would be great if you added a logrus.Terminaler (that's probably a terrible name, just a quick example) interface that would let custom io.Writer types determine if they, themselves are a terminal or not.

To be clear, I'm not asking for any kind of reversion, but an interface that would let custom writers tell logrus if they are a "terminal" or not.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.