Configure newrelic using custom config object (discussion)

[joscarsson]

I'm opening this pull request mainly to start a discussion. I could have opened an issue, but in this case I think it is more constructive to actually provide some proof-of-concept code for what I am after.

So, in our application we have multiple processes/servers running from the same source code tree (so there will be several app.js run using different executions of the node binary, but only one node_modules folder). That means placing a newrelic.js in the root of the application is not "good enough" in our case, because we want to provide different application names for the different applications.

In node-newrelic/lib/config.js the following code block is found:

var CONFIG_FILE_LOCATIONS = [
  process.cwd(),
  process.env.NEW_RELIC_HOME,
  process.env.HOME,
  path.join(__dirname, '..', '..', '..') // above node_modules
];

That means we could make sure our node executions have their working directory set to where app.js resides, and place one newrelic.js at each of these locations (because of the process.cwd() above). Although I guess it will work, I haven't tried it, because it still does not satisfy all our requirements. We also want to be able to change app_name depending on which environment we run on (for example, stage environment should report to App Name (Stage) whereas production should report to App Name).

So what we are actually after is to be able to pass our own config object into newrelic directly, without using the newrelic.js file or environment variables. This gives several advantages:

1. We can utilize our existing configuration object which already have support for environment specific settings and so on, and just add a new subobject for newrelic settings.
2. We can dynamically during runtime edit the configuration before we pass it into newrelic (not after, but before).
3. We won't need any new configuration files on the file system.

In our app.js, it will look like this:

var ourAppConfig = require('./ourAppConfig');
var newrelic = require('newrelic/bootstrap')(ourAppConfig.newrelic);

The commits in this pull request enable this functionality. It wraps the code in node-newrelic/index.js in a function in node-newrelic/bootstrap.js. The index.js directly calls this new bootstrap.js function with an empty argument, meaning it will not use a custom configuration object so existing code will continue to function without changes.

You actually had this somewhat prepared in node-newrelic/lib/config.js:

function initialize(config) {
  // removed code for display purposes

  if (config) return new Config(config);
  // more removed code for display purposes

and

/**
 * Preserve the legacy initializer, but also allow consumers to manage their
 * own configuration if they choose.
 */
Config.initialize = initialize;

The code in the new bootstrap.js utilizes this initialize(config) function.

The problem with this solution is the circular dependency between logger and config. To get this to work, I had to disable the require() of config in logger. This dependency seems a bit strange as you have noticed yourselves too, in node-newrelic/lib/config.js:

/**
 * Because this module and logger depend on each other, the logger needs
 * a way to inject the actual logger instance once it's constructed.
 * It's kind of a Rube Goldberg device, but it works.
 *
 * @param {Logger} bootstrapped The actual, configured logger.
 */
Config.prototype.setLogger = function setLogger(bootstrapped) {

And this comment in node-newrelic/index.js (bootstrap.js in this pull request) is actually not correct any longer:

    /* Loading the configuration can throw if a configuration file isn't found and
     * the environment variable NEW_RELIC_NO_CONFIG_FILE isn't set.
     */
    var config = require(path.join(__dirname, 'lib', 'config.js')).initialize(customConfig);

The row above will never throw, as it will already have thrown when requiring the logger (as that depends on config) on the first line in the file logger = require(path.join(__dirname, 'lib', 'logger.js')). Is this intended?

If the circular dependency was removed somehow, the pull request would pretty much work as is. As I understand it, the "only" thing configured in logger from config is loglevel and filepath if logging to file. Perhaps there could be defaults for this instead, or only allow these options to be configured from environment variables? Anything removing the dependency on config would work.

Or am I doing this whole thing wrong? Is there an easy solution for what I am after? If not, are you planning to support anything similar to the above? We really like newrelic, and this is the last piece for us to be able to use it in this system.

[joscarsson]

Ok, so we found a solution which will satisfy all our requirements without any changes to node-newrelic. Pseudo-code for what we're doing:

var ourAppConfig = require('./ourAppConfig');

process.env.NEW_RELIC_NO_CONFIG_FILE = true;
process.env.NEW_RELIC_LICENSE_KEY = ourAppConfig.newRelicLicenseKey;
process.env.NEW_RELIC_APP_NAME = ourAppConfig.newRelicAppName;

var newrelic = require('newrelic');

Simple and clean.

[freewil]

I wouldn't call overriding environment variables from within your app clean. I agree that you should be able to initialize newrelic by requiring it with an options object.

[joscarsson]

Ok, I agree, I'll reopen so we can discuss it.

[ruimarinho]

Agreed. Bootstrapping from a config object would be ideal. I'm currently using a config module that allows me to configure options per environment, so I'd just use that to feed options to the newrelic module. The alternatives work, but are cumbersome :)

[groundwater]

I don't think it's unreasonable to pass a config object directly.

There are a few things to consider:

1. make sure users know they cannot require('fs') to load the config, because that will break instrumentation
2. make sure this plays well with the other ways to set config values

I've put this as a feature request for now. It's reasonable, but I don't have an estimate yet for when we will integrate this.

[joscarsson]

Thanks @groundwater.

Regarding your point 1, this is only true if the same fs instance is used after newrelic has been required, right? We are actually requiring fs in ourAppConfig.js to load some configurations options from file, but that fs instance is never disclosed outside ourAppConfig.js. The next time fs is required, newrelic has been required and should be able to instrument.

[othiym23]

@joscarsson Unfortunately, that's not how Node's module cache works. In order for New Relic's low-level shims to work properly, they have to be loaded before any of the core modules that have asynchronous functions are required anywhere in your application. This is why New Relic's documentation is so insistent about require('newrelic') being the first thing that gets run. Synchronous calls to set properties on process.ENV are safe, but in general, requiring core modules or processing configuration asynchronously are not.

[joscarsson]

@othiym23 Thanks for explaining. We are not processing configuration asynchronously (we use the readFileSync() on fs), but we are requiring core modules. When I look at https://github.com/newrelic/node-newrelic/tree/master/lib/instrumentation/core there does not seem to be any instrumentation for the fs module though. New Relic seems to be working fine, but maybe we are missing some statistics or it will crash later on. We'll see :)

[othiym23]

@joscarsson The issue is not with New Relic for Node itself, but with the async-listener shim that continuation-local-storage uses to observe async calls. Because it dynamically monkeypatches modules out of core, things should just mostly work, but you're putting yourself into a partially undefined state, which is at least part of the reason why New Relic's configuration works the way it does.

[othiym23]

For more context on why New Relic for Node's configuration works this way, see issues #5, #20, #27, #52, and #81. I no longer work on New Relic for Node (or at New Relic, for that matter), so I'm just here to be helpful (and maybe stick up for my own prior design decisions), but I will say that I'm not sure that bikeshedding over how the module is configured is productive. Using process.env is kinda ugly / inconvenient, but between that and the configuration file, pretty much every use case of configuring the agent is covered.

[tgwizard]

So @othiym23, if the first thing you do is to require continuation-local-storage (or async-listener), everything would work? The shimming and so would be in place, then you'd do fs.readFileSync to get the New Relic configuration etc., then you'd initialize New Relic.

This would require the app to have continuation-local-storage / async-listener as dependencies. But if they do for other reasons, it would work fine to configure New Relic this way, right?

[othiym23]

@tgwizard You could, but that doesn't mean it's a good idea. If you're willing to take on the project of learning how all this stuff interacts (which, IMO, diminishes the value of New Relic as a black box that you add to your app to give you insight into how it's performing), you can certainly try whatever you want. However, that's going to make it harder for New Relic to support you, and may also affect your ability to trust the metrics you get out of it. Setting a few environment variables and sticking require('newrelic') at the top of your app's main file seems a lot simpler to me.

[tgwizard]

@othiym23 I agree with you. It is probably not a best solution for most applications. Seeing New Relic as a black box can be appropriate for some apps, and when configuration can easily and securely be set through environmentvariables externally to the app, or through the newrelic.js file, that would be best.

But for those apps that have other configuration setups, perhaps running separate processes and apps from the same source, and actually are using continuation-local-storage for other purposes, I'd say it could be suitable.

Still, three ways to configure the same things is probably not good or worth it. Environment variables and the newrelic.js file suffice. And given that breaking backwards compatibility isn't much fun, it wouldn't perhaps give that much to now replace newrelic.js with explicitly initializing New Relic.

[wraithan]

Instead of having a single newrelic.js at the root of your application. You could have a folder for each of your apps that has a newrelic.js in it and use the environment variable NEW_RELIC_HOME to tell the newrelic module where to look.

If there is enough want/need I can put in a feature request to be able to use an envvar to specify the file exactly instead of just pointing at a directory with a newrelic.js in it.

I am generally opposed to passing in configuration to the agent, as the vast majority of cases can be handled via ENV or via the fact that newrelic.js is JavaScript, not JSON. Also, most importantly it muddles our messaging that nothing should happen before your require('newrelic'). Even worse is it could encourage people requiring db modules or http modules to grab config values, modules that we must be required before in order to instrument.

[txase]

This issue has been open for a few weeks now without any further feedback. We feel this PR isn't the best way to configure the agent, and there are alternative mechanisms for solving this issue. I am going to close this issue out now.

If, as @wraithan mentioned, anyone feels there is a need for specifying which configuration file to use instead of defaulting to newrelic.js or using the NEW_RELIC_HOME env var, please speak up!

Thanks!

[joscarsson]

Sorry for not responding back. As I wrote in my second post in this PR we found a solution that works for us, but I reopened so we could have the discussion.

Thanks to everyone commenting and participating!