push/pop() context #6

ixti · 2011-04-24T01:26:34Z

This patch gives ability to push and then pop context. e.g.:

logger.push('filters').debug('setting filters');
chains.forEach(function(chain) {
  logger.push(chain);
  chain.forEach(function(filter) {
    logger.debug('processing ' + filter.name);
  }
  logger.pop();
}
logger.pop();

will produce something like:

debug: [filters] setting filters
debug: [filters::before] processing needs_authentication
debug: [filters::before] processing inject_some_data

withContext(context) creates new

ixti · 2011-04-25T01:39:53Z

Added withContext() method to the pull request, so now it's also possible to do it like this:

var _logger = logger.withContext('filters').debug('setting filters');
chains.forEach(function(chain) {
  _logger.push(chain);
  chain.forEach(function(filter) {
    _logger.debug('processing ' + filter.name);
  }
  _logger.pop();
}

indexzero · 2011-04-25T04:32:41Z

This looks interesting. Need some time to examine it.

Now pop() can receive `amount`, so: - `logger.pop()` as well as `logger.pop(0)` equals to `logger.pop(1)` - `logger.pop(-1)` equals to `logger.pop().pop().pop()`, when `logger` had three contexts registered

ixti · 2011-04-25T13:14:13Z

I have improved test to be more visual. Also I have improved pop() method, so now it's possible to pop(2) to pop out two last contexts or even pop(-1) to pop out all contexts.

indexzero · 2011-04-25T13:46:59Z

Ok, lots of prep coming for NodeConf on our end. Will try to get this merged in before then.

ixti · 2011-04-25T14:28:47Z

Ok. Now it really works as expected ;))

rgabo · 2011-07-24T16:09:10Z

+1, also great for contextual information, such as associating user id / tracker with log statements for the same request.

e.g.

logger.push('user:42')

results in

info: [user:42] GET /foo
verbose: [user:42] stuff we do as part of request
info: [user:42] => 200 (0.52s)

this way it is easy to filter, even if concurrent requests mingle and have overlapping log lines. Today, we just pass { user: 42 } as metadata which is sub-optimal (line ends with user=42, but less readable)

ixti · 2012-01-31T10:57:15Z

This shoud be rewritten. I agree about push/pop is not the best naming. IMHO the API should be like this:

// sync style
var ctx_logger = logger.context('foobar');

// async style (if callback given)
logger.context('foobar', function (logger) {
  // logger is context'ed logger here
});

alampros · 2012-01-31T19:47:42Z

I actually like the push/pop syntax a lot more. Either one would be good, but there something missing from the context style...maybe a method to read the context var (in order to append to it)?

I'd love to start using this!

ixti · 2012-01-31T20:29:38Z

Well, probably you're right. Basically I'm not against any name :)) so even if it will be push/pop then, probably it will be even more nicer:

//
// no block style, modify current logger's context
//

logger.push('a');
logger.debug('foobar');
// -> debug: [a] foobar

logger.push('b');
logger.debug('foobar');
// -> debug: [a - b] foobar

logger.pop();
logger.debug('foobar');
// -> debug: [a] foobar

//
// with block style, clone logger with new context
//

logger.push('c', function (logger) {
  setTimeout(function () {
    logger.debug('foobar');
    // -> debug: [a - c] foobar
  }, 10000);
});

logger.debug('foobar');
// -> debug: [a] foobar

@indexzero if you're OK with proposal, I'll be glad to update refactor and resend pull request

indexzero · 2012-01-31T21:07:41Z

I'm not a fan of this API, but considering that I cant come up with a better naming convention and the value of the feature it makes sense. It should, however, be consistent with Javascript. It is not pushing and poping onto the logger it is unshifting and shifting

ixti · 2012-01-31T21:20:18Z

With all respect, I must disagree. We are pushing new value to the end of the context. And then poping one last value out.
So push(a), push(b) will give as ab, while unshift(a), unshift(b) should give ba. Context are used for easier debugging, so it's easier to red them from left to right - they follow deepness of the code.

Anyway this names were chosen not just because I like pushing'n'popping :)) but because this feature was "inspired" by log4j's nested diagnostic contexts (NDC): http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/NDC.html

puzrin · 2012-02-01T03:39:50Z

IMHO, shift/unshift is for arrays only - push/pop is more "natural" for loger object.

Anyway, nobody prohibits you to make alises, if methods name is a principal question.

ixti · 2012-02-01T13:49:23Z

This proposal adds a diagnostic context. Which in fact is some kind of array of breadcrumbs.

In context of method naming conventions:

we are shifting one element from the HEAD of the array
and unshifting into to the HEAD of an array
and we are pushing elements to the TAIL of the array
and poping them out from the TAIL later

Everytime we add new nested level - means we get deeper, so I really thing that push is more appropriate word for this situation. Let's say:

chooseCine() (NDC: 'home')
dressUp() (NDC: 'home')
getATaxi() (NDC: 'street')
- driveToTheCine() (NDC: 'street - taxi')
buyTickets() (NDC: 'cine')
- buyPopCorn() (NDC:'cine - lobby')
- startWatchingMovie() (NDC: 'cine - movie')
  - buyPopCorn() (NDC: 'cine - movie - lobby')

alampros · 2012-02-02T14:05:39Z

It occurred to me that this could take a cue from the webkit debugger commands console.group() and console.ungroup().

When it's not called directly from the console it looks a lot prettier - none of that undefined crap all over.

ixti · 2012-02-06T23:12:17Z

@indexzero can you please clarify your current position? Because I would like to fix my pull request in order to accelerate it's merge in ;)) as we wait almost one year now :))

indexzero · 2012-02-12T00:02:39Z

Sorry guys. Been swamped with work at Nodejitsu.

I don't like inconsistent semantics, but I agree that unshift and shift are somewhat difficult for new users to understand. How about this?

//
// no block style, modify current logger's context
//

logger.prefix('a');
logger.debug('foobar');
// -> debug: [a] foobar

logger.prefix('b');
logger.debug('foobar');
// -> debug: [a - b] foobar

logger.unprefix();
logger.debug('foobar');
// -> debug: [a] foobar

//
// with block style, clone logger with new context
//

logger.prefix('c', function (logger) {
  setTimeout(function () {
    logger.debug('foobar');
    // -> debug: [a - c] foobar
  }, 10000);
});

logger.debug('foobar');
// -> debug: [a] foobar

ixti · 2012-02-12T12:14:02Z

If it's a big deal to have prefix/unprefix (btw by it's nature it should be appendPrefix, removeLastPrefix IMHO) insted of push/pop, then I can deal with that. The only thing I would like to ask in this case is to have prefix/unprefix to be aliases of push/pop :))

I believe that we must think about programmers. And I believe 100% of programmers (at least those who wasn't jailed in visual programming world of D bemole) will easily recognize push/pop :)) So I really think that having push/pop with prefix/unprefix aliases to them will be the best way.

puzrin · 2012-02-12T12:24:51Z

group / ungroup, offered by alampros, is also nice.

ixti · 2012-02-22T11:20:49Z

@indexzero, please, implement the way you like (method names etc) so rest of us will be able to start using it and bind our own aliases in worst case.

puzrin · 2012-03-05T17:59:41Z

indexzeeeeroooooooo !!! we are missing you :)

kmpm · 2012-03-07T10:17:31Z

+1 to the functionality. Bring it on whatever you call it.
While we are discussing method names... tag / untag could be an option

indexzero · 2012-03-07T18:06:35Z

All, serious work at Nodejitsu is taking up my time this week. I know I owe some action here. I will get to it this weekend.

berstend · 2012-09-11T07:39:43Z

+1

jfhbrook · 2012-10-19T21:23:04Z

I'm sorry to do this, but I'm going to close this pull request unmerged because, well, it's a year old. Again: I'm sorry.

If you would like to try again against the current codebase, I'll do my very best to handle it in a timely manner.

dignifiedquire · 2012-10-21T09:26:07Z

@jesusabdullah I really need something like this, so I will implement it. But I would need to know what the preferred api is now, so it really gets merged and doesn't rot for a year.

indexzero · 2012-12-20T18:48:33Z

@dignifiedquire It should look like #208. @clintandrewhall is a hero

lobodpav · 2013-07-03T18:43:42Z

I know this issue was closed quite some time ago, but still - was this feature implemented somehow? This is a great idea with the prefixes I would love to use in my projects.

clintandrewhall · 2013-07-08T19:47:33Z

@lobodpav Check the previously-mentioned pull request...?

lobodpav · 2013-07-09T06:35:26Z

Well, I can do that, but this branch is quite an old one, isn't it? Was mainly wondering if this will ever became a part of a stable release.

ixti added 3 commits April 24, 2011 03:20

Adds push/pop context ability

62a87c4

Add withContext() method

d5ddc8a

withContext(context) creates new

Improve withContext() method

2d48c65

ixti added 2 commits April 25, 2011 15:09

Improved logger#pop().

8ef05fa

Now pop() can receive `amount`, so: - `logger.pop()` as well as `logger.pop(0)` equals to `logger.pop(1)` - `logger.pop(-1)` equals to `logger.pop().pop().pop()`, when `logger` had three contexts registered

Improve tests

17be4a1

Expose withContext, push and pop of default logger

7fa30bb

ixti added 2 commits April 25, 2011 15:53

Add test of withContext for defaultLogger

32b6aec

Fixed bug with no-context with level methods

90ca9c3

Remove redundant comma in tests

65a3779

indexzero mentioned this pull request Jan 23, 2012

Prefix option #82

Closed

jfhbrook closed this Oct 19, 2012

indexzero mentioned this pull request Dec 20, 2012

Add labels for multiple loggers who share the same transport. #208

Merged

This was referenced Jul 31, 2023

[Snyk] Upgrade triple-beam from 1.3.0 to 1.4.1 Redwoodtj/winston#4

Open

[Snyk] Upgrade @colors/colors from 1.5.0 to 1.6.0 Redwoodtj/winston#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

push/pop() context #6

push/pop() context #6

ixti commented Apr 24, 2011

ixti commented Apr 25, 2011

indexzero commented Apr 25, 2011

ixti commented Apr 25, 2011

indexzero commented Apr 25, 2011

ixti commented Apr 25, 2011

rgabo commented Jul 24, 2011

ixti commented Jan 31, 2012

alampros commented Jan 31, 2012

ixti commented Jan 31, 2012

indexzero commented Jan 31, 2012

ixti commented Jan 31, 2012

puzrin commented Feb 1, 2012

ixti commented Feb 1, 2012

alampros commented Feb 2, 2012

ixti commented Feb 6, 2012

indexzero commented Feb 12, 2012

ixti commented Feb 12, 2012

puzrin commented Feb 12, 2012

ixti commented Feb 22, 2012

puzrin commented Mar 5, 2012

kmpm commented Mar 7, 2012

indexzero commented Mar 7, 2012

berstend commented Sep 11, 2012

jfhbrook commented Oct 19, 2012

dignifiedquire commented Oct 21, 2012

indexzero commented Dec 20, 2012

lobodpav commented Jul 3, 2013

clintandrewhall commented Jul 8, 2013

lobodpav commented Jul 9, 2013

push/pop() context #6

push/pop() context #6

Conversation

ixti commented Apr 24, 2011

ixti commented Apr 25, 2011

indexzero commented Apr 25, 2011

ixti commented Apr 25, 2011

indexzero commented Apr 25, 2011

ixti commented Apr 25, 2011

rgabo commented Jul 24, 2011

ixti commented Jan 31, 2012

alampros commented Jan 31, 2012

ixti commented Jan 31, 2012

indexzero commented Jan 31, 2012

ixti commented Jan 31, 2012

puzrin commented Feb 1, 2012

ixti commented Feb 1, 2012

alampros commented Feb 2, 2012

ixti commented Feb 6, 2012

indexzero commented Feb 12, 2012

ixti commented Feb 12, 2012

puzrin commented Feb 12, 2012

ixti commented Feb 22, 2012

puzrin commented Mar 5, 2012

kmpm commented Mar 7, 2012

indexzero commented Mar 7, 2012

berstend commented Sep 11, 2012

jfhbrook commented Oct 19, 2012

dignifiedquire commented Oct 21, 2012

indexzero commented Dec 20, 2012

lobodpav commented Jul 3, 2013

clintandrewhall commented Jul 8, 2013

lobodpav commented Jul 9, 2013