Modification of process.env.LOG_LEVEL breaks application

[apricote]

We use the LOG_LEVEL environment variable to configure log4js and pass it values like LOG_LEVEL=trace to enable logging at a specific level.

Currently the Splunk SDK just overwrites our custom levels, so inside the application this shows up:

console.log(process.env.LOG_LEVEL) // Outputs '1'

As LOG_LEVEL seems to be used by multiple application developers (Blog Post), the SDK should IMO either not change the value, or use some prefixed environment variable (SPLUNK_LOG_LEVEL, SPLUNK_SDK_LOG_LEVEL).

[jtiscione]

I landed here for the same reason. We have a set of logging infrastructure tools that read LOG_LEVEL and use a default value if the variable is undefined. Something kept defining the variable. It took us a while to realize that it's the Splunk driver setting up its own little logging system, consisting of a wrapper around console.log. (Nothing actually uses it except for the module's own unit tests.) During initialization if LOG_LEVEL is not set, it clobbers it with a value of "1" which gets misinterpreted by more serious logging stuff later.

It's pretty strange that a driver module (of all things) would just assume a variable called "LOG_LEVEL" is not being used anywhere else in a node application (of all things) and proceed to write to it instead of simply recording its absence in a local variable. If you're going to abuse process.env as a global variable namespace for your module, at least write to environment variables beginning with "SPLUNK_" instead of begging for a name collision. Any node module that writes to process.env should mention it in the documentation.

OK, rant over. One workaround is to replace "require('splunk-sdk')" with a require of this module below:

const prior = process.env.LOG_LEVEL;
const raw = require('splunk-sdk');
// undo damage
if (prior === undefined) {
  delete process.env.LOG_LEVEL;
} else {
  process.env.LOG_LEVEL = prior;
}
module.exports = raw;

[KatSick]

Same crash for me

[apricote]

@KatSick Where do you see the connection to the reported bug?

For me this bug does not result in a crash or a TypeError, just unexpected, modified content in the process.env.LOG_LEVEL environment variable.

[fantavlik]

Hi all, we are re-engaging with the SDK and coming back to this issue. Let me propose a change along the suggest lines of using SPLUNK_LOG_LEVEL as suggested and please provide feedback if you are still following along:

In order to support this as a non-breaking change I'd propose changing these lines from:

    // Normalize the value of the environment variable $LOG_LEVEL to
    // an integer (look up named levels like "ERROR" in levels above),
    // and default to "ERROR" if there is no value or an invalid value
    // set.
    var setLevel = function(level) {    
        if (utils.isString(level) && levels.hasOwnProperty(level)) {
            process.env.LOG_LEVEL = levels[level];
        } 
        else if (!isNaN(parseInt(level, 10)) &&
                   utils.keyOf(parseInt(level, 10), levels)) {
            process.env.LOG_LEVEL = level;
        } 
        else {
            process.env.LOG_LEVEL = levels["ERROR"];                
        }
    };

    if (process.env.LOG_LEVEL) {
        setLevel(process.env.LOG_LEVEL);
    } 
    else {
        process.env.LOG_LEVEL = levels["ERROR"];
    }

to:

    // Normalize the value of the environment variable $SPLUNK_LOG_LEVEL to
    // an integer (look up named levels like "ERROR" in levels above),
    // and default to "ERROR" if there is no value or an invalid value
    // set.
    var setLevel = function(level) {    
        if (utils.isString(level) && levels.hasOwnProperty(level)) {
            process.env.SPLUNK_LOG_LEVEL = levels[level];
        } 
        else if (!isNaN(parseInt(level, 10)) &&
                   utils.keyOf(parseInt(level, 10), levels)) {
            process.env.SPLUNK_LOG_LEVEL = level;
        } 
        else {
            process.env.SPLUNK_LOG_LEVEL = levels["ERROR"];                
        }
    };

    if (process.env.SPLUNK_LOG_LEVEL) {
        setLevel(process.env.SPLUNK_LOG_LEVEL);
    } 
    else if (process.env.LOG_LEVEL) { // still read from LOG_LEVEL for backwards compatibility
        setLevel(process.env.LOG_LEVEL);
    } 
    else {
        process.env.SPLUNK_LOG_LEVEL = levels["ERROR"];
    }

Does this approach address your concerns? The only tweak here is to not break existing users that may be setting LOG_LEVEL intentionally with the expectation that the SDK will read from there. We also need to call this out explicitly in the README.md when we take this work up.

[ncanumalla-splunk]

Closing this issue as there is no further response. We can reconsider the above suggested change if there is interest in this issue again.