feat(elasticsearch): support @elastic/elasticsearch client

.ci/.jenkins_tav.yml

@@ -1,4 +1,5 @@
 TAV:
+  - '@elastic/elasticsearch'
   - '@hapi/hapi'
   - '@koa/router'
   - apollo-server-express

.ci/docker/docker-compose.yml

@@ -71,7 +71,7 @@ services:
       retries: 30
 
   elasticsearch:
-    image: docker.elastic.co/elasticsearch/elasticsearch:6.6.0
+    image: docker.elastic.co/elasticsearch/elasticsearch:7.10.1
     environment:
       - "ES_JAVA_OPTS=-Xms512m -Xmx512m"
       - "network.host="

.tav.yml

@@ -242,6 +242,15 @@ koa-router:
 elasticsearch:
   versions: '>=8.0.0'
   commands: node test/instrumentation/modules/elasticsearch.js
+
+# @elastic/elasticsearch
+# - Version 7.7.0 included a change that broke usage with Node.js < 10.
+#   Fixed in 7.7.1: https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/changelog-client.html#_7_7_1
+#   Note: When Node.js v8 support is dropped, `versions` can be simplified.
+'@elastic/elasticsearch':
+  versions: '>=7.0.0 <7.7.0 || >7.7.0 <8.0.0'
+  commands: node test/instrumentation/modules/@elastic/elasticsearch.js
+
 handlebars:
   versions: '*'
   commands: node test/instrumentation/modules/handlebars.js

CHANGELOG.asciidoc

@@ -38,11 +38,24 @@ Notes:
 [float]
 ===== Features
 
+* feat: Add automatic instrumentation of the https://github.com/elastic/elasticsearch-js[@elastic/elasticsearch] package {pull}1877[#1870]
++
+The instrumentation of the legacy "elasticsearch" package has also changed
+slightly to commonalize:
++
+** "span.context.destination" is set on all Elasticsearch spans, not just a
+   subset of query-like API endpoints.
+** For query-like API endpoints (e.g. `/_search`), the capturing of query details
+   on "span.context.db.statement" has changed (a) to include *both* the
+   query params and the request body if both exist (separated by `\n\n`) and
+   (b) to *URL encode* the query params, rather than JSON encoding.
+
 * feat: Add `captureAttributes` boolean option to `apm.captureError()` to
   allow *disabling* the automatic capture of Error object properties. This
   is useful for cases where those properties should not be sent to the APM
   Server, e.g. for performance (large string fields) or security (PII data).
   {pull}1912[#1912]
+
 * feat: Add `log_level` central config support. {pull}1908[#1908] +
   Spec: https://github.com/elastic/apm/blob/master/specs/agents/logging.md
 

docs/supported-technologies.asciidoc

@@ -74,6 +74,7 @@ The Node.js agent will automatically instrument the following modules to give yo
 |Module |Version |Note
 |https://www.npmjs.com/package/cassandra-driver[cassandra-driver] |>=3.0.0 |Will instrument all queries
 |https://www.npmjs.com/package/elasticsearch[elasticsearch] |>=8.0.0 |Will instrument all queries
+|https://www.npmjs.com/package/@elastic/elasticsearch[@elastic/elasticsearch] |>=7.0.0 <8.0.0 |Will instrument all queries
 |https://www.npmjs.com/package/graphql[graphql] |>=0.7.0 <16.0.0 |Will instrument all queries
 |https://www.npmjs.com/package/handlebars[handlebars] |* |Will instrument compile and render calls
 |https://www.npmjs.com/package/jade[jade] |>=0.5.6 |Will instrument compile and render calls

lib/instrumentation/elasticsearch-shared.js

@@ -0,0 +1,63 @@
+'use strict'
+
+// Shared functionality between the instrumentations of:
+// - elasticsearch - the legacy Elasticsearch JS client
+// - @elastic/elasticsearch - the new Elasticsearch JS client
+
+const querystring = require('querystring')
+
+// URL paths matching the following pattern will have their query params and
+// request body captured in the span (as `context.db.statement`). We match
+// a complete URL path component to attempt to avoid accidental matches of
+// user data, like `GET /my_index_search/...`.
+const pathIsAQuery = /\/(_search|_msearch|_count|_async_search|_sql|_eql)(\/|$)/
+
+// (This is exported for testing.)
+exports.pathIsAQuery = pathIsAQuery
+
+// Set the span's `context.db` from the Elasticsearch request querystring and
+// body, if the request path looks like it is a query API.
+//
+// Some ES endpoints, e.g. '_search', support both query params and a body.
+// We encode both into 'span.context.db.statement', separated by '\n\n'
+// if both are present. E.g. for a possible msearch:
+//
+//    search_type=query_then_fetch&typed_keys=false
+//
+//    {}
+//    {"query":{"query_string":{"query":"pants"}}}
+exports.setElasticsearchDbContext = function (span, path, query, body) {
+  if (pathIsAQuery.test(path)) {
+    // From @elastic/elasticsearch: A read of Transport.js suggests query and
+    // body will always be serialized strings, however the documented
+    // `TransportRequestParams` allows for non-strings, so we will be defensive.
+    //
+    // From legacy elasticsearch: query will be an object and body will be an
+    // object, or an array of objects, e.g. for bulk endpoints.
+    const parts = []
+    if (query) {
+      if (typeof (query) === 'string') {
+        parts.push(query)
+      } else if (typeof (query) === 'object') {
+        const encodedQuery = querystring.encode(query)
+        if (encodedQuery) {
+          parts.push(encodedQuery)
+        }
+      }
+    }
+    if (body) {
+      if (typeof (body) === 'string') {
+        parts.push(body)
+      } else if (Array.isArray(body)) {
+        parts.push(body.map(JSON.stringify).join('\n')) // ndjson
+      } else if (typeof (body) === 'object') {
+        parts.push(JSON.stringify(body))
+      }
+    }
+
+    span.setDbContext({
+      type: 'elasticsearch',
+      statement: parts.join('\n\n')
+    })
+  }
+}

lib/instrumentation/index.js

@@ -11,6 +11,7 @@ var shimmer = require('./shimmer')
 var Transaction = require('./transaction')
 
 var MODULES = [
+  '@elastic/elasticsearch',
   'apollo-server-core',
   'bluebird',
   'cassandra-driver',

lib/instrumentation/modules/@elastic/elasticsearch.js

@@ -0,0 +1,123 @@
+'use strict'
+
+// Instrument the @elastic/elasticsearch module.
+//
+// This uses to 'request' and 'response' events from the Client (documented at
+// https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/observability.html)
+// to hook into all ES server interactions.
+
+const { getDBDestination } = require('../../context')
+const { setElasticsearchDbContext } = require('../../elasticsearch-shared')
+
+module.exports = function (elasticsearch, agent, { version, enabled }) {
+  if (!enabled) {
+    return elasticsearch
+  }
+  if (!elasticsearch.Client) {
+    agent.logger.debug('@elastic/elasticsearch@%s is not supported (no `elasticsearch.Client`) - aborting...', version)
+    return elasticsearch
+  }
+
+  function generateSpan (params) {
+    const span = agent.startSpan(null, 'db', 'elasticsearch', 'request')
+    if (span === null) {
+      return null
+    }
+
+    span.name = `Elasticsearch: ${params.method} ${params.path}`
+    return span
+  }
+
+  class ApmClient extends elasticsearch.Client {
+    constructor (...args) {
+      super(...args)
+
+      // Mapping an ES client event `result` to its active span.
+      // - Use WeakMap to avoid a leak from possible spans that don't end.
+      // - WeakMap allows us to key off the ES client `request` object itself,
+      //   which means we don't need to rely on `request.id`, which might be
+      //   unreliable because it is user-settable (see `generateRequestId` at
+      //   https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/observability.html)
+      const spanFromEsResult = new WeakMap()
+
+      this.on('request', (err, result) => {
+        const request = result.meta.request
+        agent.logger.debug('intercepted call to @elastic/elasticsearch "request" event %o',
+          { id: request.id, method: request.params.method, path: request.params.path })
+
+        let span = spanFromEsResult.get(result)
+
+        // As of @elastic/[email protected] this event's `err` will
+        // always be null, but we'll be future-proof.
+        if (err) {
+          agent.captureError(err)
+          if (span !== undefined) {
+            span.end()
+            spanFromEsResult.delete(result)
+          }
+          return
+        }
+
+        // With retries (see `makeRequest` in Transport.js) each attempt will
+        // emit this "request" event using the same `result` object. The
+        // intent is to have one Elasticsearch span plus an HTTP span for each
+        // attempt.
+        if (!span) {
+          span = generateSpan(request.params)
+          if (span) {
+            spanFromEsResult.set(result, span)
+          }
+        }
+        if (!span) {
+          return
+        }
+
+        setElasticsearchDbContext(span, request.params.path,
+          request.params.querystring, request.params.body)
+
+        const { hostname, port } = result.meta.connection.url
+        span.setDestinationContext(
+          getDBDestination(span, hostname, port))
+      })
+
+      this.on('response', (err, result) => {
+        // TODO set "span.outcome" (from err and result.statusCode)
+
+        if (err) {
+          // Error properties are specified here:
+          // https://github.com/elastic/elasticsearch-js/blob/master/lib/errors.d.ts
+          // - We capture some data from ResponseError, which is for
+          //   Elasticsearch API errors:
+          //   https://www.elastic.co/guide/en/elasticsearch/reference/current/common-options.html#common-options-error-options
+          // - Otherwise we explicitly turn off `captureAttributes` to avoid
+          //   grabbing potentially large and sensitive properties like
+          //   `err.data` on DeserializationError.
+          const errOpts = {
+            captureAttributes: false
+          }
+          if (err.name === 'ResponseError' && err.body && err.body.error) {
+            // Include some data from the Elasticsearch API response body:
+            // https://www.elastic.co/guide/en/elasticsearch/reference/current/common-options.html#common-options-error-options
+            errOpts.custom = {
+              type: err.body.error.type,
+              reason: err.body.error.reason,
+              caused_by: err.body.error.caused_by,
+              status: err.body.status
+            }
+          }
+
+          agent.captureError(err, errOpts)
+        }
+
+        const span = spanFromEsResult.get(result)
+        if (span !== undefined) {
+          span.end()
+          spanFromEsResult.delete(result)
+        }
+      })
+    }
+  }
+
+  agent.logger.debug('subclassing @elastic/elasticsearch.Client')
+  return Object.assign(elasticsearch, { Client: ApmClient })
+}

lib/instrumentation/modules/elasticsearch.js

@@ -2,8 +2,7 @@
 
 var shimmer = require('../shimmer')
 var { getDBDestination } = require('../context')
-
-var queryRegexp = /_((search|msearch)(\/template)?|count)$/
+const { setElasticsearchDbContext } = require('../elasticsearch-shared')
 
 module.exports = function (elasticsearch, agent, { enabled }) {
   if (!enabled) return elasticsearch
@@ -19,33 +18,22 @@ module.exports = function (elasticsearch, agent, { enabled }) {
       var id = span && span.transaction.id
       var method = params && params.method
       var path = params && params.path
-      var query = params && params.query
-      var body = params && params.body
 
       agent.logger.debug('intercepted call to elasticsearch.Transport.prototype.request %o', { id: id, method: method, path: path })
 
       if (span && method && path) {
         span.name = `Elasticsearch: ${method} ${path}`
 
-        if (queryRegexp.test(path)) {
-          const statement = Array.isArray(body)
-            ? body.map(JSON.stringify).join('\n')
-            : JSON.stringify(body || query)
+        setElasticsearchDbContext(span, path, params && params.query,
+          params && params.body)
 
-          if (statement) {
-            span.setDbContext({
-              type: 'elasticsearch',
-              statement
-            })
-          }
-          // get the remote host information from elasticsearch Transport options
-          const transportConfig = this._config
-          let host, port
-          if (typeof transportConfig === 'object' && transportConfig.host) {
-            [host, port] = transportConfig.host.split(':')
-          }
-          span.setDestinationContext(getDBDestination(span, host, port))
+        // Get the remote host information from elasticsearch Transport options.
+        const transportConfig = this._config
+        let host, port
+        if (typeof transportConfig === 'object' && transportConfig.host) {
+          [host, port] = transportConfig.host.split(':')
         }
+        span.setDestinationContext(getDBDestination(span, host, port))
 
         if (typeof cb === 'function') {
           var args = Array.prototype.slice.call(arguments)

lib/parsers.js

@@ -279,13 +279,32 @@ function getCulprit (frames) {
   return filename ? fnName + ' (' + filename + ')' : fnName
 }
 
+// Infer the node.js module name from the top frame filename, if possible.
+// Examples:
+//    /home/bob/.../node_modules/mymodule/index.js
+//                               ^^^^^^^^
+//    /home/bob/.../node_modules/@myorg/mymodule/index.js
+//                               ^^^^^^^^^^^^^^^
+// or on Windows:
+//    C:\Users\bob\...\node_modules\@myorg\mymodule\lib\subpath\index.js
+//                                  ^^^^^^^^^^^^^^^
+let SEP = path.sep
+if (SEP === '\\') {
+  SEP = '\\' + SEP // Escape this for use in a regex.
+}
+const MODULE_NAME_REGEX = new RegExp(`node_modules${SEP}([^${SEP}]*)(${SEP}([^${SEP}]*))?`)
 function getModule (frames) {
   if (frames.length === 0) return
   var frame = frames[0]
   if (!frame.library_frame) return
-  var match = frame.filename.match(/node_modules\/([^/]*)/)
+  var match = frame.filename.match(MODULE_NAME_REGEX)
   if (!match) return
-  return match[1]
+  var moduleName = match[1]
+  if (moduleName && moduleName[0] === '@' && match[3]) {
+    // Normalize the module name separator to '/', even on Windows.
+    moduleName += '/' + match[3]
+  }
+  return moduleName
 }
 
 function tryJsonStringify (obj) {

package.json

@@ -117,6 +117,7 @@
     "@babel/cli": "^7.8.4",
     "@babel/core": "^7.8.4",
     "@babel/preset-env": "^7.8.4",
+    "@elastic/elasticsearch": "^7.10.0",
     "@hapi/hapi": "^18.4.1",
     "@koa/router": "^9.0.1",
     "@types/node": "^13.7.4",