feat(elasticsearch): support @elastic/elasticsearch client

[trentm]

Based on earlier work in:
#1266

Fixes #968

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #1877 updated

• Start Time: 2020-12-15T18:26:13.389+0000

• Duration: 18 min 12 sec

Test stats 🧪

Test	Results
Failed	0
Passed	16398
Skipped	0
Total	16398

[trentm]

This is ready to review now. There are a few "TODO" comments in there:

% git diff master.. | grep TODO
+// TODO: ask Tomas about apiName addition to RequestEvent
+        // TODO set "span.outcome" (from err and result.statusCode)
+          // TODO: rebuild error object to avoid serialization issues?
+    // TODO test that httpSpan is child of esSpan? currently it isn't

that I don't think all need to be done now.

@delvedor You are welcome to review and/or play with this, but I don't expect you to.

[trentm]

review note: the linting "error" will go away when these commits are squashed

[astorm]

I took an initial pass at this -- it looks like a solid start and captures the functionality of the previous instrumentation and @delvedor's first pass at this well. The use of the WeakMap is a nice call-out as well -- Trust your Mechanic Garbage Collector :)

It doesn't look like sub-classing is the common approach for instrumentation in the agent -- normally we'd identify the request method, wrap it, start our business, call the original method, and then clean up our business.

I think the sub-classing/event approach can work though -- and in some ways seems less intrusive than the direct monkey-patching we normally do. There's a few additional defensive measures we'll want to take, but otherwise I'm down with this approach.

[trentm]

I think the sub-classing/event approach can work though -- and in some ways seems less intrusive than the direct monkey-patching we normally do.

My understanding is this was intentional. The 'request' and 'response' events described at https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/observability.html were, at least in part, intended for this kind of thing.

[trentm]

Remaining TODOs before merging:

• Decide on error serialization. From the original PR: "we should figure out how to serialize the errors to avoid the loss of important information"
• Docs and changelog entry.

Related TODOs that can be done separate from this PR (IMHO):

• Look into getting the http span(s) to be a child of the Elasticsearch span -> startSpan('A'); startSpan('B') in the same async op: B should be a child of A, but isn't #1889
• Set "span.outcome" (from err and result.statusCode). Deferring to Add outcome to transactions and spans #1814
• Possibly open a ticket/PR on elasticsearch-js to have an apiName field on the "request" event data to identify what Elasticsearch API endpoint is being called. This would make the current pathIsAQuery filtering (to decide if context.db is added to an APM span) more efficient and precise. [Update 20201126: Tomas has a PR for this! https://github.com/Added API name to request metadata elasticsearch-js#1364 Target release would be 7.11.]
• Open a separate ticket/PR to look at adding a "prepare-request" (or whatever name) event from elasticsearch-js to include request setup time. This was explored in the original PR: https://github.com/elastic/apm-agent-nodejs/pull/1266/files#diff-8b8cdefabed92970b05a687c766e3c51678ea17686b219d93ff63043f40a6538R27 and ... I can't find the "elasticsearch-js/update-events" branch anymore (perhaps since deleted). [Update 20201126: Tomas has a start at a PR for this, targetting 7.11. https://github.com/Added new observability events elasticsearch-js#1365]

[trentm]

I am moving the TODO item Look into getting the http span(s) to be a child of the Elasticsearch span to a separate ticket, because handling that is a bigger change to agent behaviour: #1889

[trentm]

jenkins run the module tests for elasticsearch,@elastic/elasticsearch

[trentm]

jenkins run the module tests for elasticsearch,@elastic/elasticsearch

[trentm]

jenkins run the module tests for elasticsearch,@elastic/elasticsearch

[trentm]

jenkins run the module tests for ALL

[trentm]

Since the last time y'all reviewed:

• I moved to using apm.captureError(..., {captureAttributes: false}) to avoid deopt (per @vigneshshanmugam comment): https://github.com/elastic/apm-agent-nodejs/pull/1877/files#diff-8b8cdefabed92970b05a687c766e3c51678ea17686b219d93ff63043f40a6538R92-R97
• I fixed a side issue where error.exception.module was not handling "@ns/package" namespaced packages: https://github.com/elastic/apm-agent-nodejs/pull/1877/files#diff-9ceb00d437ddb7c1de1041eea52561f5e7166af4375d35d769590caace91ed31R282-R307
• I removed races from the added tests.

This is ready for re-review when you have the time. Thanks.

[astorm]

Looks good from here -- previous feedback's been address and it gets us instrumentation for the modern @elastic/elasticsearch client. One dealer's choice comment on some additional test coverage, but I think we're good to go as it. Approving.