[ML] API integration tests for APM latency correlation. #104644

walterra · 2021-07-07T13:05:17Z

Summary

Follow up to #99905.

Fixes #105049 (updated how we pass on APM's params to the async search service)

Adds API integration tests for APM Latency Correlations code.

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

If the applied filters don't return any docs, we won't throw an error anymore. Instead, the async search service finishes early and just returns no results.
If for whatever reason the async search service throws an error, it will also set its state now to isRunning = false.
If the client triggers a request with a service ID we now make sure that async search service still exists. We throw an error if that service no longer exists. This avoids re-instantiating async search services when they've already finished or failed and for whatever reason a client triggers another request with the same ID.
Refactored requests to reuse APM's own getCorrelationsFilters(). We now require start/end to be set and it will be converted from ISO (client side) to epochmillis (server side) to be more in line with APM's existing code.
The async search service now creates a simple internal log. This gets exposed via the API and we assert it using the API tests. In the future, we might also expose it in the UI to allow for better problem investigation for users and support.

Checklist

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

elasticmachine · 2021-07-07T13:10:50Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2021-07-07T13:10:50Z

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

smith

Looks ok, but are the test failures related to the changes here? Is this ready for review or is there more work needed on making the tests pass?

smith · 2021-07-07T21:25:36Z

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

  if (resp.body.aggregations === undefined) {
    throw new Error(
-      'fetchTransactionDurationHistogramInterval failed, did not return aggregations.'
+      'fetchTransactionDurationHistogramRangesteps failed, did not return aggregations.'


Is it rangeSteps or rangesteps? The file name and function name and error messages don't agree.

Good catch! I renamed everything to be consistent rangeSteps and range_steps in 509a02d.

walterra · 2021-07-08T12:49:22Z

@smith Thanks for the review, I thought it was ready but I overlooked that the bugs I fixed as part of this PR caused the existing unit tests to fail. Fixed those in 509a02d. Also identified an issue with how we applied filters which should now be fixed but needed changing the assertions for API tests. I'll flip this to draft again until all tests pass.

walterra · 2021-07-09T09:23:07Z

Tests passed, this is ready for another look! @smith @qn895

qn895 · 2021-07-09T18:00:39Z

Tested and LGTM 🎉

walterra · 2021-07-12T07:17:57Z

I switched the dataset used for the tests from apm_8.0.0 to ml_8.0.0, this one is able to return an actual correlation based on the limited data.

kibanamachine · 2021-07-12T09:25:48Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	4.3MB	4.3MB	-4.0B

History

💚 Build #137451 succeeded 2fc2273
💔 Build #137246 failed 987f44b
💚 Build #137210 succeeded 5161477
💔 Build #137133 failed 509a02d
💔 Build #136969 failed 8250dd0

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

cc @walterra

Adds API integration tests for APM Latency Correlations code. Writing the tests surfaced some glitches fixed as part of this PR: - If the applied filters don't return any docs, we won't throw an error anymore. Instead, the async search service finishes early and just returns no results. - If for whatever reason the async search service throws an error, it will also set its state now to isRunning = false. - If the client triggers a request with a service ID we now make sure that async search service still exists. We throw an error if that service no longer exists. This avoids re-instantiating async search services when they've already finished or failed and for whatever reason a client triggers another request with the same ID. - Refactored requests to reuse APM's own getCorrelationsFilters(). We now require start/end to be set and it will be converted from ISO (client side) to epochmillis (server side) to be more in line with APM's existing code. - The async search service now creates a simple internal log. This gets exposed via the API and we assert it using the API tests. In the future, we might also expose it in the UI to allow for better problem investigation for users and support.

kibanamachine · 2021-07-13T20:48:59Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

The backport PRs will be merged automatically after passing CI.

…) (#105606) Adds API integration tests for APM Latency Correlations code. Writing the tests surfaced some glitches fixed as part of this PR: - If the applied filters don't return any docs, we won't throw an error anymore. Instead, the async search service finishes early and just returns no results. - If for whatever reason the async search service throws an error, it will also set its state now to isRunning = false. - If the client triggers a request with a service ID we now make sure that async search service still exists. We throw an error if that service no longer exists. This avoids re-instantiating async search services when they've already finished or failed and for whatever reason a client triggers another request with the same ID. - Refactored requests to reuse APM's own getCorrelationsFilters(). We now require start/end to be set and it will be converted from ISO (client side) to epochmillis (server side) to be more in line with APM's existing code. - The async search service now creates a simple internal log. This gets exposed via the API and we assert it using the API tests. In the future, we might also expose it in the UI to allow for better problem investigation for users and support. - Use 8.0.0 dataset instead of ml_8.0.0

… (#105719) Adds API integration tests for APM Latency Correlations code. Writing the tests surfaced some glitches fixed as part of this PR: - If the applied filters don't return any docs, we won't throw an error anymore. Instead, the async search service finishes early and just returns no results. - If for whatever reason the async search service throws an error, it will also set its state now to isRunning = false. - If the client triggers a request with a service ID we now make sure that async search service still exists. We throw an error if that service no longer exists. This avoids re-instantiating async search services when they've already finished or failed and for whatever reason a client triggers another request with the same ID. - Refactored requests to reuse APM's own getCorrelationsFilters(). We now require start/end to be set and it will be converted from ISO (client side) to epochmillis (server side) to be more in line with APM's existing code. - The async search service now creates a simple internal log. This gets exposed via the API and we assert it using the API tests. In the future, we might also expose it in the UI to allow for better problem investigation for users and support. - Use 8.0.0 dataset instead of ml_8.0.0

API integration tests for latency correlation.

96a1a60

walterra self-assigned this Jul 7, 2021

walterra marked this pull request as ready for review July 7, 2021 13:10

walterra requested a review from a team as a code owner July 7, 2021 13:10

walterra added :ml apm:correlations Team:APM All issues that need APM UI Team support labels Jul 7, 2021

walterra added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v7.15.0 v8.0.0 labels Jul 7, 2021

walterra requested a review from qn895 July 7, 2021 13:11

walterra added 2 commits July 7, 2021 18:02

fix unit tests.

48074a4

fix handling of params.

8250dd0

smith reviewed Jul 7, 2021

View reviewed changes

walterra added 3 commits July 8, 2021 09:43

fix unit tests.

509a02d

Merge branch 'master' into ml-apm-correlations-api-integration-tests

ea55c02

adjust api/unit test

5161477

walterra marked this pull request as draft July 8, 2021 12:50

walterra added 3 commits July 8, 2021 16:07

assert internal log.

987f44b

fix unit tests.

2fc2273

Merge branch 'master' into ml-apm-correlations-api-integration-tests

0ab7c76

walterra marked this pull request as ready for review July 9, 2021 09:22

Merge branch 'master' into ml-apm-correlations-api-integration-tests

bfb0313

qn895 approved these changes Jul 9, 2021

View reviewed changes

walterra added 2 commits July 12, 2021 09:15

dataset with correlations. fix how to return errors for correlation agg.

0598acf

Merge branch 'master' into ml-apm-correlations-api-integration-tests

8b00869

walterra mentioned this pull request Jul 12, 2021

[ML] Fix custom index name settings, functional tests for APM Latency Correlation. #105200

Merged

3 tasks

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

walterra enabled auto-merge (squash) July 12, 2021 11:21

walterra mentioned this pull request Jul 13, 2021

[ML] Increase Test Coverage 7.14 #97982

Closed

15 tasks

smith approved these changes Jul 13, 2021

View reviewed changes

walterra merged commit 5dc1c8f into elastic:master Jul 13, 2021

kibanamachine mentioned this pull request Jul 13, 2021

[7.14] [ML] API integration tests for APM latency correlation. (#104644) #105506

Closed

kibanamachine mentioned this pull request Jul 13, 2021

[7.x] [ML] API integration tests for APM latency correlation. (#104644) #105507

Closed

walterra mentioned this pull request Jul 14, 2021

[7.14] [ML] API integration tests for APM latency correlation. (#104644) #105606

Merged

walterra deleted the ml-apm-correlations-api-integration-tests branch July 15, 2021 08:26

walterra mentioned this pull request Jul 15, 2021

[7.x] [ML] API integration tests for APM latency correlation. (#104644) #105719

Merged

walterra mentioned this pull request Jul 19, 2021

[APM] Latency correlations: Error Failed to build correlative_value after last required field arrived #104890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] API integration tests for APM latency correlation. #104644

[ML] API integration tests for APM latency correlation. #104644

walterra commented Jul 7, 2021 •

edited

Loading

elasticmachine commented Jul 7, 2021

elasticmachine commented Jul 7, 2021

smith left a comment

smith Jul 7, 2021

walterra Jul 8, 2021

walterra commented Jul 8, 2021

walterra commented Jul 9, 2021

qn895 commented Jul 9, 2021

walterra commented Jul 12, 2021

kibanamachine commented Jul 12, 2021

kibanamachine commented Jul 13, 2021

[ML] API integration tests for APM latency correlation. #104644

[ML] API integration tests for APM latency correlation. #104644

Conversation

walterra commented Jul 7, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Jul 7, 2021

elasticmachine commented Jul 7, 2021

smith left a comment

Choose a reason for hiding this comment

smith Jul 7, 2021

Choose a reason for hiding this comment

walterra Jul 8, 2021

Choose a reason for hiding this comment

walterra commented Jul 8, 2021

walterra commented Jul 9, 2021

qn895 commented Jul 9, 2021

walterra commented Jul 12, 2021

kibanamachine commented Jul 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Jul 13, 2021

💚 Backport successful

walterra commented Jul 7, 2021 •

edited

Loading