[Uptime] Feature/80166 add waterfall flyout by dominiqueclarke · Pull Request #89449 · elastic/kibana

dominiqueclarke · 2021-01-27T17:06:38Z

Summary

This PR adds a flyout with a summary of data points for each resource. The flyout can be triggered on a sidebar click, a bar click, or a click on any part of the chart row.

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-01-27T17:06:40Z

Pinging @elastic/uptime (Team:uptime)

dominiqueclarke

Need to add telemetry and consider accessibility enhancements.

dominiqueclarke · 2021-01-27T17:07:49Z

...plugins/uptime/public/components/monitor/synthetics/step_detail/waterfall/data_formatting.ts

+
+  let formattedValue = formatValueForDisplay(value);
+
+  if (postFix) {


I could refactor this to add a default parameter of an empty string to post fix and then not have to have this if statement when postfix is undefined.

++, if we do this we should also consider that we'd be adding a new whitespace char that wouldn't have been there with the current solution. Not sure how much this matters.

Yeah you're right. I hadn't considered that the first time around. I think I'll leave it as is for now.

dominiqueclarke · 2021-01-27T17:08:00Z

...plugins/uptime/public/components/monitor/synthetics/step_detail/waterfall/data_formatting.ts

+  return formattedValue;
+};

+const getValueForOffset = (item: NetworkItem): number => {


These were just moved.

x-pack/plugins/uptime/public/components/monitor/synthetics/step_detail/waterfall/types.ts

dominiqueclarke · 2021-01-27T17:10:49Z

.../public/components/monitor/synthetics/step_detail/waterfall/waterfall_chart_wrapper.test.tsx

+import { WaterfallChartWrapper } from './waterfall_chart_wrapper';
+import { networkItems as mockNetworkItems } from './data_formatting.test';
+
+describe('WaterfallChartWrapper', () => {


The waterfall chart wrapper is the best place to do unit tests that are as integrated from a front end perspective as a possible. From this parent component I can test the interaction of the click on the sidebar to opening the flyout. I have no idea how to test canvas clicks for actual clicks on the chart, and a bit of research didn't provide immediate results. Let me know if you have any ideas.

dominiqueclarke · 2021-01-27T17:12:28Z

...lugins/uptime/public/components/monitor/synthetics/waterfall/components/waterfall_flyout.tsx

+
+import { useWaterfallContext } from '../context/waterfall_chart';
+
+export const DETAILS = i18n.translate('xpack.uptime.synthetics.waterfall.flyout.details', {


Is it preferred to put content in external content files, or within the component. Feel like I've seen it done both ways. Same with styled components.

My personal preference is to just keep them inline, but this is fine as well. I think I'm in a minority, most people feel a constant like this improves readability. If it's referenced in more than one place I tend to extract them to a centralized file.

I go back and forth between what I prefer. I feel like inline is often more readable and easier for maintenance, but external also helps for referencing it in RTL tests. Right now I'm mostly using whatever pattern is established in the file I'm working in at the time. I'd love to discuss it further later.

dominiqueclarke · 2021-01-27T17:13:20Z

x-pack/plugins/uptime/public/components/monitor/synthetics/waterfall/components/use_flyout.ts

+
+export const useFlyout = (metaData: WaterfallMetaData) => {
+  const [isFlyoutVisible, setIsFlyoutVisible] = useState(false);
+  const [flyoutData, setFlyoutData] = useState<WaterfallMetaDataEntry>({


I mean to change this to undefined instead of a object that matches the shape with null values.

dominiqueclarke · 2021-01-27T17:16:03Z

...plugins/uptime/public/components/monitor/synthetics/waterfall/components/waterfall_chart.tsx

          </EuiFlexItem>
        </EuiFlexGroup>
        {shouldRenderLegend && <Legend items={legendItems!} render={renderLegendItem!} />}
+        <WaterfallFlyout />


I should maybe defer the render to the Waterfall to the WaterfallChartWrapper like other renders.

paulb-elastic · 2021-01-29T09:37:56Z

The flyout looks good, great having this additional info available for each of the requests.

I seem to be seeing a different metric being given in the mouseover compared with the flyout for the connection time.

Where we already have this data being shown (in the mouseover on the chart, and the legend), can we make them use the same labels, for example TLS (instead of SSL), Waiting (TTFB) (instead of time to first byte)?

Also, will the header sections (certificate, request, response) be collapsible (like in the mockup), or just listed like this? It might also be worth putting the certificate header below the request and response (as they tend to be most valuable in that order, of req > resp > cert).

With the font being quite large for the URL, it still suffers the truncation we have elsewhere on the chart. Should we try and add an option to make it easier to get the URL (like described here)?

/cc @katrin-freihofner @drewpost

katrin-freihofner · 2021-01-29T09:43:36Z

Thank you @dominiqueclarke this is looking great! Regarding lines showing a numeric value like:

They are hard to read, could you please add a space between number and unit.
What do you think, should we right-align the numbers so they are easier to scan?
Also, is this the shared component? The same table that metrics and APM use?

paulb-elastic · 2021-01-29T10:49:21Z

@katrin-freihofner @dominiqueclarke
+1 from me on right aligning the numbers, as it does help distinguish larger/smaller values when scanning (with consistent decimal place precision).

dominiqueclarke · 2021-01-29T15:14:34Z

The flyout looks good, great having this additional info available for each of the requests.

I seem to be seeing a different metric being given in the mouseover compared with the flyout for the connection time.

Where we already have this data being shown (in the mouseover on the chart, and the legend), can we make them use the same labels, for example TLS (instead of SSL), Waiting (TTFB) (instead of time to first byte)?

Also, will the header sections (certificate, request, response) be collapsible (like in the mockup), or just listed like this? It might also be worth putting the certificate header below the request and response (as they tend to be most valuable in that order, of req > resp > cert).

With the font being quite large for the URL, it still suffers the truncation we have elsewhere on the chart. Should we try and add an option to make it easier to get the URL (like described here)?

/cc @katrin-freihofner @drewpost

Thanks for taking a look at this.

I will take a look at the connection time to confirm it's accurate
We can absolutely use the same label
Discussed with @katrin-freihofner and we decided to nix the collapsible element, as well as display the data in this table like this similar to metrics and APM. We can absolutely put certificates after the headers
I'd be happy to prioritize issue [Uptime] Waterfall to include option to link to full URL #87189 for 7.12 in a separate PR if that's desired

…-add-waterfall-flyout

paulb-elastic · 2021-01-29T16:15:30Z

Thanks @dominiqueclarke, let's make the full URL requirement for this to also be part of #87189 (I've added it in there as a requirement).

#87189 isn't currently down for 7.12. If we do do it, we may want @elastic/observability-design to check the existing mock up is ok as an implementation/ux before we do.

…om/dominiqueclarke/kibana into feature/80166-add-waterfall-flyout

dominiqueclarke · 2021-02-10T14:12:20Z

Thanks @dominiqueclarke for adding the 'open' icon too (implementing #87189)

One thing I notice is that the flyout window still appears to open even when clicking on the 'open' link. I'm not sure if this is intentional or not?

For reference, it also works well when we have error badges against the URL:

/cc @drewpost @katrin-freihofner

FYI this was resolved.

justinkambic · 2021-02-10T15:47:32Z

.../uptime/public/components/monitor/synthetics/step_detail/waterfall/waterfall_flyout.test.tsx


 describe('WaterfallFlyout', () => {
-  const flyoutData: WaterfallMetaDataEntry = {
+  const flyoutData: WaterfallMetadataEntry = {


Thanks for changing the metadata naming, we could've held off on it but it's nice to be cleared up 🙂

justinkambic · 2021-02-10T15:49:55Z

...ugins/uptime/public/components/monitor/synthetics/step_detail/waterfall/waterfall_flyout.tsx

          <EuiTitle size="s">
            <h2 id="flyoutTitle">
-              <MiddleTruncatedText text={url} url={url} />
+              <EuiFlexItem>


Out of curiosity, why EuiFlexItem outside of an EuiFlexGroup?

EuiFlexItem applies display flex, without any additional flex-grow properties, and flex-direction column. This is exactly what I need for this scenario. EuiFlexGroup applies display flex, additional flew-grow properties, and applies flex-direction row by default. These can be overwritten but seems like extra markup and html nodes.

Interesting i have never thought of using EuiFlexItem independently.

justinkambic

LGTM - smoke-tested with OBLT edge and it looks really good.

dominiqueclarke · 2021-02-11T13:42:47Z

@elasticmachine merge upstream

…om/dominiqueclarke/kibana into feature/80166-add-waterfall-flyout

kibanamachine · 2021-02-11T20:56:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a15a365

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`uptime`	590	593	+3

Async chunks

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

id	before	after	diff
`uptime`	909.2KB	925.8KB	+16.5KB

History

💚 Build #106121 succeeded 5734d9d
💚 Build #106031 succeeded f54b934
💛 Build #105367 was flaky 7406334
💔 Build #105348 failed 221701b5ff6f5bf33294366f3f712541f7193c38
💛 Build #105250 was flaky 35a39b8

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

* adjust network events * add metaData to data formatting * add useFlyout * adjust waterfall data types * adjust MiddleTruncatedText to use span instead of div * add flyout * adjust sidebar button style * update tests * convert content to use sentence case * pass onBarClick and onProjectionClick as WaterfallChart props * use undefined value for initial flyoutData state * add telemetry * adjust typo in get_network_events * adjust connection time * added space between value and units * adjust flyout spacing, rearrange certificates, and right align values * adjust flyout labels * add focus management support to flyout * improve performance with memoization * add external link to MiddleTruncatedText * update data_formatting function * remove EuiPortal * add moment mock to data_formatting test * adjust data_formatting * adjust network_events runtime types * remove extra space in test tile * toggle flyout on sidebar click * update styling and html for open in new tab resource link * rename metaData to metadata * adjust MiddleTruncatedText styling * adjust WaterfallFlyout heading * adjust waterfall sidebar item types * adjust SidebarItem onClick type * fix license header * align middle truncated text left * move flyout logic to a render prop for better composability * add ip to flyout * update label for bytes downloaded (compressed) * lowercase compressed Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-02-11T22:51:11Z

Backport result

{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"level":"info","message":"POST https://api.github.com/graphql (status: 200)"}
{"meta":{"labels":["Team:uptime","auto-backport","release_note:enhancement","v7.12.0","v8.0.0"],"branchLabelMapping":{"^v8.0.0$":"master","^v7.12.0$":"7.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"},"existingTargetPullRequests":[]},"level":"info","message":"Inputs when calculating target branches:"}
{"meta":["7.x"],"level":"info","message":"Target branches inferred from labels:"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm kibanamachine","stdout":"","stderr":"error: No such remote: 'kibanamachine'\n"},"level":"info","message":"exec error 'git remote rm kibanamachine':"}
{"meta":{"killed":false,"code":2,"signal":null,"cmd":"git remote rm elastic","stdout":"","stderr":"error: No such remote: 'elastic'\n"},"level":"info","message":"exec error 'git remote rm elastic':"}
{"level":"info","message":"Backporting [{\"sourceBranch\":\"master\",\"targetBranchesFromLabels\":[\"7.x\"],\"sha\":\"53f4d4840be38d8ec67ece6b1386b82ab9925954\",\"formattedMessage\":\"[Uptime] Feature/80166 add waterfall flyout (#89449)\",\"originalMessage\":\"[Uptime] Feature/80166 add waterfall flyout (#89449)\\n\\n* adjust network events\\r\\n\\r\\n* add metaData to data formatting\\r\\n\\r\\n* add useFlyout\\r\\n\\r\\n* adjust waterfall data types\\r\\n\\r\\n* adjust MiddleTruncatedText to use span instead of div\\r\\n\\r\\n* add flyout\\r\\n\\r\\n* adjust sidebar button style\\r\\n\\r\\n* update tests\\r\\n\\r\\n* convert content to use sentence case\\r\\n\\r\\n* pass onBarClick and onProjectionClick as WaterfallChart props\\r\\n\\r\\n* use undefined value for initial flyoutData state\\r\\n\\r\\n* add telemetry\\r\\n\\r\\n* adjust typo in get_network_events\\r\\n\\r\\n* adjust connection time\\r\\n\\r\\n* added space between value and units\\r\\n\\r\\n* adjust flyout spacing, rearrange certificates, and right align values\\r\\n\\r\\n* adjust flyout labels\\r\\n\\r\\n* add focus management support to flyout\\r\\n\\r\\n* improve performance with memoization\\r\\n\\r\\n* add external link to MiddleTruncatedText\\r\\n\\r\\n* update data_formatting function\\r\\n\\r\\n* remove EuiPortal\\r\\n\\r\\n* add moment mock to data_formatting test\\r\\n\\r\\n* adjust data_formatting\\r\\n\\r\\n* adjust network_events runtime types\\r\\n\\r\\n* remove extra space in test tile\\r\\n\\r\\n* toggle flyout on sidebar click\\r\\n\\r\\n* update styling and html for open in new tab resource link\\r\\n\\r\\n* rename metaData to metadata\\r\\n\\r\\n* adjust MiddleTruncatedText styling\\r\\n\\r\\n* adjust WaterfallFlyout heading\\r\\n\\r\\n* adjust waterfall sidebar item types\\r\\n\\r\\n* adjust SidebarItem onClick type\\r\\n\\r\\n* fix license header\\r\\n\\r\\n* align middle truncated text left\\r\\n\\r\\n* move flyout logic to a render prop for better composability\\r\\n\\r\\n* add ip to flyout\\r\\n\\r\\n* update label for bytes downloaded (compressed)\\r\\n\\r\\n* lowercase compressed\\r\\n\\r\\nCo-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>\",\"pullNumber\":89449,\"existingTargetPullRequests\":[]}] to 7.x"}

Backporting to 7.x:
{"level":"info","message":"Backporting via filesystem"}
{"level":"info","message":"Creating PR with title: \"[7.x] [Uptime] Feature/80166 add waterfall flyout (#89449)\". kibanamachine:backport/7.x/pr-89449 -> 7.x"}
{"level":"info","message":"POST /repos/elastic/kibana/pulls - 201 in 1312ms"}
{"level":"info","message":"Adding assignees to #91233: dominiqueclarke"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91233/assignees - 201 in 504ms"}
{"level":"info","message":"Adding labels: backport"}
{"level":"info","message":"POST /repos/elastic/kibana/issues/91233/labels - 200 in 470ms"}
View pull request: https://github.com/elastic/kibana/pull/91233

* adjust network events * add metaData to data formatting * add useFlyout * adjust waterfall data types * adjust MiddleTruncatedText to use span instead of div * add flyout * adjust sidebar button style * update tests * convert content to use sentence case * pass onBarClick and onProjectionClick as WaterfallChart props * use undefined value for initial flyoutData state * add telemetry * adjust typo in get_network_events * adjust connection time * added space between value and units * adjust flyout spacing, rearrange certificates, and right align values * adjust flyout labels * add focus management support to flyout * improve performance with memoization * add external link to MiddleTruncatedText * update data_formatting function * remove EuiPortal * add moment mock to data_formatting test * adjust data_formatting * adjust network_events runtime types * remove extra space in test tile * toggle flyout on sidebar click * update styling and html for open in new tab resource link * rename metaData to metadata * adjust MiddleTruncatedText styling * adjust WaterfallFlyout heading * adjust waterfall sidebar item types * adjust SidebarItem onClick type * fix license header * align middle truncated text left * move flyout logic to a render prop for better composability * add ip to flyout * update label for bytes downloaded (compressed) * lowercase compressed Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Dominique Clarke <doclarke71@gmail.com>

…yout (#246343) ## Summary Related to #245413 Removes the manual z-index: `euiZLevel5` override from the waterfall flyout component to ensure compatibility with the new EUI Flyout System. The override was lowering the flyout's z-index below EUI's default. This override is very old (2021) [PR](#89449), and I couldn't identify why. Keeping or removing that override didn't change anything in the UI.

dominiqueclarke added 9 commits January 26, 2021 09:26

adjust network events

6e21c96

add metaData to data formatting

bcd467e

add useFlyout

eaf931f

adjust waterfall data types

b9bc837

adjust MiddleTruncatedText to use span instead of div

3aa0566

add flyout

455d305

merge upstream master

3a8e1cf

adjust sidebar button style

2002865

update tests

b1680b3

dominiqueclarke added v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.12.0 labels Jan 27, 2021

dominiqueclarke requested a review from a team as a code owner January 27, 2021 17:06

dominiqueclarke commented Jan 27, 2021

View reviewed changes

dominiqueclarke marked this pull request as draft January 27, 2021 17:16

dominiqueclarke added the release_note:roadmap label Jan 27, 2021

convert content to use sentence case

778fa3d

dominiqueclarke force-pushed the feature/80166-add-waterfall-flyout branch from 418a2f5 to 778fa3d Compare January 27, 2021 18:08

dominiqueclarke added 2 commits January 29, 2021 09:35

pass onBarClick and onProjectionClick as WaterfallChart props

6c3c49b

use undefined value for initial flyoutData state

f6116ce

Merge branch 'master' of github.com:elastic/kibana into feature/80166…

4d0c7ca

…-add-waterfall-flyout

dominiqueclarke added 3 commits January 29, 2021 11:51

add telemetry

4e68e96

adjust typo in get_network_events

fd3c433

adjust connection time

34e2f88

kibanamachine and others added 5 commits February 9, 2021 07:53

Merge branch 'master' into feature/80166-add-waterfall-flyout

37df220

adjust SidebarItem onClick type

fb9bb48

Merge branch 'feature/80166-add-waterfall-flyout' of https://github.c…

a3b44f6

…om/dominiqueclarke/kibana into feature/80166-add-waterfall-flyout

fix license header

1a725ed

align middle truncated text left

35a39b8

dominiqueclarke force-pushed the feature/80166-add-waterfall-flyout branch from 221701b to df6699a Compare February 9, 2021 20:54

move flyout logic to a render prop for better composability

7406334

dominiqueclarke force-pushed the feature/80166-add-waterfall-flyout branch from df6699a to 7406334 Compare February 9, 2021 20:57

justinkambic reviewed Feb 10, 2021

View reviewed changes

justinkambic approved these changes Feb 10, 2021

View reviewed changes

add ip to flyout

3194e4d

kibanamachine and others added 3 commits February 11, 2021 08:42

Merge branch 'master' into feature/80166-add-waterfall-flyout

f54b934

update label for bytes downloaded (compressed)

1ee8c67

Merge branch 'feature/80166-add-waterfall-flyout' of https://github.c…

5734d9d

…om/dominiqueclarke/kibana into feature/80166-add-waterfall-flyout

dominiqueclarke added release_note:enhancement auto-backport Deprecated - use backport:version if exact versions are needed and removed release_note:roadmap labels Feb 11, 2021

lowercase compressed

a15a365

dominiqueclarke merged commit 53f4d48 into elastic:master Feb 11, 2021

dominiqueclarke deleted the feature/80166-add-waterfall-flyout branch February 11, 2021 22:48

kibanamachine mentioned this pull request Feb 11, 2021

[7.x] [Uptime] Feature/80166 add waterfall flyout (#89449) #91233

Merged

fkanout mentioned this pull request Dec 15, 2025

[Actionable Obs] Remove z-index override from Synthetics waterfall flyout #246343

Merged


		let formattedValue = formatValueForDisplay(value);

		if (postFix) {


		import { useWaterfallContext } from '../context/waterfall_chart';

		export const DETAILS = i18n.translate('xpack.uptime.synthetics.waterfall.flyout.details', {

Conversation

dominiqueclarke commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 27, 2021

Uh oh!

dominiqueclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulb-elastic commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katrin-freihofner commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulb-elastic commented Jan 29, 2021

Uh oh!

dominiqueclarke commented Jan 29, 2021

Uh oh!

paulb-elastic commented Jan 29, 2021

Uh oh!

dominiqueclarke commented Feb 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominiqueclarke Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinkambic left a comment

Choose a reason for hiding this comment

Uh oh!

dominiqueclarke commented Feb 11, 2021

Uh oh!

kibanamachine commented Feb 11, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dominiqueclarke commented Jan 27, 2021 •

edited

Loading

paulb-elastic commented Jan 29, 2021 •

edited

Loading

katrin-freihofner commented Jan 29, 2021 •

edited

Loading

dominiqueclarke Feb 11, 2021 •

edited

Loading