[ML] APM Latency Correlations: Code consolidation.

[walterra]

Summary

Part of #109220.

• 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

[sorenlouv]

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 }
}

[walterra]

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]

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

[walterra]

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.

[sorenlouv]

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

[walterra]

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.

[sorenlouv]

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

[walterra]

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.

[sorenlouv]

Afaict this doesn't need to be memoized:

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

[walterra]

Updated in b665413.

[sorenlouv]

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]
);

[walterra]

Updated in 6076fbf.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 6076fbf
• Storybooks Preview
• Documentation Changes

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]

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.

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.