pinojs / pino

🌲 super fast, all natural json logger
http://getpino.io
MIT License
14.21k stars 875 forks source link

print caller name for debugging purpose #191

Closed phra closed 7 years ago

phra commented 7 years ago

hi,

it would be nice to have the ability to print the caller function name of the logging functions in order to easily remove useless log statements when you don't know where they are located in big projects. obvioulsy this feature will kill the awesome performance of pino so it MUST be disabled by default.

i will open a PR with the proposed feature in order to discuss about it.

davidmarkclements commented 7 years ago

By logging function name you mean the log label (i.e. info) rather than just the log level number?

If that's the case this can be done with a transport (for debugging the pino prettifier could have this added).

If you mean the previous call site, as in the function in which a log function is called, that's not always possible - e.g. in strict mode arguments.callee isn't available - in node and chrome you can use Error.prepareStackTrace capture each ticks call stack and figure out the calling function

phra commented 7 years ago

@davidmarkclements as you can see i've implemented it in a way that is also working in strict mode thanks to stacktrace.js the only pitfall is that this will kill the performance of the logger itself, so it must be used only in development environment.

SPOILER: the stacktrace is generated by throwing and catching a fake error. (in this way it also works in strict mode)

davidmarkclements commented 7 years ago

to be implemented as plugin - see #192

phra commented 7 years ago

WIP @ https://github.com/phra/pino-caller

jsumners commented 7 years ago

I'm going to take this opportunity to voice my opposition to projects being part of the org if they are transpiled.

mcollina commented 7 years ago

I agree @jsumners.

davidmarkclements commented 7 years ago

Agreed - I've discussed reasons for this before here https://github.com/pinojs/pino/issues/109#issuecomment-260207532

Also this https://github.com/pinojs/pino/issues/109#issuecomment-260215716

phra commented 7 years ago

@jsumners @mcollina @davidmarkclements I've refactored the plugin to vanilla javascript. (https://github.com/phra/pino-caller) I still have to publish it on NPM, waiting for your approval.

davidmarkclements commented 7 years ago

@phra this is looking nice, couple of things

  1. in the readme can you put the warning on it's own line
  2. dinamically -> dynamically
  3. can you apply http://npm.im/standard linting to all code (inc. tests + examples)
  4. pino-caller is a plugin for pino that let you print also the calling site of the logging functions. -> pino-caller is a wrapper for pino which adds the call site of each log message to the log output.
  5. How do you feel about changing the name to pino-callsite ?
phra commented 7 years ago

@davidmarkclements i will do for sure 1-4. regarding the fifth point, i've already set up travis and coveralls with the actual name.. is it really required to change the package name? i think that caller is a nice keyword to make clear what this plugin is doing. (because it's recalling the caller property of arguments.)

davidmarkclements commented 7 years ago

no it's your module, the name is up to you :)

for me though - pino-caller is ambiguous - it makes me think this a module that calls pino in some way

phra commented 7 years ago

@davidmarkclements 1-4 are done. let me know if there is something else that i can do.

davidmarkclements commented 7 years ago

nope I'm happy - @mcollina @jsumners do you want to comment here - thoughts on bringing into pino org etc.

mcollina commented 7 years ago

I'm 👍 if @phra would like to bring this to the org.

phra commented 7 years ago

@mcollina it's fine to me. what do i have to do?

jsumners commented 7 years ago

What happens if you change the logger level after wrapping pino with pino-caller?

davidmarkclements commented 7 years ago

@phra can you add a test per @jsumners question? Then we'd love to bring pino-caller into the org if you're amenable?

phra commented 7 years ago

@davidmarkclements sorry, i was busy. i'm trying right now to change the log level and something strange is happening. i need to investigate the source code in order to fix it. i will let you know something soon.

phra commented 7 years ago

the problem is this line it changes the function pointer at runtime so it's restoring the original one. i'm thinking how to fix it.

phra commented 7 years ago

@davidmarkclements i've submitted #198 in order to fix it. basically now pino keeps a copy of the previous log function when switching from noop and viceversa. in this way if the function has been altered, it will continue to work in the intended way. obviously with this change also other plugins can beneficiate from this.

do you think that this is a proper fix/feature?

@mcollina @jsumners

phra commented 7 years ago

see https://github.com/pinojs/pino/pull/198#issuecomment-283800236 for updates.

phra commented 7 years ago

@davidmarkclements @mcollina @jsumners i think that pino-caller is finally ready to be published! do you want to publish it under the pinojs org or i have to publish it under my account?

jsumners commented 7 years ago

You would publish it to npm under your account since you are the "lead maintainer." But you can migrate the repository to the org if that is your desire.

phra commented 7 years ago

@jsumners the package has been published on npm registry

i've also tried to transfer the ownership of the repository to pinojs org but it tells me: You don’t have the permission to create repositories on pinojs

mcollina commented 7 years ago

@phra I've just sent you an invitation into the org.

phra commented 7 years ago

done

thanks, @mcollina

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.