[ML] Display advanced mode toggle for the APM failed transactions table #114363

qn895 · 2021-10-07T21:14:51Z

Summary

This PR adds a new toggle for the APM Failed transactions table to show the additional statistics. Previously, this was coupled with the Inspect ES query setting in the advanced setting.

Changes include:

The advanced statistics mode will be off by default
State is saved in LocalStorage which means the user will most likely have the view persist if they've chosen to switch it on
The mode will only enable the additional data columns to the table.
Removed the current p-value "tooltip" upon hovering a table row
The Logs content will still only show when the "Inspect ES queries" feature flag is enabled.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

formgeist

UX looks good to me 👍

peteharverson · 2021-10-11T13:16:14Z

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

+            paddingLeft: euiTheme.eui.paddingSizes.s,
+          }}
+        >
+          <EuiSwitch


@qn895 @formgeist is there any value in hiding the switch when no correlations are found?

@peteharverson I think the result would be that the switch would come and go which can be confusing too

peteharverson

Tested and LGTM.

Not changed in this PR, but just wondered if the p-value column would benefit from an info tooltip?

peteharverson

Just one minor comment on the text, but otherwise tested and LGTM

peteharverson · 2021-10-12T10:50:48Z

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

+                  'xpack.apm.correlations.failedTransactions.correlationsTable.pValueDescription',
+                  {
+                    defaultMessage:
+                      'The chance of seeing at least this count of field name & field value for failed transactions given its prevalence in successful transactions.',


Nit - I don't think we normally use the ampersand & in text, so I'd replace it with and.

If I understand the context correctly, I would change seeing to something else. I left a suggestion, please take or leave it.

Suggested change

'The chance of seeing at least this count of field name & field value for failed transactions given its prevalence in successful transactions.',

'The chance of getting at least this amount of field name and value for failed transactions given its prevalence in successful transactions.',

szabosteve

I left one suggestion, otherwise UI text LGTM!

szabosteve · 2021-10-12T14:28:50Z

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

+                  'xpack.apm.correlations.failedTransactions.correlationsTable.pValueDescription',
+                  {
+                    defaultMessage:
+                      'The chance of seeing at least this count of field name & field value for failed transactions given its prevalence in successful transactions.',


If I understand the context correctly, I would change seeing to something else. I left a suggestion, please take or leave it.

Suggested change

'The chance of seeing at least this count of field name & field value for failed transactions given its prevalence in successful transactions.',

'The chance of getting at least this amount of field name and value for failed transactions given its prevalence in successful transactions.',

elasticmachine · 2021-10-12T21:08:33Z

Pinging @elastic/ml-ui (:ml)

dgieselaar · 2021-10-13T08:22:00Z

x-pack/plugins/translations/translations/ja-JP.json

@@ -4912,6 +4912,16 @@
    "visTypeMetric.colorModes.backgroundOptionLabel": "背景",
    "visTypeMetric.colorModes.labelsOptionLabel": "ラベル",
    "visTypeMetric.colorModes.noneOptionLabel": "なし",
+    "visTypeMetric.metricDescription": "計算結果を単独の数字として表示します。",


I assume this is here because some labels were resorted, but it might make sense to undo the changes in these files to prevent merge conflicts (for you or others).

Fixed here 8477e29

elasticmachine · 2021-10-13T08:22:05Z

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

dgieselaar · 2021-10-13T08:22:42Z

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

+            )}
+            checked={showStats}
+            onChange={(e) => setShowStats(!showStats)}
+            compressed={true}


you can leave out ={true} here and just use compressed

Updated here 8477e29

dgieselaar · 2021-10-13T08:31:02Z

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

+              }
+            )}
+            checked={showStats}
+            onChange={(e) => setShowStats(!showStats)}


can you use a callback here? i.e. setShowState(prevShowStats => !prevShowStats). See https://reactjs.org/docs/hooks-reference.html#functional-updates

Unfortunately setShowStats is using useLocalStorage and not useState so this callback doesn't work here. We can modify useLocalStorage to support this behavior but I think that's outside the scope of this PR.

dgieselaar

Looks good, just a few nits. Thanks!

qn895 · 2021-10-14T14:26:53Z

@elasticmachine merge upstream

kibanamachine · 2021-10-14T16:29:26Z

💚 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`	2.7MB	2.7MB	+1.0KB

History

💚 Build #160508 succeeded 1c6eb50
💔 Build #160009 failed b3c2ae9
💔 Build #159693 failed 34a4887
💚 Build #159155 succeeded 431d4a1

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

cc @qn895

…le (elastic#114363) * Add new toggle * [ML] Add tooltip for p value * [ML] Add tooltip for p value * Update tooltip * Add callback, revert i18n, compressed Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-10-14T18:21:35Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…le (#114363) (#115073) * Add new toggle * [ML] Add tooltip for p value * [ML] Add tooltip for p value * Update tooltip * Add callback, revert i18n, compressed Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Quynh Nguyen <[email protected]>

Add new toggle

431d4a1

qn895 requested review from walterra, formgeist, peteharverson and smith October 7, 2021 21:15

qn895 self-assigned this Oct 7, 2021

qn895 added :ml apm:correlations apm:ml Integration between APM and ML release_note:enhancement v7.16.0 v8.0.0 labels Oct 7, 2021

formgeist approved these changes Oct 11, 2021

View reviewed changes

peteharverson reviewed Oct 11, 2021

View reviewed changes

peteharverson approved these changes Oct 11, 2021

View reviewed changes

qn895 added 3 commits October 11, 2021 14:27

[ML] Add tooltip for p value

bd7c55c

Merge remote-tracking branch 'upstream/master' into ml-apm-toggle-button

0c920fc

[ML] Add tooltip for p value

34a4887

peteharverson approved these changes Oct 12, 2021

View reviewed changes

szabosteve approved these changes Oct 12, 2021

View reviewed changes

qn895 added 2 commits October 12, 2021 09:31

Update tooltip

258c926

Merge remote-tracking branch 'upstream/master' into ml-apm-toggle-button

b3c2ae9

qn895 marked this pull request as ready for review October 12, 2021 21:08

qn895 requested a review from a team as a code owner October 12, 2021 21:08

dgieselaar reviewed Oct 13, 2021

View reviewed changes

botelastic bot added the Team:APM All issues that need APM UI Team support label Oct 13, 2021

dgieselaar reviewed Oct 13, 2021

View reviewed changes

dgieselaar approved these changes Oct 13, 2021

View reviewed changes

qn895 added 2 commits October 13, 2021 09:49

Add callback, revert i18n, compressed

8477e29

Merge remote-tracking branch 'upstream/master' into ml-apm-toggle-button

1c6eb50

Merge branch 'master' into ml-apm-toggle-button

612843e

qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 14, 2021

qn895 merged commit 35da323 into elastic:master Oct 14, 2021

kibanamachine mentioned this pull request Oct 14, 2021

[7.x] [ML] Display advanced mode toggle for the APM failed transactions table (#114363) #115073

Merged

qn895 deleted the ml-apm-toggle-button branch October 15, 2021 16:25

walterra mentioned this pull request Oct 18, 2021

[APM] Latency correlations 7.16 meta issue #109220

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Display advanced mode toggle for the APM failed transactions table #114363

[ML] Display advanced mode toggle for the APM failed transactions table #114363

qn895 commented Oct 7, 2021

formgeist left a comment

peteharverson Oct 11, 2021

formgeist Oct 11, 2021

peteharverson left a comment

peteharverson left a comment

peteharverson Oct 12, 2021

szabosteve Oct 12, 2021

szabosteve left a comment

szabosteve Oct 12, 2021

elasticmachine commented Oct 12, 2021

dgieselaar Oct 13, 2021

qn895 Oct 14, 2021

elasticmachine commented Oct 13, 2021

dgieselaar Oct 13, 2021

qn895 Oct 14, 2021

dgieselaar Oct 13, 2021

qn895 Oct 13, 2021

dgieselaar left a comment

qn895 commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

	'The chance of seeing at least this count of field name & field value for failed transactions given its prevalence in successful transactions.',
	'The chance of getting at least this amount of field name and value for failed transactions given its prevalence in successful transactions.',

[ML] Display advanced mode toggle for the APM failed transactions table #114363

[ML] Display advanced mode toggle for the APM failed transactions table #114363

Conversation

qn895 commented Oct 7, 2021

Summary

Checklist

formgeist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szabosteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar left a comment

Choose a reason for hiding this comment

qn895 commented Oct 14, 2021

kibanamachine commented Oct 14, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Oct 14, 2021

💚 Backport successful