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

Add option to stop waterline from posting multi-line log entries #4737

Open
alxndrsn opened this issue Apr 11, 2019 · 9 comments · May be fixed by balderdashy/waterline#1606
Open

Add option to stop waterline from posting multi-line log entries #4737

alxndrsn opened this issue Apr 11, 2019 · 9 comments · May be fixed by balderdashy/waterline#1606
Labels
has pr There is an open pull request (in this repo or elsewhere) related to this issue. orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. proposal what do you think? Community feedback requested

Comments

@alxndrsn
Copy link

alxndrsn commented Apr 11, 2019

Waterline provides some really helpful error messages when there are weird/incorrect database configurations. However, these are sent as multi-line log entries, which doesn't really make sense in production systems. (e.g. MIGHT_BE_YOUR_FAULT).

It would be great to have an option to change these multi-line messages to be sent as a single line.

Alternatively, if waterline were to log via the sails logger rather than using console.error, console.warn etc. then it would be easier to control these messages.

@sailsbot
Copy link

@alxndrsn Thanks for posting, we'll take a look as soon as possible.


For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

@alxndrsn alxndrsn changed the title Add option to stop sails from posting multi-line log entries Add option to stop waterline from posting multi-line log entries Apr 11, 2019
@johnabrams7 johnabrams7 added orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. proposal labels Apr 11, 2019
@mikermcneil
Copy link
Member

@alxndrsn I see where you're coming from, although I don't entirely agree with this:

which doesn't really make sense in production systems

We rely on these logs in our own production systems, and having extra information (especially when it comes to stack traces) can be really helpful to figure out which customers/orders/etc. were affected.

That said, it would make sense to me to have the option for fewer troubleshooting tips, etc in production for a lot of the errors (and especially some of the warnings).

I'm not sure adding a new config setting is the right way to go, though (I've learned the hard way it's easy to add those and much much more annoying to maintain them).

Maybe what we should do is identify some specific examples of errors that are too long (i.e. drop a github link to the line of code where they come from -- easiest way to find that is to search github for a chunk of the error msg). Then we'll be able to address improving them (or conditionally shortening them) on a case by case basis.

Re: conditional shortening, I'm thinking it would probably work the same way as how production stack traces work (i.e. normally we improve stack traces by instantiating Error instances called omens -- but we skip doing that in prod because it has a (albeit small) negative impact on performance. We do that based on env vars: we'd shorten only if either NODE_ENV==='production' and DEBUG is not truthy).

@mikermcneil mikermcneil added more info please what do you think? Community feedback requested labels Apr 11, 2019
@alxndrsn
Copy link
Author

@mikermcneil thanks for the considered response.

We rely on these logs in our own production systems, and having extra information (especially when it comes to stack traces) can be really helpful to figure out which customers/orders/etc. were affected.

I've found the content of these errors very helpful including in production. My issue is specifically with error messages which include linebreaks: in production these are written as multiple log entries which are very hard to filter out.

If we could just remove the linebreaks, this would immediately solve my problem.

@sailsbot sailsbot removed what do you think? Community feedback requested more info please labels Apr 25, 2019
@mikermcneil
Copy link
Member

@alxndrsn that makes sense! OK, so then I think the next step is for us to identify some specific examples of production errors that are too long (i.e. drop a github link to the line of code where they come from -- easiest way to find that is to search github for a chunk of the error msg). Then we can address them one by one.

There are probably errors that you see during dev that should still probably span multiple lines (e.g. sails CLI related stuff), but for the other errors, I'm down to look at consolidating them onto a single line. (We'll just have to deal with it on a case-by-case basis)

@alxndrsn
Copy link
Author

@mikermcneil
Copy link
Member

mikermcneil commented Apr 29, 2019

Sure enough - I'm cool with all of those living on a single line, because they are oftentimes seen in production logs (although in my experience, if they're showing up in production logs, that's a bad sign, because it means there needed to be some migration that didn't happen. Still, it'd be nice to actually be able to read what's going on in that situation! Or to be able to have an alert that neatly reports it in a Slack channel, etc)

@mikermcneil mikermcneil added the what do you think? Community feedback requested label Apr 29, 2019
@alxndrsn
Copy link
Author

PR at balderdashy/waterline#1606

@sailsbot sailsbot removed the what do you think? Community feedback requested label May 17, 2019
@johnabrams7 johnabrams7 added the has pr There is an open pull request (in this repo or elsewhere) related to this issue. label May 20, 2019
@rachaelshaw
Copy link
Member

@alxndrsn just took a look at your PR and added some feedback, let me know what you think: balderdashy/waterline#1606 (comment)

@wulfsolter
Copy link
Contributor

wulfsolter commented Jul 1, 2019

Changing content of error messages will break tests that have expectations of a certain response/error.

EDIT: Initial comment in opposition, I have thought about it longer now and support @alxndrsn

The referenced PR will add a non-zero amount of work , for what I can only interpret as a cosmetic change. I would support an option, as stated in title, but oppose breaking existing behaviour. Just my 2c though. in updating existing tests but single line (and thus single error message) is "the right thing to do".

Single line of error = one error. Currently the multiple lines of errors are being treated as multiple errors by logging tools

@whichking whichking added the what do you think? Community feedback requested label Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has pr There is an open pull request (in this repo or elsewhere) related to this issue. orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. proposal what do you think? Community feedback requested
Development

Successfully merging a pull request may close this issue.

7 participants