feat: 'elasticsearchCaptureBodyUrls' config option #2873

trentm · 2022-08-12T20:30:53Z

This adds a new config option, per the
'elasticsearch_capture_body_urls' cross-agent spec, to control for which
ES client requests the request body is captured. By default it is just
the body of search-related APIs. The use case for configuring this is
to capture for all requests for reply in Kibana performance work. This
option is not expected to be used often.

Closes: #2880
Closes: #2019
Refs: elastic/apm#665
Refs: elastic/apm#670

checklist

finish implementation (docs, some tests)
CHANGELOG entry
TAV tests: https://apm-ci.elastic.co/job/apm-agent-nodejs/job/apm-agent-nodejs-mbp/job/PR-2873/7/
perhaps wait for the spec to be merged

This adds a new config option, per the 'elasticsearch_capture_body_urls' cross-agent spec, to control for which ES client requests the request body is captured. By default it is just the body of search-related APIs. The use case for configuring this is to capture for *all* requests for reply in Kibana performance work. This option is not expected to be used often.

apmmachine · 2022-08-12T20:47:26Z

💚 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: 2022-08-12T23:15:21.852+0000
Duration: 34 min 28 sec

Test stats 🧪

Test	Results
Failed	0
Passed	260042
Skipped	0
Total	260042

🤖 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-08-12T23:14:51Z

run module tests for elasticsearch,@elastic/elasticsearch

trentm · 2022-08-15T19:31:44Z

some premature optimization perf notes

I digressed slightly to sanity check that the change in ES URL path checking here (from a custom pathIsAQuery regexp, to a WildcardMatcher-created array of regexps) will not be a performance problem.

tl;dr: The WildcardMatcher-based comparison is much slower. However, it is on the order of 1 microsecond per path check on my MacBook Pro (2.4 GHz 8-Core Intel Core i9). Given that ES queries are remote calls and on the order of milliseconds, it would generally be premature optimization to bother getting fancy to improve performance of this.

details

Here is my benchmark script: https://gist.github.com/trentm/ba426e5a08317b18d40e897aeb536c0c

And a run:

% node bench-shouldCaptureBody.js
WilcardMatcher array:
	[/^.*\/_search$/i, /^.*\/_search\/template$/i, /^.*\/_msearch$/i, /^.*\/_msearch\/template$/i, /^.*\/_async_search$/i, /^.*\/_count$/i, /^.*\/_sql$/i, /^.*\/_eql\/search$/i]
Current pathIsAQuery RegExp: /\/(_search|_msearch|_count|_async_search|_sql|_eql)(\/|$)/
Naive WildcardMatcher in a single RegExp:
	/^(.*\/_search|.*\/_search\/template|.*\/_msearch|.*\/_msearch\/template|.*\/_async_search|.*\/_count|.*\/_sql|.*\/_eql\/search)$/i
customEquivReIgnoreCase:
	/\/(_search|_search\/template|_msearch|_msearch\/template|_async_search|_count|_sql|_eql\/search)$/i
customEquivRe:
	/\/(_search|_search\/template|_msearch|_msearch\/template|_async_search|_count|_sql|_eql\/search)$/

WildcardMatcher array x 719,208 ops/sec ±0.99% (90 runs sampled)
Current pathIsAQuery RegExp x 15,700,534 ops/sec ±0.97% (90 runs sampled)
Naive WildcardMatcher in a single RegExp x 856,094 ops/sec ±1.10% (88 runs sampled)
customEquivReIgnoreCase x 17,477,871 ops/sec ±1.37% (88 runs sampled)
customEquivRe x 17,942,769 ops/sec ±0.91% (88 runs sampled)
Fastest is customEquivRe

The WildcardMatcher-based array of regexps that this PR changes to is ~20x slower than the current pathIsAQuery RegExp.
A naive potential optimization might be to make a WildcardMatcher array config into a single regexp -- ^(pattern1|pattern2|...)$. This turns out to be not much faster, in this case.
The major perf difference is that the (a) the WildcardMatcher necessarily always has a ^ anchor and (b) every pattern is leading with * to avoid that anchor. A possible optimization of WildcardMatcher usage would be to drop the ^ anchor and the leading * if and only if every pattern had that leading *. An equivalent optimization could be done for the '$' anchor and a trailing * on every pattern.
There is a small perf win with dropping the case-insensitive flag. Arguably the default elasticsearch_capture_body_urls values could/should be case-sensitive. I don't believe the Elasticsearch REST API would accept POST /my-index/_SeArCh.

However, this is all fast enough, so I'm not proposing doing any WildcardMatcher perf improvements right now.

trentm · 2022-08-17T20:00:29Z

@astorm the spec for this will get merged tomorrow, so feel free to review this when you get a chance. Thanks.

astorm · 2022-08-18T18:16:30Z

examples/trace-elasticsearch7.js

@@ -18,6 +18,8 @@ const apm = require('../').start({ // elastic-apm-node
  logUncaughtExceptions: true
 })

+// Note that version *8* is installed by default. To use v7 you'll need to:


nit: Might be better to say version 7 is not installed by default instead of Note that version 8 is installed by default, since the later will inevitably be not true one day but the former will almost certainly be true into the future.

Sounds good to me, thanks.

Note, if interested: For a while I'd had both v7 and v8 installed using an alias for v7 -- "@elastic/elasticsearch7": "npm:@elastic/elasticsearch@7.x". However the default npm in node v8 doesn't support that alias syntax, so I dropped it.

commit f8631c0

astorm · 2022-08-18T18:17:27Z

index.d.ts

@@ -249,6 +249,7 @@ declare namespace apm {
    contextPropagationOnly?: boolean;
    disableInstrumentations?: string | string[];
    disableSend?: boolean;
+    elasticsearchCaptureBodyUrls?: Array<string>;


No objection, but why Array<string> instead of string[]?

Honestly just copying syntax from elsewhere in this file. https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#arrays says both are fine and doesn't claim a preference, so I'm fine with either way. Both are used in index.d.ts.

astorm · 2022-08-18T18:22:43Z

lib/instrumentation/elasticsearch-shared.js

@@ -10,64 +10,50 @@
 // - elasticsearch - the legacy Elasticsearch JS client
 // - @elastic/elasticsearch - the new Elasticsearch JS client

-const querystring = require('querystring')


👍 dings the "two require('querystring') references left before we can drop another dep." bell 🛎️

apmmachine · 2022-08-19T00:10:21Z

💚 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: 2022-08-18T23:49:18.790+0000
Duration: 21 min 41 sec

Test stats 🧪

Test	Results
Failed	0
Passed	260042
Skipped	0
Total	260042

🤖 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-10-19T15:30:11Z

elastic-apm-node@3.39.0 is release with this now

This adds a new config option, per the 'elasticsearch_capture_body_urls' cross-agent spec, to control for which ES client requests the request body is captured. By default it is just the body of search-related APIs. The use case for configuring this is to capture for *all* requests for reply in Kibana performance work. This option is not expected to be used often.

trentm self-assigned this Aug 12, 2022

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

trentm added 8 commits August 12, 2022 14:05

enable capturing ES span.context.http.url even with contextManager=patch

f7e21b4

Merge branch 'main' into trentm/elasticsearch_capture_body_urls

0452ebc

refactor setElasticsearchDbContext -> getElasticsearchDbStatement

8001549

cannot use dep aliases with the npm in node v8.6; just don't bother

19b7c7b

test fix to use that actual configured host (different in Docker setup)

29bd7e9

configuration doc entry; config.test.js additions

87dbe9b

Merge branch 'main' into trentm/elasticsearch_capture_body_urls

df0b9e7

changelog entry

e5f8e68

trentm marked this pull request as ready for review August 13, 2022 00:06

trentm requested a review from astorm August 13, 2022 00:07

trentm added the performance label Aug 15, 2022

AlexanderWert linked an issue Aug 16, 2022 that may be closed by this pull request

[META 672] Add option for when to capture the Elasticsearch query #2880

Closed

astorm approved these changes Aug 18, 2022

View reviewed changes

future-proof comment suggestion from alan

f8631c0

trentm merged commit ee08f45 into main Aug 18, 2022

trentm deleted the trentm/elasticsearch_capture_body_urls branch August 18, 2022 23:54

dmlemeshko mentioned this pull request Sep 1, 2022

[performance] add support for 'elasticsearchCaptureBodyUrls' APM config option elastic/kibana#139944

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 'elasticsearchCaptureBodyUrls' config option #2873

feat: 'elasticsearchCaptureBodyUrls' config option #2873

trentm commented Aug 12, 2022 •

edited

Loading

apmmachine commented Aug 12, 2022 •

edited

Loading

Build stats

Test stats 🧪

trentm commented Aug 12, 2022

trentm commented Aug 15, 2022

trentm commented Aug 17, 2022

astorm Aug 18, 2022

trentm Aug 18, 2022

trentm Aug 18, 2022

astorm Aug 18, 2022

trentm Aug 18, 2022

astorm Aug 18, 2022

apmmachine commented Aug 19, 2022

Build stats

Test stats 🧪

trentm commented Oct 19, 2022

feat: 'elasticsearchCaptureBodyUrls' config option #2873

feat: 'elasticsearchCaptureBodyUrls' config option #2873

Conversation

trentm commented Aug 12, 2022 • edited Loading

checklist

apmmachine commented Aug 12, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

trentm commented Aug 12, 2022

trentm commented Aug 15, 2022

some premature optimization perf notes

details

trentm commented Aug 17, 2022

astorm Aug 18, 2022

Choose a reason for hiding this comment

trentm Aug 18, 2022

Choose a reason for hiding this comment

trentm Aug 18, 2022

Choose a reason for hiding this comment

astorm Aug 18, 2022

Choose a reason for hiding this comment

trentm Aug 18, 2022

Choose a reason for hiding this comment

astorm Aug 18, 2022

Choose a reason for hiding this comment

apmmachine commented Aug 19, 2022

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

trentm commented Oct 19, 2022

trentm commented Aug 12, 2022 •

edited

Loading

apmmachine commented Aug 12, 2022 •

edited

Loading