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

fix: Include stacktrace into error message #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ayzrian
Copy link

@Ayzrian Ayzrian commented May 7, 2021

The problem

Log format errors actually confuses users. When a user specifies stack: true he or she expects that the stack trace will be included into the message, but it does not. For instance, this issue is a great example of such confusion winstonjs/winston#1498

Because we are not appending the message to the info.message and info[MESSAGE] some formatters may not show the actual stack trace, for example, simple.

Solution

The easy way is to append the stack trace to the info.message and info[MESSAGE] if stack: true, and leave info.stack = stack for backward compatibility with other code that was written so far

@Ayzrian
Copy link
Author

Ayzrian commented May 11, 2021

@indexzero excuse me for bothering, but could you please review this and merge if everything is okay? (It is kinda pain that a lot of users encountered with winston)

@BrennerSpear
Copy link

Bump. I love everything else about Winston but not being able to see my error stack traces is pretty much a deal breaker ;(

@wbt
Copy link
Contributor

wbt commented Jan 20, 2022

Thanks for the PR.
I'm not sure that appending the stack to the message parameter is really the best way to go.

The issue discussion pointed to a related issue in winston-transport reportedly fixed about 3 years ago; the issue was closed in #1576. More complex handling was explicitly rejected by a core maintainer for performance reasons. The subsequent comment that folks seem to have picked up on as an OP for reopening a new issue under a closed one, with a spirit of "I want to do more complex stack trace display but only ever use simple()" seems to have missed that core maintainer's message of "use a format matching the functionality you want rather than making simple formatters more complex."

It would seem consistent with that post to accept improvements making an alternative formatter that provides stack trace information easier to use, but not to make a simple formatter that works well for some use cases more complex.

@BrennerSpear
Copy link

Thank you for the detailed response with links. I really appreciate it. I will try out @medington's code.

In general, I think the request of "make it super easy to log the stack trace, especially when you're developing / debugging" is a reasonable request of a logging package, and seems to have not been adequately addressed for years.

my current hack is:
export const logger = isProdEnv ? winstonLogger : console;

which seems... pretty hacky

@wbt
Copy link
Contributor

wbt commented Jan 20, 2022

I agree that seems hacky; I think there should be (whether or not there is today) some way to include a few lines in your construction of the winstonLogger that incorporates a formatter giving you that.

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 this pull request may close these issues.

3 participants