Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

prefix all logger.warn calls with WARNING. #721

Merged
merged 3 commits into from Mar 18, 2013

Conversation

Projects
None yet
5 participants
Contributor

smurthas commented Dec 4, 2012

This will make them grep-able so we can set (papertrail) alerts.

cc @quartzjer @temas @dizzyd

Contributor

dizzyd commented Dec 4, 2012

Good idea, but all log levels should do something like this, imo. I.e. DEBUG, ERROR, etc.

Contributor

smurthas commented Dec 4, 2012

The intent behind this specific one is that we often write code with logger.warn thinking "Oh, we'll just monitor for this and if anything shows up, we'll react," but then dont monitor it. This would act as a catch all for those cases.

I don't know if I really see why it would be helpful to see the other levels (other and error, I suppose)?

Contributor

dizzyd commented Dec 4, 2012

Like the addition of "WARNING" it would provide context about the intended severity of the message. Typically there are four levels of logging: error, warning, info, debug.

@kristjan kristjan commented on an outdated diff Dec 4, 2012

lib/logger.js
@@ -83,7 +83,9 @@ Console.prototype.onLog = function(logger, level, args) {
if (level < this.filter) return;
var msg = util.format.apply(this, args);
msg = msg[rlevels[level]] || msg;
- msg = this.prefix(logger) + msg;
+ var prfx = this.prefix(logger);
+ if (level === levels.warn) prfx += 'WARNING: '.yellow;
@kristjan

kristjan Dec 4, 2012

Contributor

If you wrap it like [WARN], we won't get false positives if the string warning appears elsewhere. I agree with @dizzyd that we may as well record [:level].

smurthas and others added some commits Dec 3, 2012

@smurthas @beaugunderson smurthas prefix all logger.warn calls with WARNING.
This will make them grep-able so we can set (papertrail) alerts.
4fb4c01
@beaugunderson beaugunderson Improvements to logger
- Rip out File logger (unused)
- Clean up handling logic (unused)
- Remove unused color themes
- Roll LOGLOCAL into logger rather than doing it ad hoc
- Separate with pipes rather than []'s (saves space)
f8b51e7
Contributor

beaugunderson commented Mar 18, 2013

@temas or @kristjan I spruced this up a bit, can you give it a look-see?

New format looks like this (under foreman):

13:40:41 apihost.1   | hallwayd|INFO Starting an API host
13:40:42 apihost.1   | hallwayd|INFO Hallway is now listening at 0.0.0.0:8045
13:40:42 apihost.1   | hallwayd|INFO Hallway is up and running.
13:40:48 apihost.1   | acl|DEBUG getting account profiles faca591550408129bce5b78168a8ed04
13:40:48 apihost.1   | dal-mysql|DEBUG connecting 1
13:40:48 apihost.1   | dal-mysql|DEBUG >> mysql: SELECT profile FROM Accounts WHERE account = 'faca591550408129bce5b78168a8ed04'
13:40:48 apihost.1   | dal-mysql|DEBUG << mysql: SELECT profile FROM Accounts WHERE account = 'faca591550408129bce5b78168a8ed04'
13:40:48 apihost.1   | tokenz|WARN error getting profile null []
13:40:48 apihost.1   | middleware|LOGLOCAL GET /services/facebook/checkins?access_token=derp&account=faca591550408129bce5b78168a8ed04 401 30.001218
Contributor

temas commented Mar 18, 2013

Format seems, fine, but I'd propose a change on loglocal. Can we instead make it a level that automatically becomes loglocal if it's under that? So we could set loglocal to info and loglevel to info so the local logs would get info and under, warn and higher would go remote? I don't think that's clear but maybe you get the idea. I just don't really like the explicit call.

@beaugunderson beaugunderson Add localLevel, logger.vital
lconfig.logging.localLevel controls what log messages are prefixed with
LOGLOCAL. It defaults to `debug` so that it doesn't affect your local log
messages. In production it should be set to `vital`, so that `info` and `debug`
messages are prefixed with LOGLOCAL and don't get sent to papertrail.

I also audited all our uses of logger.info and promoted some to logger.vital
since they should be sent to papertrail (even though they aren't warnings).
4361786
Contributor

temas commented Mar 18, 2013

very +1 with the new changes. Just a reminder to do the production config to vital for the locallevel

@beaugunderson beaugunderson added a commit that referenced this pull request Mar 18, 2013

@beaugunderson beaugunderson Merge pull request #721 from Singly/logger-warn
prefix all logger.warn calls with WARNING.
104b1bc

@beaugunderson beaugunderson merged commit 104b1bc into master Mar 18, 2013

1 check passed

default The Travis build passed
Details

@beaugunderson beaugunderson deleted the logger-warn branch Mar 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment