Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Uptime] Annotate waterfall chart with additional metrics #103642

Merged
merged 30 commits into from
Oct 5, 2021

Conversation

shahzad31
Copy link
Contributor

Summary

Related to elastic/synthetics#120

Since synthetics added relative performance metrics like LCP, FCP, CLS, Load event and DocumnetLoad

Annotated all of these metrics on top of waterfall chart

image

@shahzad31 shahzad31 self-assigned this Jun 29, 2021
@shahzad31 shahzad31 added release_note:enhancement v7.14.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jun 29, 2021
@shahzad31 shahzad31 requested a review from justinkambic June 29, 2021 11:38
@shahzad31 shahzad31 marked this pull request as ready for review June 29, 2021 11:38
@shahzad31 shahzad31 requested a review from a team as a code owner June 29, 2021 11:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looking pretty good, with a few minor changes.

I did have issues smoke testing this against more recent commits of Synthetics/Heartbeat.

image


if (data) {
const metricDocs = (data.hits.hits as unknown) as Array<{ fields: any }>;
let navigationStart = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, This is prone to errors and wold result in absurd values when navigationStart is not present in the doc.

I would change this logic to this.

  • Find the navigationStart from the metricDocs and if not present don't calculate the flyout metrics.
  • If present, calculate the offset metrics as already done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved it to make sure check for existence explicitly.

@spalger spalger added v7.15.0 and removed v7.14.0 labels Jun 30, 2021
Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me, tested against latest synthetics it seems to be working great.

LGTM based on the functionality. Unsure if there are any unanswered questions about plotting these steps.

I did have one nit question regarding the code, maybe it's no problem though.

const dispatch = useDispatch();

useEffect(() => {
dispatch(getDynamicSettings());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are using data plugin search api, so we need to fetch heartbeat indices settings to construct that query params to pass it to the data.search

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 I do have one question

  • If the step contains multiple requests before the navigation request, would we still show the waterfall markers?

} = fields;
if (metricType?.[0] !== 'navigationStart') {
metrics.push({
id: metricType?.[0],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can skip doing the undefined check here?

export const DOCUMENT_CONTENT_LOADED_LABEL = i18n.translate(
'xpack.uptime.synthetics.waterfall.domContentLabel',
{
defaultMessage: 'DOMContentLoaded',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: DOMContentLoaded event to be in sync with Load event

@@ -10,6 +10,17 @@ import { WaterfallData, WaterfallDataEntry, WaterfallMetadata } from '../types';
import { OnSidebarClick, OnElementClick, OnProjectionClick } from '../components/use_flyout';
import { SidebarItems } from '../../step_detail/waterfall/types';

export type MarkerItems = Array<{
id:
| 'domContentLoaded'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we also get cls, but it's not a relative offset or trace. Its an actual value and not presentable in the waterfall.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'm seeing potential problems with really large waterfall charts. When CNN is the first step, no annotations are displayed. Interestingly, I actually saw the annotations flash then disappear at one point during testing.

Screen Shot 2021-09-29 at 4 18 59 PM

  1. Why is the DOMContentLoaded 0 for subsequent steps?
    Here is my github profile as the first step

Screen Shot 2021-09-29 at 4 10 45 PM

Here it is as a second step
Screen Shot 2021-09-29 at 4 08 25 PM
Also other second/third steps:
Screen Shot 2021-09-29 at 4 10 58 PM
Screen Shot 2021-09-29 at 3 59 15 PM
Screen Shot 2021-09-29 at 4 11 10 PM

Example inline journey

step("goto cnn", async () => page.goto('https://www.cnn.com'));
step("goto github", async () => page.goto('https://github.com/dominiqueclarke'));
step("goto facebook", async () => page.goto('https://www.facebook.com'));

const heartbeatIndices = settings?.heartbeatIndices || '';

const { data, loading } = useEsSearch(
hasNavigationRequest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this whole hook depends on hasNavigation request, can be exit early if this is false and return an empty metrics array and loading false?

@@ -26,9 +27,10 @@ export const renderLegendItem: RenderItem<LegendItem> = (item) => {
interface Props {
total: number;
data: NetworkItems;
markerItems: MarkerItems;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this optional all the way down the component tree? That way you won't need to pass it to existing tests all the way through the component tree? You already have an early return here to account for it being option https://github.com/elastic/kibana/pull/103642/files#diff-06ea7122b3e4859a49d4baeb64472ed36489474493b8f6024f40a40d2e34fa49R47

}
);

export function WaterfallCharMarkers() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function WaterfallCharMarkers() {
export function WaterfallChartMarkers() {

metricDocs.forEach(({ fields }) => {
if (fields['browser.relative_trace.type']?.[0] === 'mark') {
const {
'browser.relative_trace.name': metricType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can reuse the constants above for both these values.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
uptime 656 658 +2

Async chunks

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

id before after diff
uptime 565.0KB 568.0KB +3.0KB

History

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

cc @shahzad31

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2021-10-04 at 3 18 15 PM

Screen Shot 2021-10-04 at 3 10 28 PM

Screen Shot 2021-10-04 at 3 10 16 PM

Screen Shot 2021-10-04 at 3 10 11 PM

Issue with large waterfall charts is fixed, and the issues with subsequent steps dom content loaded has been discussed and clarified. LGTM

@shahzad31 shahzad31 merged commit 00bb597 into elastic:master Oct 5, 2021
@shahzad31 shahzad31 deleted the waterfall-annotations branch October 5, 2021 07:13
@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 5, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 5, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 6, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Oct 7, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 7, 2021
@shahzad31 shahzad31 removed their assignment Nov 8, 2021
@awahab07 awahab07 self-assigned this Nov 11, 2021
@awahab07
Copy link
Contributor

Post FF Testing

A webpage alibaba.com is visited as the first step in a browser monitor. The step goes successful and visiting the performance break waterfall chart, the following annotations are available

  • First Contentful Paint (FCP): 3270ms
  • Largest Contentful Paint (LCP): 4450ms
  • DOM Content Loaded: 4320ms
  • Layout Shift: 7600ms
    Screenshot 2021-11-15 at 20 50 13

Comparison of FCP and LCP metrics with Lighthouse and calibreapp for the same page:

Agent FCP (ms) LCP (ms)
Kibana Synthetics 3270 4450
Chrome Lighthouse 600 1900
calibreapp 1150 2100

Issues:
LCP is not reported, see #118569
Possible UX improvements, see #118607

@awahab07 awahab07 removed their assignment Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants