Debug API

[Pessimistress]

An alternative approach of #3949

Change List

• Pull all logging into a central location
• Do not bundle the default logging functions in production mode
• Allow logging functions to be loaded at runtime

[coveralls]

Coverage decreased (-0.04%) to 83.708% when pulling 40d4c0e on x/debug-api into 9225d45 on master.

[tsherif]

I really like the idea of a centralized "debug" object, and I'm wondering if this approach can be combined with the other one, i.e. remove any references to the debug object completely in production code. This could allow us to use it for more than removing logs, e.g.:

// Run an expensive assertion
deck.debug.run(() => {
    assert(gl.getError() === gl.OK, "No errors");
});

// Instrument a method.
deck.debug.run(() => {
    const oldDrawLayer = Layer.protototype.drawLayer;
    Layer.protototype.drawLayer = function (...args) {
		// Logging, profiling, etc.
        oldDrawLayer.apply(this, args);
        // More logging profiling, etc.
    };
});

[tsherif]

I'm thinking deck.debug(true) should set a minimum log priority to get logging so this can be done in one step (with the user having the option to set the priority higher). And maybe the log priority could be an optional second argument to deck.debug.

[ibgreen]

I'm thinking deck.debug(true) should set a minimum log priority to get logging

Agree that we should make this easy for user. This logging system has always had a discovery problem - how many deck.gl users are aware of it and able to leverage it?

For reference, the probe.gl Log.enable() etc returns this for chaining. deck.log.enable().setPriority(3) etc. It is a reasonable design, but the user needs to know that those methods exist.

[Pessimistress]

An easy solution to this is to set the default priority of deck.log to a non-zero number.

If we want to address this usability issue in probe.gl, I would set the priority default to 1. When log is disabled only priority=0 messages will pass through. Since enabled is default to false, the logging behavior will remain the same for most users who haven't messed with it.

[ibgreen]

An easy solution to this is to set the default priority of deck.log to a non-zero number.

Yes. The system is probably not optimal. I would not be opposed to changes. They could be small as you suggest but here is some additional grist for the mill:

• We have both enable and priority, maybe we don't need both? Initial probe.gl users (a long time ago) had concerns about perf impact when not in use, and the idea was that enable(false) would replace all methods with empty functions, not a bad idea though that feature generated surprising amounts of errors during probe.gl maintenance, not sure it was worth it.

Another question is whether enable/disable should affect log.warn and log.error as we now rely on those for non-debugging cases. If disable does not defeat those, it does not really so what it sounds like, and if it does, enable should probably be true by default as I assume we do want such messages...

Another criticism on the probe.gl API that was often raised: The term priority was considered very confusing since higher values are less likely to be logged. Most logging framework calls this value "log level", and it would make perfect sense to align with that convention.

[ibgreen]

I really like the idea of a centralized "debug" object, and I'm wondering if this approach can be combined with the other one, i.e. remove any references to the debug object completely in production code. This could allow us to use it for more than removing logs, e.g.:

Yes it is a nice idea. In fact the pre-cursor version of probe.gl Log class (non-open-source) had exactly this method (used the same priority system as the log statements). It wasn't used much so it doesn't seem to have made it into the open source version, but certainly easy to add on either level.

[ibgreen]

Aren't these pretty important? Assuming they are covered somewhere else?

[Pessimistress]

They are covered in AttributeManager.

[ibgreen]

Assume the new system should be nicely testable by injecting test assertions instead of log functions? Assume that can be improved especially if this becomes the foundation of a plugin system.

[ibgreen]

I'm thinking deck.debug(true) should set a minimum log priority to get logging

Agree that we should make this easy for user. This logging system has always had a discovery problem - how many deck.gl users are aware of it and able to leverage it?

For reference, the probe.gl Log.enable() etc returns this for chaining. deck.log.enable().setPriority(3) etc. It is a reasonable design, but the user needs to know that those methods exist.

[ibgreen]

Absolutely delighted to see that you went ahead and did this exploration!

I was hoping (at best) that my running commentary on debug support would slightly alter the future trajectory of development, I did not expect that it could be implemented already in v8, this is much appreciated.

All comments are nits, no concerns:

I don't understand the require/import strategy here, but I am fine with any approach you choose. That said:

Some more comments especially around anyrequire statements would probably help as I consider use of require to be a slightly "hacky" and fragile part of the setup (e.g. I seem to recall mixing require and import caused pains for some bundlers and was to be avoided).

E.g. the comments say that logging is not bundled, but my sense is that require does not prevent bundling, it just prevents execution, which saves little. Maybe add a comment above this if statement explaining how the bundling config actually strips this file? - I don't seem to find it in this PR...

if (process.env.NODE_ENV !== 'production') {
  loggers = require('./loggers').getLoggers(log); // This does not stop webpack from bundling right?
}

And the below destructuring is pretty jarring to my eyes and raises questions every time I see it:

import debug from '../../debug';
const debugLog = debug.log;

FWIW, I was able to avoid such hacks in loaders.gl for conditionally bundled (node only) files using some techniques but they may not apply here.

[Pessimistress]

my sense is that require does not prevent bundling, it just prevents execution

Correct, although it will be stripped during compression. This is the entry point of React:

if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react.production.min.js');
} else {
  module.exports = require('./cjs/react.development.js');
}

I had some offline discussion with @tsherif. We will merge the debug utility into probe.gl's Log class:

• It will allow us to get rid of the manual "syncing" of debug.enable and log.enable
• In the core classes we'll call something like log.debug(eventType, ...) instead
• It will allow us to optionally strip the debug calls using the babel plugin should we choose to do so

[ibgreen]

Nice, moving this to probe.gl makes a lot sense if done in a generic and well-documented way.

Correct, although it will be stripped during compression. This is the entry point of React:

Got it. Long term perhaps it can be loaded dynamically when needed instead of always included and then stripped. (The loading just needs to survive page reloads to enable logging of startup code but that is just another reason to move the debug system to probe.gl which already manages that).

In the core classes we'll call something like log.debug(eventType, ...) instead

For my understanding log.warn and log.error will be unaffected right? This is not a wholesale unbundling of the log class in production, just the debug calls?