[ML] APM Latency Correlations: Code consolidation. by walterra · Pull Request #110790 · elastic/kibana

walterra · 2021-09-01T12:24:39Z

Summary

Code deduplication:
- combine 3 existing client side hooks into useSearchStrategy
- combine server side multiple search strategy files into shared search_strategy_provider.ts
Clarified naming (client/server params, strategy naming etc.), improved types.
None of the actual deeper internal logic changed, larger chunks of code that show up as new lines is mostly just moved code + additional types (e.g. function overloads) to support the different search strategies with the same server side code and client side hook.

useSearchStrategy uses generic types and function overloads to provide types for parameters and the response based on the strategy name.

  const { progress, response, startFetch, cancelFetch } = useSearchStrategy(
    APM_SEARCH_STRATEGIES.APM_FAILED_TRANSACTIONS_CORRELATIONS
  );

  const { progress, response, startFetch, cancelFetch } = useSearchStrategy(
    APM_SEARCH_STRATEGIES.APM_LATENCY_CORRELATIONS,
    {
      percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD,
      analyzeCorrelations: true,
    }
  );

In the examples above, both are valid TS, based on the strategy name the first one doesn't require any additional parameters whereas the second one does. The client and server code share the same types defined in plugins/apm/common/search_strategies/* so it only has to be defined once.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

This reverts commit ca86f76.

x-pack/plugins/apm/server/lib/search_strategies/queries/query_histograms_generator.ts

sorenlouv · 2021-09-06T23:47:23Z

...rver/lib/search_strategies/latency_correlations/latency_correlations_search_service_state.ts

      progress,
-      values,
+      latencyCorrelations,
    };


Instead of:

return { ccsWarning, error, isCancelled, isRunning, overallHistogram, percentileThresholdValue, progress, latencyCorrelations, };

What about having a consistent interface for all kibana search services like:

return { error, status, data };

status can perhaps borrow the status from useFetcher and data can contain the rest.

I'm also not sure about the top level progress. It seems like we are doing three separate calls. They could each have their own status

data: { overallHistogram: { data, status, progress }, percentileThresholdValue: { data, status, progress }, latencyCorrelations: { data, status, progress } }

That code is an implementation detail of the async code that fetches results. In the code that manages the Kibana search strategy we have then to stick to the format Kibana's search strategies are expecting, that is:

{ id: string; loaded: number; total: number; isRunning: boolean; isPartial: boolean; rawResponse: T; }

So Kibana's Search strategy is expecting one total progress. On the client side, useStrategy will receive and split it into progress (loaded, total, isRunning, is Partial) and response (rawResponse) to be more in line with the structure of the rest of APM's code.

sorenlouv · 2021-09-06T23:48:09Z

x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx

+      {response.ccsWarning && (
        <>
          <EuiSpacer size="m" />
          <CrossClusterSearchCompatibilityWarning version="7.15" />


This is 7.14 for latency correlations. Is that on purpose?

Yes this one is on purpose, latency correlations uses ES aggregation that are available since 7.14, but failed transaction correlations uses ES aggs that were only introduced in 7.15.

And this is only a problem when the user is using ccs? It works if they use a local cluster?

In the Kibana UI the updated feature with this functionality is not part of 7.14, so if the local cluster + Kibana is 7.14, they won't run into a problem, the updated feature is simply not there. If the local cluster + Kibana is 7.15 with the updated feature, a cross cluster search against 7.14- will return errors, that's when we show the callout.

peteharverson

Latest changes LGTM. Also gave it a quick test.

x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx

sorenlouv · 2021-09-07T09:19:23Z

x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx

            significantTerms={correlationTerms}
-            status={isRunning ? FETCH_STATUS.LOADING : FETCH_STATUS.SUCCESS}
+            status={
+              progress.isRunning ? FETCH_STATUS.LOADING : FETCH_STATUS.SUCCESS


I'm wondering if we should just apply this status server side?

isRunning is part of Kibana's search strategy contract, we cannot replace it, we could just add our own loading logic in addition on top of Kibana's customizable rawResponse which I'd like to avoid.

x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx

sorenlouv · 2021-09-07T12:18:27Z

x-pack/plugins/apm/public/components/app/correlations/failed_transactions_correlations.tsx

  const selectedTerm = useMemo(() => {
    if (selectedSignificantTerm) return selectedSignificantTerm;
-    return result?.values &&
-      Array.isArray(result.values) &&
-      result.values.length > 0
-      ? result?.values[0]
+    return Array.isArray(response.failedTransactionsCorrelations) &&
+      response.failedTransactionsCorrelations.length > 0
+      ? response.failedTransactionsCorrelations[0]
      : undefined;
-  }, [selectedSignificantTerm, result]);
+  }, [selectedSignificantTerm, response.failedTransactionsCorrelations]);


Afaict this doesn't need to be memoized:

const selectedTerm = selectedSignificantTerm ?? response.failedTransactionsCorrelations?.[0]

Updated in b665413.

sorenlouv · 2021-09-07T12:30:21Z

x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx

          direction: sortDirection,
        },
-      } as EuiTableSortingType<MlCorrelationsTerms>,
+      } as EuiTableSortingType<LatencyCorrelation>,


nit: afaict there's no need to memoize sorting.

const sorting = { sort: { field: sortField, direction: sortDirection }, } as EuiTableSortingType<LatencyCorrelation>; const histogramTerms = useMemo( () => orderBy(response.latencyCorrelations ?? [], sortField, sortDirection), [response.latencyCorrelations, sortField, sortDirection] );

Updated in 6076fbf.

kibanamachine · 2021-09-07T15:17:12Z

💚 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.4MB	4.4MB	-9.8KB

History

💚 Build #151427 succeeded 787d9d7
💛 Build #151354 was flaky c52158b
💚 Build #151293 succeeded 1709e11
💛 Build #151286 was flaky 1656d8d
💚 Build #150895 succeeded 83c2b74

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

cc @walterra

sorenlouv · 2021-09-07T21:39:00Z

Thanks for all the improvements! Code is much more readable now. I'm sure I can keep nitpicking forever but this is a great improvement as-is.

Code deduplication: - combine 3 existing client side hooks into useSearchStrategy - combine server side multiple search strategy files into shared search_strategy_provider.ts - Clarified naming (client/server params, strategy naming etc.), improved types. - None of the actual deeper internal logic changed, larger chunks of code that show up as new lines is mostly just moved code + additional types (e.g. function overloads) to support the different search strategies with the same server side code and client side hook.

kibanamachine · 2021-09-08T07:50:03Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (61 commits) [Logs UI] Fix alert previews for thresholds of `0` (elastic#111150) [Archive Migration][Partial] discover apps-discover (elastic#110437) [APM] Set start date of APM ML job to -4 weeks (elastic#111375) [ML] APM Latency Correlations: Code consolidation. (elastic#110790) [Discover] Fix indices permission for multiline test (elastic#111284) [Detection Rules] Add 7.15 rules (elastic#111464) [Security Solution][Endpoint][Host Isolation] Hide isolate host option in alert details rather than disabling (elastic#111064) React version of angular license view (elastic#111317) [APM] Fix link in readme (elastic#111362) [Security Solution] add agent field to generator (elastic#111428) [Dashboard] Retain Tags on Quicksave (elastic#111015) Reorder App Search ingestion methods (elastic#111361) Port performance docs to new docs system. (elastic#111063) [Security Solution][RAC] Fixes updatedAt loading bug (elastic#111010) [sample data] update web log geo.src field to match country code of geo.coordinates (elastic#110885) [Security solution] [Endpoint] Fix bad artifact migration (elastic#111294) Fix copy typo. (elastic#111203) [build] Remove empty optimize directory (elastic#111393) [Maps] fix term join not updating when editing right field (elastic#111030) [Fleet] Set default settings in component template instead of the index template (elastic#111197) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap # x-pack/plugins/reporting/public/management/report_listing.test.tsx

Code deduplication: - combine 3 existing client side hooks into useSearchStrategy - combine server side multiple search strategy files into shared search_strategy_provider.ts - Clarified naming (client/server params, strategy naming etc.), improved types. - None of the actual deeper internal logic changed, larger chunks of code that show up as new lines is mostly just moved code + additional types (e.g. function overloads) to support the different search strategies with the same server side code and client side hook. Co-authored-by: Walter Rafelsberger <walter@elastic.co>

Code deduplication: - combine 3 existing client side hooks into useSearchStrategy - combine server side multiple search strategy files into shared search_strategy_provider.ts - Clarified naming (client/server params, strategy naming etc.), improved types. - None of the actual deeper internal logic changed, larger chunks of code that show up as new lines is mostly just moved code + additional types (e.g. function overloads) to support the different search strategies with the same server side code and client side hook.

walterra added 13 commits August 24, 2021 17:21

[ML] Fix tooltip text.

ca86f76

Revert "[ML] Fix tooltip text."

b17a0b9

This reverts commit ca86f76.

Merge branch 'master' of https://github.com/elastic/kibana

c6afb7b

Merge branch 'master' of https://github.com/elastic/kibana

cc04280

[ML] Code consolidation.

7f39a6b

[ML] Fix naming inconsistencies.

547ae65

[ML] Fix naming inconsistencies.

64cc796

[ML] Fix naming inconsistencies.

ade9431

[ML] Consolidate hooks.

be265ee

[ML] Consolidate hooks.

013369b

[ML] Consolidate hooks.

cb6c9de

[ML] Use function overloads.

5244347

[ML] Fix naming inconsistencies.

9104784

walterra self-assigned this Sep 1, 2021

walterra added the release_note:skip Skip the PR/issue when compiling release notes label Sep 1, 2021

walterra mentioned this pull request Sep 1, 2021

[APM] Latency correlations 7.16 meta issue #109220

Closed

11 tasks

walterra added 14 commits September 1, 2021 15:43

[ML] Fix jest test.

9a4b99d

[ML] Fix chart loading behavior.

afbcd8b

[ML] Rename values to latencyCorrelations.

f131283

[ML] Clean up types.

c9b3040

[ML] Add function overloads.

ecd1d75

Merge branch 'master' into ml-apm-correlations-consolidation-1

89dfda2

[Ml] Update API integration tests.

130f498

Merge branch 'master' into ml-apm-correlations-consolidation-1

9ddde60

[ML] Rename values to failedTransactionsCorrelations.

1236eb6

[ML] Fix naming inconsistencies.

bfa89a1

[ML] Fix naming inconsistencies.

8724c98

[ML] Fix naming inconsistencies.

aa4f909

[ML] Fix jest test.

ac873d3

Merge branch 'master' into ml-apm-correlations-consolidation-1

d95cea7

walterra added 3 commits September 6, 2021 15:32

[ML] Fix typo.

a0f9d4f

[ML] Simplify mock.

1709e11

[ML] Refactor code that used type guards.

c52158b