Add optional dynamic debug instances (react on enable() and disable() once created) #209

ibc · 2015-07-02T21:09:18Z

First of all: this PR is 100% compliant with the current design and adds zero overhead to it. This is, no changes must be done in apps and libraries already using the debug module, and the performance is the same as before.

This PR adds "dynamic debug instances" which are created by passing true as the second parameter to the module exported function:

var debug = require('debug');
var applog = debug('myApp', true);

Those instances do properly react if debug.enable(xxxx) or debug.disable() is called after they are created.

An usage example is provided in the example/ folder.

The PR also resets exports.names and exports.skips when enable() or disable() is called (rationale here).

Related issues:

…disable() once created). Related issues: - #150 - #154

leftieFriele · 2015-08-07T09:15:56Z

+1 on this PR, turning on / off on the fly would be great for browser applications

ibc · 2015-08-28T13:36:27Z

@TooTallNate any comment about this?

bragma · 2015-12-09T10:29:35Z

+1 please merge this!

wision · 2016-01-06T12:09:38Z

@TooTallNate any chance of merging this feature?

techjeffharris · 2016-03-01T21:15:37Z

👍 Yes, please!

stephenmathieson · 2016-03-01T21:24:32Z

debug.js

-        index--;
+  // Return a dynamic debug instance.
+  else {
+    function dynamic() {


-1 for all this copypasta

What do you mean??

the dynamic function is nearly identical to the enabled function

Right. We can talk about code optimization if the PR is at least considered by the author, which said nothing yet about it.

roccomuso · 2016-05-17T11:27:47Z

Wouldn't it also be referred to #275 ?

thebigredgeek · 2016-11-14T17:29:54Z

@TooTallNate thoughts here? Not sure if we want to go this direction with the API

thebigredgeek · 2016-11-14T17:37:20Z

#156

thebigredgeek · 2016-11-14T17:43:43Z

Referenced issue #326

TooTallNate · 2016-11-16T16:53:12Z

A few comments:

I'm not a fan of the new API surface area. Why not all of them just be dynamic? And forget the ugly , true second argument to debug (boolean trap waiting to happen).
My PR in Node: configurable util.inspect() options #327 (specifically this commit f0e8a97) makes individual debug instances "hot-enableable" (that is, if you toggle the enabled flag on a debug instance it works as expected). Perhaps use that as a base instead of the "copy pasta".
You have to explicitly release dynamic debug instances otherwise exports.dynamics is a memory leak (plus it's an API, not change, but addition).

TooTallNate · 2016-11-16T16:54:23Z

Honestly that last point is the main reason a feature like this is not in debug right now. I don't want to have to maintain a list of all the debug instances in memory, which just grows and grows as more modules are required.

thebigredgeek · 2016-11-16T18:22:15Z

@TooTallNate agreed. Closing this for now.

tehsenaus · 2016-12-20T16:02:10Z

I don't see how this would cause a leak - the number of modules which can be require()'d is bounded, is it not?

ibc · 2016-12-20T16:09:24Z

I don't see how this would cause a leak - the number of modules which can be require()'d is bounded, is it not?

If you want to dynamicall enable/disable a debug instance you must store it somewhere. If you create tons of debug instances and never destroy them, they will in fact leak. Easy.

That's why in my PR I added a debug.release() method.

But, the current implementation of this module is too not handle debug instances. Instead they just exist while they are in the scope, and they are garbage collected if not like any variable.

tehsenaus · 2016-12-20T16:17:15Z

If you're creating debug instances on the fly and not releasing them, then sure. But is the library being used in that way? I'd bet 99.99% of cases users just require('debug')('blah:blah') at the top of their module. Then by the same logic, require() leaks memory!

This would be sooo useful for debugging in production, it seems a real shame to pass on for such an esoteric use case. If I were to wrap the library in userland, i'd only get the functionality for my application (not submodules like express).

ibc · 2016-12-20T16:27:52Z

If you're creating debug instances on the fly and not releasing them, then sure. But is the library being used in that way? I'd bet 99.99% of cases users just require('debug')('blah:blah') at the top of their module.

Sure. However sometimes I create dynamic debug instances in class constructors so their label include the id or whatever.

tehsenaus · 2016-12-20T17:32:01Z

That doesn't seem too efficient, it would be running lots of regex checks on your class instantiation?

Why not just log('%s: ...', this.id)?

thebigredgeek · 2016-12-20T20:06:34Z

The reason this leaks is because internal state within the debug lib is required. If you call debug over and over in it's current state it simply returns a function. This PR requires it to keep an ever growing amount of bookkeeping. A lot of people create debug instances in class constructors as mentioned before. This would be a breaking change that would manifest itself as a server crash once ram filled up. I get that it's useful, but that doesn't necessarily mean it's doable. There are always tradeoffs

ibc · 2016-12-21T11:01:06Z

That doesn't seem too efficient, it would be running lots of regex checks on your class instantiation?

I agree, but that's up to the app developer.

This PR requires it to keep an ever growing amount of bookkeeping.

Not true. This PR does NOT change the current/default behavior. If the user wants manageable debug instances it must create them by passing true as second parameter:

var dyndebug = require('debug')('myApp', true);

I understand that such an API (passing just true as second argument) is not too cool, but you got the idea.

tehsenaus · 2016-12-21T16:05:44Z

require()s are synchronous, right? So if one were to enable bookkeeping initially on startup, then disable bookkeeping in a process.nextTick() (so dynamic debug instances would not be tracked), this would achieve the desired effect with no leaks.

Food for thought...?

Othreumaru · 2016-12-21T16:33:03Z

I would argue with the API for enabling that feature, but I would definitely love to see it in next major release, enabled by default.

thebigredgeek · 2016-12-21T17:39:11Z

Check out the 3.0 conversation issue. It's something that will be added. However, I don't think we want to resort to hackery to make it work. We should probably just save enables/disabled state on a boolean within the factory closure as that won't leak

tehsenaus · 2017-01-10T22:25:50Z

I need this feature, so I created a module which enables it with no API changes:
https://github.com/tehsenaus/hot-debug

It patches the debug module, so any sub-dependencies of your project (e.g. express) will get the enable/disable functionality.

It'd be great to see this feature natively in debug v3!

Yalhu · 2018-04-13T07:12:04Z

There are some questions that I use debug on Browser.

Does debug.disable('debug:debug1') disable all the debug? On my test ,it was.
Does debug.enable() must have a argument. If there is no argument, it will disable debug.
If I disable debug on the script start up, on console debug.enable('debug:debug2') doesn't work. It will works again if I use debug.enable('debug:*'). (I use these API on chrome console).

steps

localStorage.setItem('debug','debug:*');
var d1=debug('debug:debug1');
var d2=debug('debug:debug2');
var d3=debug('debug:debug3');
debug.disable('debug:debug2'); // only debug2,but d1 not work
debug.enable('debug:debug1'); // debug1 not work
debug.enable('debug:debug3'); // only debug3 work
d1('test d1 ....');
d2('test d2 ...');
d3('test d3 ... ');

on chrome console:

only d3 workde, d1 didn't work.
do debug.enable('debug:debug1') d1('can work') worked.
do debug.enable('debug:debug2') d2('can work') did not work.
do debug.enable('debug:*') d2('can work') worked.
do debug.enable() d1('can work') d2('can work') no debug.

Add optional dynamic debug instances (react on calls to enable() and …

f107faa

…disable() once created). Related issues: - #150 - #154

ibc changed the title ~~Add optional dynamic debug instances which react on enable() and disable()~~ Add optional dynamic debug instances (react on enable() and disable() one created) Jul 2, 2015

ibc changed the title ~~Add optional dynamic debug instances (react on enable() and disable() one created)~~ Add optional dynamic debug instances (react on enable() and disable() once created) Jul 2, 2015

This was referenced Jul 3, 2015

debug module with dynamic behavior (enable/disable in runtime) expressjs/express#2698

Open

Introducing "dynamic mode" allowing enable/disable on the fly #156

Closed

ibc mentioned this pull request Nov 23, 2015

Enabling debuggin requires page reload #238

Closed

stephenmathieson reviewed Mar 1, 2016
View reviewed changes

thebigredgeek added discussion This issue is requesting comments and discussion change-major This proposes or provides a change that requires a major release feature This proposes or provides a feature or enhancement labels Nov 14, 2016

thebigredgeek added this to the 3.0 milestone Nov 14, 2016

thebigredgeek changed the base branch from master to v3 November 15, 2016 16:37

thebigredgeek closed this Nov 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional dynamic debug instances (react on enable() and disable() once created) #209

Add optional dynamic debug instances (react on enable() and disable() once created) #209

ibc commented Jul 2, 2015

leftieFriele commented Aug 7, 2015

ibc commented Aug 28, 2015

bragma commented Dec 9, 2015

wision commented Jan 6, 2016

techjeffharris commented Mar 1, 2016

stephenmathieson Mar 1, 2016

ibc Mar 1, 2016

stephenmathieson Mar 1, 2016

ibc Mar 1, 2016 •

edited

Loading

roccomuso commented May 17, 2016 •

edited

Loading

thebigredgeek commented Nov 14, 2016

thebigredgeek commented Nov 14, 2016

thebigredgeek commented Nov 14, 2016 •

edited

Loading

TooTallNate commented Nov 16, 2016

TooTallNate commented Nov 16, 2016

thebigredgeek commented Nov 16, 2016

tehsenaus commented Dec 20, 2016

ibc commented Dec 20, 2016

tehsenaus commented Dec 20, 2016

ibc commented Dec 20, 2016

tehsenaus commented Dec 20, 2016

thebigredgeek commented Dec 20, 2016

ibc commented Dec 21, 2016

tehsenaus commented Dec 21, 2016

Othreumaru commented Dec 21, 2016

thebigredgeek commented Dec 21, 2016

tehsenaus commented Jan 10, 2017

Yalhu commented Apr 13, 2018 •

edited

Loading

Add optional dynamic debug instances (react on enable() and disable() once created) #209

Add optional dynamic debug instances (react on enable() and disable() once created) #209

Conversation

ibc commented Jul 2, 2015

leftieFriele commented Aug 7, 2015

ibc commented Aug 28, 2015

bragma commented Dec 9, 2015

wision commented Jan 6, 2016

techjeffharris commented Mar 1, 2016

stephenmathieson Mar 1, 2016

Choose a reason for hiding this comment

ibc Mar 1, 2016

Choose a reason for hiding this comment

stephenmathieson Mar 1, 2016

Choose a reason for hiding this comment

ibc Mar 1, 2016 • edited Loading

Choose a reason for hiding this comment

roccomuso commented May 17, 2016 • edited Loading

thebigredgeek commented Nov 14, 2016

thebigredgeek commented Nov 14, 2016

thebigredgeek commented Nov 14, 2016 • edited Loading

TooTallNate commented Nov 16, 2016

TooTallNate commented Nov 16, 2016

thebigredgeek commented Nov 16, 2016

tehsenaus commented Dec 20, 2016

ibc commented Dec 20, 2016

tehsenaus commented Dec 20, 2016

ibc commented Dec 20, 2016

tehsenaus commented Dec 20, 2016

thebigredgeek commented Dec 20, 2016

ibc commented Dec 21, 2016

tehsenaus commented Dec 21, 2016

Othreumaru commented Dec 21, 2016

thebigredgeek commented Dec 21, 2016

tehsenaus commented Jan 10, 2017

Yalhu commented Apr 13, 2018 • edited Loading

steps

ibc Mar 1, 2016 •

edited

Loading

roccomuso commented May 17, 2016 •

edited

Loading

thebigredgeek commented Nov 14, 2016 •

edited

Loading

Yalhu commented Apr 13, 2018 •

edited

Loading