fix: ensure correct run context for '@elastic/elasticsearch' instrumentation #2550

trentm · 2022-01-27T20:48:42Z

This is a re-write of the @elastic/elasticsearch instrumentation that
stops using the ES client observability events
https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/observability.html
and switches to patching Transport#request() instead. This is necessary
to be able to bind that request() invocation to the RunContext for the
Span we've created; without using apm.startSpan(...), which bleeds the
RunContext out to user code. (Patching Transport#request() is what the
legacy 'elasticsearch' instrumentation is also doing.)

Refs: #2430

Checklist

Implement code
Add tests
ensure TAV tests pass
Add CHANGELOG.asciidoc entry

…ntation This is a re-write of the @elastic/elasticsearch instrumentation that stops using the ES client observability events https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/observability.html and switches to patching Transport#request() instead. This is necessary to be able to bind that `request()` invocation to the RunContext for the Span we've created; without using `apm.startSpan(...)`, which bleeds the RunContext out to user code. (Patching Transport#request() is what the legacy 'elasticsearch' instrumentation is also doing.) Refs: #2430

apmmachine · 2022-01-27T21:18:38Z

💚 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

Reason: null
Start Time: 2022-01-28T17:50:45.710+0000
Duration: 56 min 51 sec
Commit: 94a2b2e

Test stats 🧪

Test	Results
Failed	0
Passed	244041
Skipped	0
Total	244041

🤖 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.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

trentm · 2022-01-27T21:40:14Z

run module tests for @elastic/elasticsearch,@elastic/elasticsearch-canary,elasticsearch

…ater elastic/elasticsearch-js@76659b6 special-cased 'sort' so it is always passed as a query param in the ES request.

- No longer need to adjust tests around elasticsearch-js issue 151 because we are no longer using the diagnostics events where that happened. - Cope with <v7.7.0 where the instrumentation would create *two* spans for Promise-usage of client requests.

trentm · 2022-01-28T00:38:44Z

run module tests for @elastic/elasticsearch,@elastic/elasticsearch-canary,elasticsearch

trentm · 2022-01-28T00:42:35Z

.tav.yml

@@ -367,7 +367,7 @@ elasticsearch:
  commands: node test/instrumentation/modules/@elastic/elasticsearch.test.js
 '@elastic/elasticsearch-canary':
  name: '@elastic/elasticsearch-canary'
-  versions: '^8.0.0-canary.35'
+  versions: '>=8.0.0-canary.36 || >=8.1.0-canary.2'


REVIEW NOTE: Kibana's HEAD is using 8.1.0-canary.2 now. I had to drop at least 8.0.0-canary.35 because our tests depend on elastic/elasticsearch-js@76659b6 which special-cased handling of the "sort" param to client commands such that it is always passed as a query param to ES.

trentm · 2022-01-28T00:44:24Z

lib/instrumentation/elasticsearch-shared.js

          parts.push(JSON.stringify(body))
-        }
+        } catch {}


REVIEW NOTE: try/catch around these in case there is a surprise circular ref or something. I'll be opening a separate ticket to improve our instrumentation patching so that we can capture the body and querystring as serialized by the client instead of having to do it (again) in the agent.

trentm · 2022-01-28T00:46:06Z

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

@@ -542,17 +541,11 @@ ${body.map(JSON.stringify).join('\n')}

          const err = data.errors
            .filter((e) => e.exception.type === 'RequestAbortedError')[0]
-          if (semver.satisfies(esVersion, '7.14.x')) {
-            // https://github.com/elastic/elasticsearch-js/issues/1517 was fixed


REVIEW NOTE: This issue was only relevant when using the diagnostics events from the ES client. Given that the instrumentation re-write is no longer using those events, we no longer need to have the tests workaround the issue.

trentm · 2022-01-28T00:56:59Z

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

-      // Remove leading ES span and HTTP span from product check.
-      data.spans = data.spans.slice(2)
+      // Remove the product check spans for subsequent assertions.
+      data.spans = data.spans.slice(0, 1).concat(data.spans.slice(3))


REVIEW NOTE: The order of product-check spans has changed due to the change in the implementation of the instrumentation. We create a Span on Transport#request() now, rather than later when the "request" diagnostic event is sent. That means that the first Span is the one we want to keep.

ES span for the actual client command (want to keep this one)

ES span for the product check

HTTP span for the product check

HTTP span for the actual client command (want to keep this one)

trentm · 2022-01-28T00:59:16Z

test/instrumentation/modules/elasticsearch.test.js

@@ -125,7 +125,7 @@ if (semver.satisfies(pkg.version, '>= 13')) {
      }
    ]

-    var statement = body.map(JSON.stringify).join('\n')
+    var statement = body.map(JSON.stringify).join('\n') + '\n'


REVIEW NOTE: This is for the slight change in setElasticsearchDbContext()'s handling of the Array.isArray(body) case to match how this is serialized by the @elastic/elasticsearch client.

…t out to user code

trentm · 2022-01-28T05:28:13Z

run module tests for @elastic/elasticsearch,@elastic/elasticsearch-canary,elasticsearch

trentm · 2022-01-28T16:48:25Z

run module tests for @elastic/elasticsearch,@elastic/elasticsearch-canary,elasticsearch

…service container was not being setup for its test run; we didn't see this in CI before because the Jenkins TAV config doesn't have the canary module

trentm · 2022-01-28T17:50:04Z

run module tests for @elastic/elasticsearch,@elastic/elasticsearch-canary,elasticsearch

In #2550 this module was rewritten to not be primarily based on the ES client "diagnostic" events for instrumentation. That change should have also removed this part of the top-comment. Refs: #2550

In elastic#2550 this module was rewritten to not be primarily based on the ES client "diagnostic" events for instrumentation. That change should have also removed this part of the top-comment. Refs: elastic#2550

trentm self-assigned this Jan 27, 2022

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jan 27, 2022

trentm mentioned this pull request Jan 27, 2022

run-context for spans created by instrumentations should not bleed out to user code #2430

Closed

38 tasks

Merge branch 'main' into trentm/run-context-es

c470455

changelog entry

f17c58a

trentm added 2 commits January 27, 2022 15:45

our ES client canary tests depend on a change only in canary.36 and l…

4ccebd8

…ater elastic/elasticsearch-js@76659b6 special-cased 'sort' so it is always passed as a query param in the ES request.

trentm commented Jan 28, 2022

View reviewed changes

fix node 8 syntax (no empty catch); add tests for bleeding run-contex…

38effb0

…t out to user code

ensure APM Server version fetch doesn't surprise these tests

5a11de6

'@elastic/elasticsearch-canary' TAV tests were failing because an ES …

94a2b2e

…service container was not being setup for its test run; we didn't see this in CI before because the Jenkins TAV config doesn't have the canary module

trentm marked this pull request as ready for review January 28, 2022 18:57

trentm requested a review from astorm January 28, 2022 18:57

astorm approved these changes Jan 29, 2022

View reviewed changes

trentm merged commit ddad434 into main Jan 31, 2022

trentm deleted the trentm/run-context-es branch January 31, 2022 15:52

trentm mentioned this pull request Jan 24, 2023

chore: correct comment on ES client instrumentation #3122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure correct run context for '@elastic/elasticsearch' instrumentation #2550

fix: ensure correct run context for '@elastic/elasticsearch' instrumentation #2550

trentm commented Jan 27, 2022 •

edited

Loading

apmmachine commented Jan 27, 2022 •

edited

Loading

Build stats

Test stats 🧪

trentm commented Jan 27, 2022

trentm commented Jan 28, 2022

trentm Jan 28, 2022

trentm Jan 28, 2022

trentm Jan 28, 2022

trentm Jan 28, 2022

trentm Jan 28, 2022

trentm commented Jan 28, 2022

trentm commented Jan 28, 2022

trentm commented Jan 28, 2022

fix: ensure correct run context for '@elastic/elasticsearch' instrumentation #2550

fix: ensure correct run context for '@elastic/elasticsearch' instrumentation #2550

Conversation

trentm commented Jan 27, 2022 • edited Loading

Checklist

apmmachine commented Jan 27, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

trentm commented Jan 27, 2022

trentm commented Jan 28, 2022

trentm Jan 28, 2022

Choose a reason for hiding this comment

trentm Jan 28, 2022

Choose a reason for hiding this comment

trentm Jan 28, 2022

Choose a reason for hiding this comment

trentm Jan 28, 2022

Choose a reason for hiding this comment

trentm Jan 28, 2022

Choose a reason for hiding this comment

trentm commented Jan 28, 2022

trentm commented Jan 28, 2022

trentm commented Jan 28, 2022

trentm commented Jan 27, 2022 •

edited

Loading

apmmachine commented Jan 27, 2022 •

edited

Loading