Repository: newrelic/newrelic-node-apollo-server-plugin
Pull request #62 (2020-11-09 21:25:35)

Fixes bug improperly handling state loss prior to plugin execution.
<!--
Thank you for submitting a Pull Request.

This code is leveraged to monitor critical services. Please consider the following:
* Tests are required.
* Performance matters.
* Features that are specific to just your app are unlikely to make it in.

Please fill out the relevant sections as follows:
* Proposed Release Notes: Bulleted list of recommended release notes for the change(s).
* Links: Any relevant links for the change.
* Details: In-depth description of changes, other technical notes, etc.
-->

## Proposed Release Notes

* Fixed bug that would cause errors if transaction state loss occurred prior to plugin execution.

## Links

* Fixes: https://github.com/newrelic/newrelic-node-apollo-server-plugin/issues/59

## Details

This will fix the error part of the attached issue. The customer will still have state loss issues (unsure the cause).

One thing for us to consider is if we want to wrap some (or all) of these handlers in `try/catch` blocks to prevent accidental future bugs from erroring out customer queries. There can be some complication there with our usage of closures and any potential overhead. Chose not to tackle that with this effort.



Patch:
diff --git a/lib/create-plugin.js b/lib/create-plugin.js
index 4226029..00f7861 100644
--- a/lib/create-plugin.js
+++ b/lib/create-plugin.js
@@ -71,6 +71,11 @@ function createPlugin(instrumentationApi, config = {}) {
 
       const requestParent = instrumentationApi.getActiveSegment()
 
+      if (!requestParent) {
+        logger.trace('No active segment found at query start. Not recording.')
+        return null
+      }
+
       // We do not set to active here as batched queries will hit this
       // back to back and we'd prefer those not nest with each-other.
       const operationSegment = instrumentationApi.createSegment(
@@ -78,6 +83,12 @@ function createPlugin(instrumentationApi, config = {}) {
         recordOperationSegment,
         requestParent
       )
+
+      if (!operationSegment) {
+        logger.trace('Operation segment was not created. Not recording.')
+        return null
+      }
+
       operationSegment.start()
 
       const deepestResolvedPath = {
@@ -123,6 +134,15 @@ function createPlugin(instrumentationApi, config = {}) {
                 operationSegment
               )
 
+              if (!resolverSegment) {
+                logger.trace(
+                  'Resolver segment was not created (%s).',
+                  formattedPath
+                )
+
+                return null
+              }
+
               resolverSegment.start()
               instrumentationApi.setActiveSegment(resolverSegment)
 


Test patch:
diff --git a/tests/apollo-server-setup.js b/tests/apollo-server-setup.js
index 5fc0648..0a36fe3 100644
--- a/tests/apollo-server-setup.js
+++ b/tests/apollo-server-setup.js
@@ -14,7 +14,13 @@ const { getTypeDefs, resolvers } = require('./data-definitions')
 
 const WEB_FRAMEWORK = 'Expressjs'
 
-function setupApolloServerTests({suiteName, createTests, pluginConfig}, agentConfig) {
+function setupApolloServerTests(options, agentConfig) {
+  const {
+    suiteName,
+    createTests,
+    pluginConfig
+  } = options
+
   tap.test(`apollo-server: ${suiteName}`, (t) => {
     t.autoend()
 
@@ -28,14 +34,16 @@ function setupApolloServerTests({suiteName, createTests, pluginConfig}, agentCon
       const createPlugin = require('../lib/create-plugin')
       const nrApi = helper.getAgentApi()
 
-      const plugin = createPlugin(nrApi.shim, pluginConfig)
+      const startingPlugins = initializePlugins(nrApi.shim, options.startingPlugins)
+
+      const instrumentationPlugin = createPlugin(nrApi.shim, pluginConfig)
 
       // Do after instrumentation to ensure express isn't loaded too soon.
       const { ApolloServer, gql } = require('apollo-server')
       server = new ApolloServer({
         typeDefs: getTypeDefs(gql),
         resolvers,
-        plugins: [plugin]
+        plugins: [...startingPlugins, instrumentationPlugin]
       })
 
       server.listen({port: 0}).then(({ url }) => {
@@ -73,6 +81,19 @@ function clearCachedModules(modules, callback) {
   callback()
 }
 
+function initializePlugins(instrumentationApi, plugins) {
+  plugins = plugins || []
+  const initializedPlugins = plugins.map((plugin) => {
+    if (typeof plugin === 'function') {
+      return plugin(instrumentationApi)
+    }
+
+    return plugin
+  })
+
+  return initializedPlugins
+}
+
 module.exports = {
   setupApolloServerTests
 }
diff --git a/tests/integration/state-loss.test.js b/tests/integration/state-loss.test.js
new file mode 100644
index 0000000..b3a7dc4
--- /dev/null
+++ b/tests/integration/state-loss.test.js
@@ -0,0 +1,65 @@
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+
+const { executeQuery } = require('../test-client')
+const { setupEnvConfig } = require('../agent-testing')
+
+const { setupApolloServerTests } = require('../apollo-server-setup')
+
+setupApolloServerTests({
+  suiteName: 'State Loss',
+  createTests: createStateLossTests,
+  startingPlugins: [createStateLossPlugin]
+})
+
+function createStateLossTests(t) {
+  setupEnvConfig(t)
+
+  t.test('should should not error when state loss prior to query', (t) => {
+    const { serverUrl } = t.context
+
+    const expectedName = 'GetAllForLibrary'
+    const query = `query ${expectedName} {
+      library(branch: "downtown") {
+        books {
+          title
+          author {
+            name
+          }
+        }
+        magazines {
+          title
+          issue
+        }
+      }
+    }`
+
+    executeQuery(serverUrl, query, (err, result) => {
+      t.error(err)
+
+      if (result.errors) {
+        result.errors.forEach((error) => {
+          t.error(error)
+        })
+      }
+
+      t.ok(result.data)
+      t.ok(result.data.library)
+
+      t.end()
+    })
+  })
+}
+
+function createStateLossPlugin(instrumentationApi) {
+  return {
+    requestDidStart() {
+      // Setting active segment to null to mimic state loss
+      instrumentationApi.setActiveSegment(null)
+    }
+  }
+}