Repository: honeycombio/beeline-nodejs
Pull request #249 (2020-09-17 19:55:55)

early return if the beeline has already been configured
Repeated calls to the toplevel configure function can yield errors like this:

```
The following modules were required before honeycomb-beeline: express
These modules will not be instrumented.  Please ensure honeycomb-beeline is required first.
```

This is because the toplevel configure function calls all the sub-configure functions, including our `instrumentation/` configure which checks for already-loaded modules, causing the warning above.

The function is meant to be idempotent, so early return if we're already configured.  We do, however, need a warning in this case as well - if the beeline configure call is passed arguments we should let the user know they aren't going to affect anything.

so, after this change:

```
const beeline = require("honeycomb-beeline");
beeline({ ... });
beeline({ ... }); // => "Beeline is already configured.  Further calls are allowed but should not be passed configuration options (as they will be ignored)."
```

warning feels kinda wordy, but /shrug

toshok: the assumption is that you call the function to initialize the beeline:

```
const beeline = require("honeycomb-beeline")({ configArgs: here });

beeline.startTrace(...);
```

once this has been done, you can use the api without making the call:

```
const beeline = require("honeycomb-beeline");

beeline.startSpan(...);
```

I think a combination of docs being unclear and folks instrumenting, then c&p the beeline require call means we end up with a lot of calls to the configure function.  I expect that most of the time, these are without args and harmless.

The beeline will pull the dataset/writekey from the environment, so there's no need to pass config params most of the time.  That's why it's usually empty.

Patch:
diff --git a/lib/index.js b/lib/index.js
index 6416ed7..7859293 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -3,10 +3,35 @@ const api = require("./api"),
   instrumentation = require("./instrumentation"),
   propagation = require("./propagation");
 
-function configure(opts = {}) {
+let configured;
+
+function checkIfConfigureNeeded(opts) {
+  if (!configured) {
+    return true;
+  }
+
+  if (opts) {
+    console.warn(
+      "Beeline is already configured.  Further calls are allowed but should not be passed configuration options (as they will be ignored)."
+    );
+  }
+
+  return false;
+}
+
+function configureBeeline(opts) {
+  configured = true;
   api.configure(opts);
   instrumentation.configure(opts);
   propagation.configure(opts);
+}
+
+function configure(opts) {
+  const needConfigure = checkIfConfigureNeeded(opts);
+
+  if (needConfigure) {
+    configureBeeline(opts);
+  }
 
   return configure;
 }


Test patch:
diff --git a/lib/index.test.js b/lib/index.test.js
index 703d3a3..ba2cd3b 100644
--- a/lib/index.test.js
+++ b/lib/index.test.js
@@ -13,7 +13,25 @@ test("the default export function returns itself, so the properties are still th
 });
 
 test("the function is idempotent.  call it as often as you want.", () => {
+  const oldConsole = global.console;
+  global.console = { warn: jest.fn() };
+
   const beeline = require(".");
+  beeline();
+  expect(console.warn).not.toBeCalled();
+
+  // but we'll warn if you call it again with options
   beeline({ disableInstrumentation: true });
-  beeline({ disableInstrumentation: true });
+  expect(console.warn).toBeCalled();
+  expect(console.warn.mock.calls[0][0]).toEqual(
+    expect.stringContaining("Beeline is already configured")
+  );
+
+  console.warn.mockClear();
+
+  // and make sure it's just when options are present
+  beeline();
+  expect(console.warn).not.toBeCalled();
+
+  global.console = oldConsole;
 });