feat: support @elastic/elasticsearch@8 #2385

trentm · 2021-10-22T00:17:26Z

This adds support for the still pre-release version 8.x of the ES client. We are testing canary releases via the @elastic/elasticsearch-canary package. Kibana v8 is working to use v8 of the ES client and part of that is the self-APM integration in Kibana supporting instrumenting it.

Fixes: #2355

Checklist

This is a first incomplete crack at it. TAV is probably wrong. There are some XXXs to complete in the tests for a change in the 'request' diagnostic data.

apmmachine · 2021-10-22T00:37:27Z

💚 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

Start Time: 2021-10-25T18:48:19.053+0000
Duration: 20 min 24 sec
Commit: ecbb6a1

Test stats 🧪

Test	Results
Failed	0
Passed	22
Skipped	0
Total	22

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules
run benchmark tests : Run the benchmark test only.

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

…or config.test.js

trentm · 2021-10-25T01:04:41Z

Local TAV test run pass FWIW:

[18:01:49 trentm@pink:~/el/apm-agent-nodejs5 (n:16 git:trentm/esclient8)]
% TAV=@elastic/elasticsearch-canary npm run test:tav

> [email protected] test:tav
> tav --quiet

-- required packages ["@elastic/[email protected]"]
-- installing ["@elastic/[email protected]"]
-- running test "node test/instrumentation/modules/@elastic/elasticsearch.test.js" with @elastic/elasticsearch-canary
-- required packages ["@elastic/[email protected]"]
-- installing ["@elastic/[email protected]"]
-- running test "node test/instrumentation/modules/@elastic/elasticsearch.test.js" with @elastic/elasticsearch-canary
-- required packages ["@elastic/[email protected]"]
-- installing ["@elastic/[email protected]"]
-- running test "node test/instrumentation/modules/@elastic/elasticsearch.test.js" with @elastic/elasticsearch-canary
-- required packages ["@elastic/[email protected]"]
-- installing ["@elastic/[email protected]"]
-- running test "node test/instrumentation/modules/@elastic/elasticsearch.test.js" with @elastic/elasticsearch-canary
-- ok

trentm · 2021-10-25T02:18:41Z

See also #2389 and #2388 for follow up work.

running (cwd: ./): node --unhandled-rejections=strict test/instrumentation/modules/@elastic/elasticsearch-canary.test.js TAP version 13 # client.ping with promise not ok 1 ProductNotSupportedError: The client noticed that the server is not Elasticsearch and we do not support this unknown product. --- operator: error stack: |- ProductNotSupportedError: The client noticed that the server is not Elasticsearch and we do not support this unknown product. at SniffingTransport.request (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/@elastic/transport/lib/Transport.js:405:27) at processTicksAndRejections (internal/process/task_queues.js:97:5) at async ApmClient.PingApi [as ping] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/@elastic/elasticsearch-canary/lib/api/api/ping.js:37:12)

…upport ES 7.10.1, but do 7.15.1

…differ enough, esp. while v8 is still pre-release

…and published in @elastic/[email protected]

I believe this is (a) safer and (b) doesn't lose any functionality. "Safer" because, a recent change in elastic/elasticsearch-js to the types for this "body" field on the "request" diagnostic event shows these possible types for version 8 of the ES client: body?: string | Buffer | ReadableStream | null That "body" can be a Buffer or a ReadableStream. JSON.stringify-ing those for span.context.db.statement could possibly result in a HUGE object. This isn't the intent. No loss in functionality because we have tests for most of the ES queries that we support: const pathIsAQuery = /\/(_search|_msearch|_count|_async_search|_sql|_eql)(\/|$)/ or we should, and a TAV test run shows no test failures with this change. Better safe for possible future additions to `pathIsAQuery` than a theoretical regression in the value captured for span.context.db.statement.

trentm · 2021-10-25T18:34:44Z

lib/instrumentation/elasticsearch-shared.js

-    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))
-      }
+    if (typeof (body) === 'string') {
+      parts.push(body)


REVIEW NOTE: The acc97e5 commit message explains the reasoning for this change.

…asticsearch' client

trentm · 2021-10-25T20:04:02Z

Tests all passed, I'm running a TAV test run now. From the TAV test run I only care about elasticsearch client tests. I expect there may be failures in fastify and hapi tests (see #2387 for a later fix for those).

astorm

👍 approving. It does the thing, splitting the new example makes sense with the API changes in 8, and adding the canary repo should help us be ahead of the curve when it comes to changes.

astorm · 2021-10-25T19:22:11Z

.tav.yml

  name: '@elastic/elasticsearch'
  versions: '>=7.12.0 <8.0.0'
  node: '>=10.0.0'
  commands: node test/instrumentation/modules/@elastic/elasticsearch.test.js
+'@elastic/elasticsearch-canary':
+  name: '@elastic/elasticsearch-canary'
+  versions: '^8.0.0-canary.35'


TIL: semver canary conventions 👍

Also -- it looks like canary release are coming fast and furious, with multiples in a day. We may wan to revisit testing every release greater than -canary.35 in the future and adopt an approach similar to what we do for AWS modules with a large number of releases. What we have here now though should be fine.

Right now I am trying to (manually) track what the Kibana 8.0 PR is using.

Potentially after the 8.0 release of the ES client settles down we should remove testing of the canary package.

astorm · 2021-10-25T19:48:32Z

lib/instrumentation/index.js

@@ -11,7 +11,7 @@ var shimmer = require('./shimmer')
 var Transaction = require('./transaction')

 var MODULES = [
-  '@elastic/elasticsearch',
+  ['@elastic/elasticsearch', '@elastic/elasticsearch-canary'],


👍 tested this manually (in addition to the automated test changes below), and both a require('@elastic/elasticsearch') and require('@elastic/elasticsearch-canary') result in the agent loading the instrumentation module.

trentm · 2021-10-25T20:10:17Z

Something in the Jenkins run for the TAV tests is ridiculously slow/stuck: https://apm-ci.elastic.co/job/apm-agent-nodejs/job/apm-agent-nodejs-mbp/job/PR-2385/14/
I'm not waiting for those to get going or complete.

apmmachine · 2021-10-25T20:52:12Z

💔 Build Failed

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

Expand to view the summary

Build stats

Start Time: 2021-10-25T19:41:12.577+0000
Duration: 71 min 46 sec
Commit: ecbb6a1

Steps errors

Expand to view the steps failures

Show only the first 10 steps failures

`Run Tests`

Took 6 min 53 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "14"

`Run Tests`

Took 9 min 34 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "12"

`Run Tests`

Took 7 min 0 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "12"

`Run Tests`

Took 11 min 24 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "10"

`Run Tests`

Took 6 min 31 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "10"

`Run Tests`

Took 12 min 10 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "@elastic/elasticsearch" "8"

`Run Tests`

Took 0 min 21 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "@elastic/elasticsearch" "8"

`Run Tests`

Took 13 min 7 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "8"

`Run Tests`

Took 6 min 16 sec . View more details here
Description: .ci/scripts/test.sh -b "release" -t "hapi" "8"

`Error signal`

Took 0 min 0 sec . View more details here
Description: hudson.AbortException: script returned exit code 217

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules
run benchmark tests : Run the benchmark test only.

When `npm test` is run outside of CI, then "test/docker-compose.yml" is used to setup required services (redis, ES, etc.). This is a separate compose file than is used in CI. To test @elastic/elasticsearch@8 instrumentation requires an ES server at least 7.14 for its product check to work. The test ES service had been updated for Jenkins and GH CI, but this one was missed. Refs: #2385

feat: support @elastic/elasticsearch@8

925043c

This is a first incomplete crack at it. TAV is probably wrong. There are some XXXs to complete in the tests for a change in the 'request' diagnostic data.

trentm self-assigned this Oct 22, 2021

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 22, 2021

trentm mentioned this pull request Oct 22, 2021

support v8.x of @elastic/elasticsearch #2355

Closed

delvedor reviewed Oct 22, 2021

View reviewed changes

lib/instrumentation/modules/@elastic/elasticsearch.js Show resolved Hide resolved

ensure ES canary client isn't imported with node 8.6 (syntax error) f…

520298f

…or config.test.js

trentm mentioned this pull request Oct 25, 2021

'request' diagnostic event .meta.request.params for msearch no longer has serialized body field elastic/elasticsearch-js#1583

Closed

updates to TODO notes

1d17812

trentm mentioned this pull request Oct 25, 2021

@elastic/elasticsearch@8 instrumentation of msearch does not capture search body in span.context.db.statement #2388

Closed

trentm added 4 commits October 24, 2021 19:19

move out some follow-up notes to separate issues

62f4540

remove debugging print

d6bbbd7

fix tests to actually run against '@elastic/elasticsearch-canary'

cb9cd6a

changelog and docs update for es client v8 support

d074bf3

trentm marked this pull request as ready for review October 25, 2021 02:44

trentm requested a review from astorm October 25, 2021 02:44

trentm added 4 commits October 24, 2021 20:07

same for Jenkins tests; the ES product checks in ES client v8 don't s…

3edec95

…upport ES 7.10.1, but do 7.15.1

separate v7 and v8 versions of ES client trace examples because they …

e3c4af4

…differ enough, esp. while v8 is still pre-release

add test for AbortController usage with ES client v8 (this is #2389)

c10b833

trentm mentioned this pull request Oct 25, 2021

add AbortController test to @elastic/elasticsearch@8 tests #2389

Closed

trentm added 3 commits October 25, 2021 10:56

fix make check; drop unneeded old 7.14 workaround in this 8.x-only test

9c281c1

fix this issue now that elastic/elasticsearch-js#1583 has been fixed …

53cb783

…and published in @elastic/[email protected]

trentm commented Oct 25, 2021

View reviewed changes

the last change to setElasticsearchDbContext broke for the legacy 'el…

ecbb6a1

…asticsearch' client

astorm approved these changes Oct 25, 2021

View reviewed changes

trentm merged commit d23e9e2 into master Oct 25, 2021

trentm deleted the trentm/esclient8 branch October 25, 2021 20:14

trentm mentioned this pull request Oct 25, 2021

tests: skip ES client tests that use Promise.finally on node 8 #2390

Merged

trentm mentioned this pull request Oct 27, 2021

tests: fix elasticsearch-canary.test.js when run outside of CI #2397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support @elastic/elasticsearch@8 #2385

feat: support @elastic/elasticsearch@8 #2385

trentm commented Oct 22, 2021 •

edited

Loading

apmmachine commented Oct 22, 2021 •

edited

Loading

Build stats

Test stats 🧪

trentm commented Oct 25, 2021

trentm commented Oct 25, 2021

trentm Oct 25, 2021

trentm commented Oct 25, 2021

astorm left a comment

astorm Oct 25, 2021

trentm Oct 25, 2021

astorm Oct 25, 2021

trentm commented Oct 25, 2021

apmmachine commented Oct 25, 2021

Build stats

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Error signal`

feat: support @elastic/elasticsearch@8 #2385

feat: support @elastic/elasticsearch@8 #2385

Conversation

trentm commented Oct 22, 2021 • edited Loading

Checklist

apmmachine commented Oct 22, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

trentm commented Oct 25, 2021

trentm commented Oct 25, 2021

trentm Oct 25, 2021

Choose a reason for hiding this comment

trentm commented Oct 25, 2021

astorm left a comment

Choose a reason for hiding this comment

astorm Oct 25, 2021

Choose a reason for hiding this comment

trentm Oct 25, 2021

Choose a reason for hiding this comment

astorm Oct 25, 2021

Choose a reason for hiding this comment

trentm commented Oct 25, 2021

apmmachine commented Oct 25, 2021

💔 Build Failed

Build stats

Steps errors

Run Tests

Run Tests

Run Tests

Run Tests

Run Tests

Run Tests

Run Tests

Run Tests

Run Tests

Error signal

🤖 GitHub comments

trentm commented Oct 22, 2021 •

edited

Loading

apmmachine commented Oct 22, 2021 •

edited

Loading

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Run Tests`

`Error signal`