[ML] APM Latency Correlations: Fix empty state #109813

walterra · 2021-08-24T11:05:21Z

Summary

Fixes #109608.
Fixes #109879.

Correctly renders the empty chart state when no data is available.
Hides the "Click drag to select" and trace samples message when the chart shows an empty state to avoid redundant info.
Adds jest unit tests that would fail with the previously visible loading indicators.

Checklist

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

elasticmachine · 2021-08-24T11:46:42Z

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

elasticmachine · 2021-08-24T11:46:43Z

Pinging @elastic/ml-ui (:ml)

peteharverson

Tested and LGTM.

Not related, but I think the text in the tooltips for the Failure and Success columns in the Failed transactions tabs needs an edit to replace appear with appears.

walterra · 2021-08-25T08:55:02Z

@peteharverson Fixed the tooltip text in ca86f76. Also fixed #109879 in cc7f8f1, the code changes would be overlapping if doing it in a separate PR.

formgeist · 2021-08-25T09:44:10Z

@walterra Noticed a small change in the empty prompt where the word-break is now showing the two paragraphs too far apart. I think we need to remove the 2 <p>'s and instead just add a <br /> in between. Can you include this fix in this PR?

Current

<EuiText size="s">
              <p>
                <FormattedMessage
                  id="xpack.apm.correlations.noCorrelationsTextLine1"
                  defaultMessage="Correlations will only be identified if they have significant impact."
                />
              </p>
              <p>
                <FormattedMessage
                  id="xpack.apm.correlations.noCorrelationsTextLine2"
                  defaultMessage="Try selecting another time range or remove any added filter."
                />
              </p>
            </EuiText>

Proposed

<EuiText size="s">
              <p>
                <FormattedMessage
                  id="xpack.apm.correlations.noCorrelationsTextLine1"
                  defaultMessage="Correlations will only be identified if they have significant impact."
                />
                <br />
                <FormattedMessage
                  id="xpack.apm.correlations.noCorrelationsTextLine2"
                  defaultMessage="Try selecting another time range or remove any added filter."
                />
              </p>
            </EuiText>

walterra · 2021-08-25T09:51:37Z

@formgeist good catch! fixed in 137b14e.

peteharverson

Tested latest changes and LGTM

This reverts commit ca86f76.

sorenlouv · 2021-08-25T21:40:10Z

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

@@ -95,7 +95,7 @@ export function FailedTransactionsCorrelations({
  const startFetchHandler = useCallback(() => {
    startFetch(searchServicePrams);
    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [environment, serviceName, kuery, start, end]);
+  }, [environment, serviceName, transactionType, kuery, start, end]);


This bug occurred because we have eslint suppress errors. Why do we do that in the first place? Can we remove it?
Also: looking at this it looks like we are still missing transactionName

Improved in 34e882c. We originally had it there because the callback startFetch/cancelFetch were reinstantiated on every hook call. I wrapped them in useCallback so that doesn't happen anymore.

sorenlouv · 2021-08-25T21:43:37Z

x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx

@@ -117,11 +132,10 @@ export function TransactionDistribution({

    return () => {
      // cancel any running async partial request when unmounting the component
-      // we want this effect to execute exactly once after the component mounts
      cancelFetch();
    };
    // eslint-disable-next-line react-hooks/exhaustive-deps


Same as above: can we remove the lint supression?

We have to keep it here because isRunning is in there as a safety measure to cancel a still running service on a refresh, but don't want to trigger on every isRunning change. I added a comment to make it more clear.

sorenlouv · 2021-08-25T21:46:47Z

x-pack/plugins/apm/public/components/shared/charts/transaction_distribution_chart/index.tsx

+  );
+
+  useEffect(() => {
+    if (typeof onHasData === 'function') {


Isn't this typed so it can only be () => void | undefined or something like that? In that case we know it's a function if it's not undefined:

Suggested change

if (typeof onHasData === 'function') {

if (onHasData) {

Usually I try to write these ifs similar to "How would we write this if we'd move it to a type guard?" to be a bit more strict at runtime. It won't make much a difference in this case of course because it's just us consuming the component and it's not dynamic data.

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

sorenlouv · 2021-08-26T09:42:01Z

As a general comment: it is somtimes hard to see where the separation between Kibana Search service logic and the correlation business logic.

I noticed a lot of duplication between the following files:

Data fetchers

x-pack/plugins/apm/public/hooks/use_failed_transactions_correlations_fetcher.ts
x-pack/plugins/apm/public/hooks/use_transaction_latency_correlations_fetcher.ts
x-pack/plugins/apm/public/hooks/use_transaction_distribution_fetcher.ts

Search strategies

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

Search services

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

Having some generic abstractions on top of Kibana Search service would go along way. We both need it on the server, and something similar to useFetcher on the client.

walterra · 2021-08-26T09:53:02Z

Because we worked in parallel on similar features in separate PRs, we decided to go with duplicate code for 7.15 to make FF and avoid running out of sync or put a burden on us with more complex merge conflicts. Now that everything is in master there's definitely room for a lot of deduplication.

…s hit before retrieving any data.

peteharverson

Tested latest edits and LGTM

walterra · 2021-08-27T08:52:24Z

@elasticmachine merge upstream

kibanamachine · 2021-08-27T11:08: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.4MB	4.4MB	+2.1KB

History

💚 Build #148860 succeeded f4c3c5f
💚 Build #148834 succeeded cd871b2
💚 Build #148814 succeeded 34e882c
💚 Build #148494 succeeded 137b14e
💚 Build #148472 succeeded cc7f8f1

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

cc @walterra

- Correctly renders the empty chart state when no data is available. - Hides the "Click drag to select" and trace samples message when the chart shows an empty state to avoid redundant info. - Adds jest unit tests that would fail with the previously visible loading indicators. - Fix a bug with cancelling search strategies.

kibanamachine · 2021-08-27T11:11:29Z

💔 Backport failed

Status	Branch	Result
❌	7.15	Commit could not be cherrypicked due to conflicts
✅	7.x

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 109813

- Correctly renders the empty chart state when no data is available. - Hides the "Click drag to select" and trace samples message when the chart shows an empty state to avoid redundant info. - Adds jest unit tests that would fail with the previously visible loading indicators. - Fix a bug with cancelling search strategies. Co-authored-by: Walter Rafelsberger <walter@elastic.co>

- Correctly renders the empty chart state when no data is available. - Hides the "Click drag to select" and trace samples message when the chart shows an empty state to avoid redundant info. - Adds jest unit tests that would fail with the previously visible loading indicators. - Fix a bug with cancelling search strategies. # Conflicts: # x-pack/plugins/apm/public/components/app/correlations/latency_correlations.tsx # x-pack/plugins/apm/public/components/app/transaction_details/distribution/index.tsx

…eporting-to-v2 * 'master' of github.com:elastic/kibana: (120 commits) [Lens] should register "suffix" field formatter in setup lifecycle (elastic#110218) skip flaky suite (elastic#98463) skip flaky suite (elastic#108633) [Data Table] Expensive queries are causing unnecessary load and delays on Elasticsearch (elastic#98903) fixes failing tests (elastic#110436) [TSVB] Remove deprecated `IFieldType` (elastic#110404) [Lens] Remove deprecated `IFieldType` (elastic#109825) [Vega] Expensive queries are causing unnecessary load and delays on Elasticsearch (elastic#99023) [Visualizations] Expensive queries are causing unnecessary load and delays on Elasticsearch (elastic#99031) [TSVB] Expensive queries are causing unnecessary load and delays on Elasticsearch (elastic#98914) Don't add split part of UI if we have one series (elastic#109483) [Discover] Migrate angular routing to react router (elastic#107042) [Security Solution][Endpoint][Event Filters] Fixes missing spacers between event filters cards (elastic#110282) [ML] Data Grid: Fix alignment of sorting arrow when histogram charts are enabled (elastic#110053) [canvas] Fix image argument form issues (elastic#109767) Fix asset in Pitch template (elastic#109742) chore(NA): moving @kbn/securitysolution-list-api to babel transpiler (elastic#110265) chore(NA): moving @kbn/securitysolution-list-constants to babel transpiler (elastic#110269) [Fleet] Fix upgrade link in Fleet policy table (elastic#110228) [ML] APM Latency Correlations: Fix empty state (elastic#109813) ... # Conflicts: # src/plugins/data/common/query/timefilter/types.ts

walterra added 2 commits August 23, 2021 18:33

fix chart state for empty results.

01e3a9a

unit tests

b30359d

walterra added bug Fixes for quality problems that affect the customer experience 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 apm:correlations v7.15.0 v7.16.0 labels Aug 24, 2021

walterra self-assigned this Aug 24, 2021

walterra added 2 commits August 24, 2021 13:41

[ML] Fix empty state for latency correlations.

6e21c1b

Merge branch 'master' into ml-apm-correlations-fix-empty-state

5a8bfd7

walterra marked this pull request as ready for review August 24, 2021 11:46

walterra requested a review from a team as a code owner August 24, 2021 11:46

walterra requested a review from peteharverson August 24, 2021 11:46

walterra mentioned this pull request Aug 24, 2021

[ML] Increase Test Coverage 7.15 #106113

Closed

14 tasks

[ML] Fix tooltip text.

ca86f76

peteharverson approved these changes Aug 24, 2021

View reviewed changes

walterra added 2 commits August 25, 2021 10:27

Merge branch 'master' into ml-apm-correlations-fix-empty-state

04bf2de

[Ml] Trigger reload for log log histogram and analysis.

cc7f8f1

[ML] Fix paragraph spacing.

137b14e

peteharverson approved these changes Aug 25, 2021

View reviewed changes

Revert "[ML] Fix tooltip text."

b17a0b9

This reverts commit ca86f76.

sorenlouv reviewed Aug 25, 2021

View reviewed changes

sorenlouv approved these changes Aug 25, 2021

View reviewed changes

walterra added 4 commits August 26, 2021 08:23

Merge branch 'master' into ml-apm-correlations-fix-empty-state

08df940

[ML] Fix typo.

22e1b9c

[ML] Fix latency chart legend label.

acc4caa

[ML] Improve useEffects deps.

34e882c

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

sorenlouv reviewed Aug 26, 2021

View reviewed changes

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

[ML] Fix useEffects.

cd871b2

[ML] Fix loading spinner of latency correlations chart when cancel wa…

f4c3c5f

…s hit before retrieving any data.

peteharverson approved these changes Aug 26, 2021

View reviewed changes

Merge branch 'master' into ml-apm-correlations-fix-empty-state

3952f7b

walterra enabled auto-merge (squash) August 27, 2021 08:53

walterra merged commit 54a45bb into elastic:master Aug 27, 2021

kibanamachine mentioned this pull request Aug 27, 2021

[7.x] [ML] APM Latency Correlations: Fix empty state (#109813) #110398

Merged

walterra mentioned this pull request Aug 27, 2021

[7.15] [ML] APM Latency Correlations: Fix empty state (#109813) #110399

Merged

walterra deleted the ml-apm-correlations-fix-empty-state branch August 27, 2021 11:18

sorenlouv mentioned this pull request Aug 27, 2021

[APM] Trace Samples error message shows while section is still loading #110166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] APM Latency Correlations: Fix empty state #109813

[ML] APM Latency Correlations: Fix empty state #109813

walterra commented Aug 24, 2021 •

edited

Loading

elasticmachine commented Aug 24, 2021

elasticmachine commented Aug 24, 2021

peteharverson left a comment

walterra commented Aug 25, 2021

formgeist commented Aug 25, 2021

walterra commented Aug 25, 2021

peteharverson left a comment

sorenlouv Aug 25, 2021

walterra Aug 26, 2021

sorenlouv Aug 25, 2021

walterra Aug 26, 2021

sorenlouv Aug 25, 2021

walterra Aug 26, 2021

sorenlouv commented Aug 26, 2021

walterra commented Aug 26, 2021

peteharverson left a comment

walterra commented Aug 27, 2021

kibanamachine commented Aug 27, 2021

kibanamachine commented Aug 27, 2021

[ML] APM Latency Correlations: Fix empty state #109813

[ML] APM Latency Correlations: Fix empty state #109813

Conversation

walterra commented Aug 24, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Aug 24, 2021

elasticmachine commented Aug 24, 2021

peteharverson left a comment

Choose a reason for hiding this comment

walterra commented Aug 25, 2021

formgeist commented Aug 25, 2021

walterra commented Aug 25, 2021

peteharverson left a comment

Choose a reason for hiding this comment

sorenlouv Aug 25, 2021

Choose a reason for hiding this comment

walterra Aug 26, 2021

Choose a reason for hiding this comment

sorenlouv Aug 25, 2021

Choose a reason for hiding this comment

walterra Aug 26, 2021

Choose a reason for hiding this comment

sorenlouv Aug 25, 2021

Choose a reason for hiding this comment

walterra Aug 26, 2021

Choose a reason for hiding this comment

sorenlouv commented Aug 26, 2021

walterra commented Aug 26, 2021

peteharverson left a comment

Choose a reason for hiding this comment

walterra commented Aug 27, 2021

kibanamachine commented Aug 27, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Aug 27, 2021

💔 Backport failed

walterra commented Aug 24, 2021 •

edited

Loading