Skip to content

added capturing of request body in Elasticsearch queries#1222

Merged
felixbarny merged 5 commits intoelastic:masterfrom
videnkz:issue-519-capture-elasticsearch-apis
Jun 25, 2020
Merged

added capturing of request body in Elasticsearch queries#1222
felixbarny merged 5 commits intoelastic:masterfrom
videnkz:issue-519-capture-elasticsearch-apis

Conversation

@videnkz
Copy link
Contributor

@videnkz videnkz commented Jun 9, 2020 •
edited by zube bot
Loading

closes #519
added capturing of request body in Elasticsearch queries: _msesarch, _count, _rollup_search, _msearch/template, _search/template
@ghost
Copy link

ghost commented Jun 9, 2020 •
edited by ghost
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1222 updated]

  • Start Time: 2020-06-25T08:30:16.260+0000

  • Duration: 39 min 21 sec

Test stats 🧪

Test Results
Failed 0
Passed 1429
Skipped 13
Total 1442

@videnkz videnkz force-pushed the issue-519-capture-elasticsearch-apis branch from 0104ca1 to 7083340 Compare June 9, 2020 19:10
felixbarny
felixbarny requested changes Jun 10, 2020
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this!
...ava/co/elastic/apm/agent/es/restclient/ElasticsearchRestClientInstrumentationHelperImpl.java Outdated
private static final Logger logger = LoggerFactory.getLogger(ElasticsearchRestClientInstrumentationHelperImpl.class);

public static final String SEARCH_QUERY_PATH_SUFFIX = "_search";
public static final String QUERY_REGEXP = ".*_((search|msearch)(\\/template)?|count)$";
Copy link
Member

@felixbarny felixbarny Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex is rather expensive. Better use WildcardMatchers instead. Arrays.asList(WildcardMatcher.valueOf("*_search", ...).

To match all, use WildcardMatcher::isAnyMatch
Copy link
Contributor Author

@videnkz videnkz Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done
...est/java/co/elastic/apm/agent/es/restclient/v6_4/AbstractEs6_4ClientInstrumentationTest.java Outdated

// Now update and re-search
reporter.reset();
// Do CountRequest
Copy link
Member

@felixbarny felixbarny Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test the different endpoints in different test methods.
Copy link
Contributor Author

@videnkz videnkz Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done
Copy link
Member

@felixbarny felixbarny Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was to create multiple @Test annotated methods for each type of query. Better to have multiple simple test cases than one big complex one so that the tests can be executed individually.
Copy link
Contributor Author

@videnkz videnkz Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed
@videnkz videnkz requested a review from felixbarny June 10, 2020 08:55
@videnkz videnkz force-pushed the issue-519-capture-elasticsearch-apis branch from 0104022 to 9d29fc4 Compare June 14, 2020 19:14
@videnkz videnkz force-pushed the issue-519-capture-elasticsearch-apis branch from 9d29fc4 to 4b78b82 Compare June 17, 2020 11:52
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2020

Codecov Report

Merging #1222 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1222      +/-   ##
============================================
- Coverage     59.37%   59.36%   -0.02%     
  Complexity       87       87              
============================================
  Files           354      354              
  Lines         16270    16276       +6     
  Branches       2269     2269              
============================================
+ Hits           9661     9662       +1     
- Misses         5952     5959       +7     
+ Partials        657      655       -2
Impacted Files Coverage Δ Complexity Δ
...sticsearchRestClientInstrumentationHelperImpl.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...pm/agent/profiler/asyncprofiler/AsyncProfiler.java 60.46% <0.00%> (-4.66%) 0.00% <0.00%> (ø%)
...o/elastic/apm/agent/profiler/SamplingProfiler.java 71.82% <0.00%> (ø) 0.00% <0.00%> (ø%)
...ic/apm/agent/profiler/collections/LongHashSet.java 18.65% <0.00%> (+0.39%) 0.00% <0.00%> (ø%)
...tic/apm/agent/report/ssl/TLSFallbackSSLSocket.java 37.60% <0.00%> (+1.70%) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6196fe5...4b78b82. Read the comment docs.
@videnkz videnkz force-pushed the issue-519-capture-elasticsearch-apis branch from 4b78b82 to 9b44485 Compare June 17, 2020 15:20
@felixbarny felixbarny merged commit c547d4d into elastic:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@videnkz @codecov-commenter @felixbarny