[ML] Fractions API fix and jest unit tests for APM Latency Correlations. #103907

walterra · 2021-06-30T15:10:01Z

Summary

Follow up to #99905.

Adds jest unit tests for APM Latency Correlations code.

Writing the tests surfaced some minor glitches fixed as part of this PR:

Fixes a typo in the name for the fetchTransactionDurationPercentiles() function.
Avoids adding a @timestamp filter if neither start/end are set as parameters for getQueryWithParams().
Adds a check to only push to ranges arrays if it's length is already greater than 0.
Makes the check against from more strict otherwise it wouldn't be added as an attribute if 0.
Fixes progress calculation for field/value pair fetching.
Removes leading 0 from fractions since [ML] ensure the fractions and bucket doc count have the same lengths in bucket_count_ks_test elasticsearch#74624 got merged.
Removes deprecated use of track_total_hits.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-07-01T10:03:21Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2021-07-01T10:03:22Z

Pinging @elastic/apm-ui (Team:apm)

qn895 · 2021-07-01T16:13:59Z

x-pack/plugins/apm/server/lib/search_strategies/correlations/query_field_candidates.test.ts

+            },
+          },
+          size: 1000,
+          track_total_hits: true,


Looking at this again, I think we can remove this track_total_hits completely because we now use the totalDocCount from fetchTransactionDurationFractions for the histograms instead. This is something I should have removed in the last PR.

Good catch, removed it in f42dda2.

qn895 · 2021-07-01T16:27:17Z

x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.test.ts

+    | undefined;
+
+  // fetchTransactionDurationPercentiles
+  if (req?.body?.aggs?.transaction_duration_percentiles !== undefined) {


nit: can we do an assertion for req?.body?.aggs? before these if statements so we don't have to duplicate these checks again?

Refactored in 16ef560.

qn895 · 2021-07-01T16:33:42Z

Thanks for adding the unit tests. Changes LGTM 🎉 , just left a few minor comments.

smith

Thanks for adding these. We usually don't add () in descriptions when describing functions so you can take those out if you like.

smith · 2021-07-01T20:12:38Z

x-pack/plugins/apm/server/lib/search_strategies/correlations/get_query_with_params.test.ts

+import { getQueryWithParams } from './get_query_with_params';
+
+describe('correlations', () => {
+  describe('getQueryWithParams()', () => {


Suggested change

describe('getQueryWithParams()', () => {

describe('getQueryWithParams', () => {

Thanks, updated in 02a1fd2.

smith · 2021-07-01T20:17:10Z

x-pack/plugins/apm/server/lib/search_strategies/correlations/search_strategy.test.ts

+        );
+      });
+
+      it('performs a client search with params when no ID is provided', async () => {


I usually break these up if there's a when in there:

Suggested change

it('performs a client search with params when no ID is provided', async () => {

describe('when no ID is provided', () => {

it('performs a client search with params', async () => {

Updated in 44d5cc5.

kibanamachine · 2021-07-05T14:56:48Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💛 Build #135924 was flaky 2642aab
💔 Build #135918 failed acb70ac
💚 Build #135634 succeeded de28966

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

@timestamp

Adds jest unit tests for APM Latency Correlations code. Writing the tests surfaced some minor glitches fixed as part of this PR: - Fixes a typo in the name for the fetchTransactionDurationPercentiles() function. - Avoids adding a @timestamp filter if neither start/end are set as parameters for getQueryWithParams(). - Adds a check to only push to ranges arrays if it's length is already greater than 0. - Makes the check against from more strict otherwise it wouldn't be added as an attribute if 0. - Fixes progress calculation for field/value pair fetching. - Removes leading 0 from fractions since an ES update got merged. - Removes deprecated use of track_total_hits.

@timestamp

Adds jest unit tests for APM Latency Correlations code. Writing the tests surfaced some minor glitches fixed as part of this PR: - Fixes a typo in the name for the fetchTransactionDurationPercentiles() function. - Avoids adding a @timestamp filter if neither start/end are set as parameters for getQueryWithParams(). - Adds a check to only push to ranges arrays if it's length is already greater than 0. - Makes the check against from more strict otherwise it wouldn't be added as an attribute if 0. - Fixes progress calculation for field/value pair fetching. - Removes leading 0 from fractions since an ES update got merged. - Removes deprecated use of track_total_hits.

kibanamachine · 2021-07-05T15:00:00Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

The backport PRs will be merged automatically after passing CI.

@timestamp

Adds jest unit tests for APM Latency Correlations code. Writing the tests surfaced some minor glitches fixed as part of this PR: - Fixes a typo in the name for the fetchTransactionDurationPercentiles() function. - Avoids adding a @timestamp filter if neither start/end are set as parameters for getQueryWithParams(). - Adds a check to only push to ranges arrays if it's length is already greater than 0. - Makes the check against from more strict otherwise it wouldn't be added as an attribute if 0. - Fixes progress calculation for field/value pair fetching. - Removes leading 0 from fractions since an ES update got merged. - Removes deprecated use of track_total_hits. Co-authored-by: Walter Rafelsberger <walter@elastic.co>

@timestamp

Adds jest unit tests for APM Latency Correlations code. Writing the tests surfaced some minor glitches fixed as part of this PR: - Fixes a typo in the name for the fetchTransactionDurationPercentiles() function. - Avoids adding a @timestamp filter if neither start/end are set as parameters for getQueryWithParams(). - Adds a check to only push to ranges arrays if it's length is already greater than 0. - Makes the check against from more strict otherwise it wouldn't be added as an attribute if 0. - Fixes progress calculation for field/value pair fetching. - Removes leading 0 from fractions since an ES update got merged. - Removes deprecated use of track_total_hits. Co-authored-by: Walter Rafelsberger <walter@elastic.co>

@timestamp

Adds jest unit tests for APM Latency Correlations code. Writing the tests surfaced some minor glitches fixed as part of this PR: - Fixes a typo in the name for the fetchTransactionDurationPercentiles() function. - Avoids adding a @timestamp filter if neither start/end are set as parameters for getQueryWithParams(). - Adds a check to only push to ranges arrays if it's length is already greater than 0. - Makes the check against from more strict otherwise it wouldn't be added as an attribute if 0. - Fixes progress calculation for field/value pair fetching. - Removes leading 0 from fractions since an ES update got merged. - Removes deprecated use of track_total_hits.

walterra added Team:APM All issues that need APM UI Team support :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.14.0 apm:correlations labels Jun 30, 2021

walterra self-assigned this Jun 30, 2021

spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

walterra added the v7.14.0 label Jul 1, 2021

walterra mentioned this pull request Jul 1, 2021

[ML] Increase Test Coverage 7.14 #97982

Closed

15 tasks

walterra added 3 commits July 1, 2021 11:41

unit tests for APM correlations.

bcbaf25

remove leading 0 from fractions array.

700bd03

unit tests for fetch functions.

2642aab

walterra force-pushed the ml-apm-correlations-unit-tests branch from acb70ac to 2642aab Compare July 1, 2021 09:42

walterra marked this pull request as ready for review July 1, 2021 09:43

walterra requested a review from a team as a code owner July 1, 2021 09:43

walterra requested a review from qn895 July 1, 2021 09:44

qn895 reviewed Jul 1, 2021

View reviewed changes

qn895 approved these changes Jul 1, 2021

View reviewed changes

smith approved these changes Jul 1, 2021

View reviewed changes

walterra added 5 commits July 5, 2021 14:35

remove track_total_hits from field candidates query.

f42dda2

simpliy aggs check code.

16ef560

remove parenthesis.

02a1fd2

restructure tests.

44d5cc5

Merge branch 'master' into ml-apm-correlations-unit-tests

30c609e

walterra added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 5, 2021

walterra enabled auto-merge (squash) July 5, 2021 12:52

walterra added the bug Fixes for quality problems that affect the customer experience label Jul 5, 2021

walterra changed the title ~~[ML] Jest unit tests for APM Latency Correlations.~~ [ML] Bug fixes and jest unit tests for APM Latency Correlations. Jul 5, 2021

walterra changed the title ~~[ML] Bug fixes and jest unit tests for APM Latency Correlations.~~ [ML] Fractions API fix and jest unit tests for APM Latency Correlations. Jul 5, 2021

walterra merged commit 2a37ef8 into elastic:master Jul 5, 2021

kibanamachine mentioned this pull request Jul 5, 2021

[7.14] [ML] Jest unit tests for APM Latency Correlations. (#103907) #104313

Merged

kibanamachine mentioned this pull request Jul 5, 2021

[7.x] [ML] Jest unit tests for APM Latency Correlations. (#103907) #104314

Merged

walterra deleted the ml-apm-correlations-unit-tests branch July 6, 2021 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fractions API fix and jest unit tests for APM Latency Correlations. #103907

[ML] Fractions API fix and jest unit tests for APM Latency Correlations. #103907

walterra commented Jun 30, 2021 •

edited

Loading

elasticmachine commented Jul 1, 2021

elasticmachine commented Jul 1, 2021

qn895 Jul 1, 2021 •

edited

Loading

walterra Jul 5, 2021

qn895 Jul 1, 2021

walterra Jul 5, 2021

qn895 commented Jul 1, 2021

smith left a comment

smith Jul 1, 2021

walterra Jul 5, 2021

smith Jul 1, 2021

walterra Jul 5, 2021

kibanamachine commented Jul 5, 2021

kibanamachine commented Jul 5, 2021

	describe('getQueryWithParams()', () => {
	describe('getQueryWithParams', () => {

	it('performs a client search with params when no ID is provided', async () => {
	describe('when no ID is provided', () => {
	it('performs a client search with params', async () => {

[ML] Fractions API fix and jest unit tests for APM Latency Correlations. #103907

[ML] Fractions API fix and jest unit tests for APM Latency Correlations. #103907

Conversation

walterra commented Jun 30, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Jul 1, 2021

elasticmachine commented Jul 1, 2021

qn895 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

walterra Jul 5, 2021

Choose a reason for hiding this comment

qn895 Jul 1, 2021

Choose a reason for hiding this comment

walterra Jul 5, 2021

Choose a reason for hiding this comment

qn895 commented Jul 1, 2021

smith left a comment

Choose a reason for hiding this comment

smith Jul 1, 2021

Choose a reason for hiding this comment

walterra Jul 5, 2021

Choose a reason for hiding this comment

smith Jul 1, 2021

Choose a reason for hiding this comment

walterra Jul 5, 2021

Choose a reason for hiding this comment

kibanamachine commented Jul 5, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Jul 5, 2021

💚 Backport successful

walterra commented Jun 30, 2021 •

edited

Loading

qn895 Jul 1, 2021 •

edited

Loading