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

Can't set global log() function overriding namespaces ones #873

Open
piranna opened this issue Mar 4, 2022 · 10 comments
Open

Can't set global log() function overriding namespaces ones #873

piranna opened this issue Mar 4, 2022 · 10 comments
Labels
bug This issue identifies a malfunction change-patch This proposes or provides a change that requires a patch release needs-documentation This issue or change requires additional documentation pr-welcome This issue has an approved change; a pull request would be appreciated

Comments

@piranna
Copy link

piranna commented Mar 4, 2022

On the example located at https://github.com/debug-js/debug#output-streams, it shows that's possible to set custom log() functions for each namespace, but also that setting it directly on the debug module takes precedence and will be used instead of the namespace ones. This is specially useful in my use case, since I want to capture Mediasoup debuging Logger and redirect it to Moleculer logger. Problem is that according to

debug/src/common.js

Lines 112 to 113 in c0805cc

const logFn = self.log || createDebug.log;
logFn.apply(self, args);
namespaces log() function has absolute priority, so it's not possible to do it globally, and it's not possible to directly bind on top of Mediasoup Loggers since they are private, so I'm in an end road.

Since the linken example at https://github.com/debug-js/debug/blob/master/examples/node/stdout.js doesn't exists too, I'm not sure if the documentation is outdated or wrong, or if setting debug.log() should effectively override per-namespace log() function and this is in fact a bug. Besides fixing this one way or another, how can I solve my problem of redirect Mediasoup log messages to Moleculer?

@Qix-
Copy link
Member

Qix- commented Mar 4, 2022

Yes but self.log shouldn't be set to anything.

Welcome to Node.js v17.3.1.
Type ".help" for more information.
> const debug = require('debug');
undefined
> debug.log
[Function: log]
> d1 = debug('foo:bar')
[Function: debug] {
  namespace: 'foo:bar',
  useColors: true,
  color: 4,
  extend: [Function: extend],
  destroy: [Function: deprecated],
  enabled: [Getter/Setter],
  inspectOpts: {}
}
> d1.log
undefined
> d1.enabled = true
true
> d1('test')
  foo:bar test +0ms
undefined
> debug.log = (...args) => console.error('MY DEBUG:', ...args);
[Function (anonymous)]
> d1('test')
MY DEBUG:   foo:bar test +24s
undefined

This works for me on the latest version. Can you reproduce that locally?

@Qix- Qix- added the question This issue asks a question or requests usage support label Mar 4, 2022
@piranna
Copy link
Author

piranna commented Mar 4, 2022

This works for me on the latest version.

It's not the same case here, that works because d1.log is undefined, but at Mediasoup https://github.com/versatica/mediasoup/blob/4783b3dde93d43b5b120a8088af873aa5661202f/node/src/Logger.ts#L27-L29 d1.log is set to a function, the same as what the example in the docs says:

debug/README.md

Lines 254 to 272 in c0805cc

```js
var debug = require('debug');
var error = debug('app:error');
// by default stderr is used
error('goes to stderr!');
var log = debug('app:log');
// set this namespace to log via console.log
log.log = console.log.bind(console); // don't forget to bind to console!
log('goes to stdout');
error('still goes to stderr!');
// set all output to go via console.info
// overrides all per-namespace log settings
debug.log = console.info.bind(console);
error('now goes to stdout via console.info');
log('still goes to stdout, but via console.info now');
```

With d1.log set to a function as debug documentation example does (and also Mediasoup does too), it fails.

@piranna
Copy link
Author

piranna commented Mar 8, 2022

How can I help you with that? It's tagged as question, but it's also a bug on docs and/or actual implementation. How can we move this forward?

@Qix-
Copy link
Member

Qix- commented Mar 8, 2022

I'm not sure what you want. If you specify a logger on individual debug instances, of course it will override the global logger. That's well defined.

What behavior would you like to see happen?

@piranna
Copy link
Author

piranna commented Mar 8, 2022

What behavior would you like to see happen?

According to the docs example, if you define a logger in debug instance, it will override all the ones from its parent debug instances, but if you define a global logger, it will override all the debug instances. The example in the docs is very explicit about this behaviour:

debug/README.md

Lines 254 to 272 in c0805cc

```js
var debug = require('debug');
var error = debug('app:error');
// by default stderr is used
error('goes to stderr!');
var log = debug('app:log');
// set this namespace to log via console.log
log.log = console.log.bind(console); // don't forget to bind to console!
log('goes to stdout');
error('still goes to stderr!');
// set all output to go via console.info
// overrides all per-namespace log settings
debug.log = console.info.bind(console);
error('now goes to stdout via console.info');
log('still goes to stdout, but via console.info now');
```

@Qix-
Copy link
Member

Qix- commented Mar 8, 2022

You're right, GitHub has a scrollbar on mobile and I missed it in your other comment. My apologies.

PR welcome to fix the docs. There isn't a great way to fix this that won't break a bunch of people unfortunately. I need to think about how to approach this for v5.

@Qix- Qix- added bug This issue identifies a malfunction change-patch This proposes or provides a change that requires a patch release needs-documentation This issue or change requires additional documentation pr-welcome This issue has an approved change; a pull request would be appreciated and removed question This issue asks a question or requests usage support labels Mar 8, 2022
@piranna
Copy link
Author

piranna commented Mar 8, 2022

Thing is, it seems like this was the previous behaviour, but can't find the actual reference.

@shryao1

This comment was marked as spam.

@RabiaSaid
Copy link

2.6.8

@RabiaSaid
Copy link

4.3.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue identifies a malfunction change-patch This proposes or provides a change that requires a patch release needs-documentation This issue or change requires additional documentation pr-welcome This issue has an approved change; a pull request would be appreciated
Development

No branches or pull requests

4 participants