feat: use events from PW for network data #372

vigneshshanmugam · 2021-09-08T20:54:42Z

fix Use network events from PW instead of using CDPSession #287
fix Synthetics Doesn’t Handle Popups (new tab or new window) #253
All requests inside all frames, windows gets captured with this change and also we get header and body size for each individual request.
Timing calculation becomes a lot simpler to deal with and don't require much understanding of the chrome devtools code which we relied on before.

Missing data

We don't have a queuing time exposed from the PW API as Queueing time is pretty specific to chromium and not exposed for other browsers like FF and Webkit and its in the order of 1-2ms max when tested -
Bare uncompressed resource size is not exposed for each request instead we have header and body sizes for both request and responses.
http.version data is not exposed, but we can add that as a follow up once PW adds support for it and we are not using it on the waterfall UI.

apmmachine · 2021-09-08T21:03:17Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-10-06T19:10:20.728+0000
Duration: 15 min 1 sec
Commit: 58f6e19

Test stats 🧪

Test	Results
Failed	0
Passed	140
Skipped	0
Total	140

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

andrewvc · 2021-09-27T23:31:24Z

__tests__/plugins/network.test.ts

-    expect(netinfo[1].timings.total).toBeGreaterThan(delayTime);
-  });
-
-  describe('waterfall timing calculation', () => {


Is there a good reason to remove this? It seems helpful if for no other reason than to ensure that the data we send to heartbeat/uptime is consistent. Is that checked elsewhere?

We don't do calculation in the Synthetics code anymore, It's as part of Playwright code.

__tests__/reporters/__snapshots__/json.test.ts.snap

andrewvc · 2021-09-27T23:33:17Z

src/reporters/json.ts

    },
    http: {
-      version: formatVersion(response?.protocol),


Why did we remove the version?

In the PR description already

http.version data is not exposed, but we can add that as a follow up once PW adds support for it and we are not using it on the waterfall UI.

src/reporters/json.ts

andrewvc · 2021-09-27T23:36:15Z

__tests__/plugins/network.test.ts

    expect(netinfo[0]).toMatchObject({
-      resourceSize: 10,
-      transferSize: expect.any(Number),
+      resourceSize: -1,


What's the reason for the change here?

As per the PR description

Bare uncompressed resource size is not exposed for each request instead we have header and body sizes for both request and responses.

shahzad31 · 2021-09-28T11:59:26Z

Aren't we displaying queuing time in waterfal chart, if so, this seems like a breaking change.

dominiqueclarke

Looks great. Do you have any suggestions for testing the feature? Can I run it with heartbeat to check for compatibility with waterfall?

src/plugins/network.ts

vigneshshanmugam · 2021-09-28T18:39:07Z

Looks great. Do you have any suggestions for testing the feature? Can I run it with heartbeat to check for compatibility with waterfall?

Yeah we need to run it via HB to test this for compatibility

Switch to this branch and install it globally npm i -g .
Run only inline monitors via HB which will use this version.

vigneshshanmugam · 2021-09-28T18:41:16Z

Aren't we displaying queuing time in waterfal chart, if so, this seems like a breaking change.

I wouldnt call it as breaking change, if its not sent to UI, it shouldn't show those timings.

dominiqueclarke

We are expecting requestStartTime and loadEndTime to be chrome's special baseline in seconds value. It appears to have now been changed to epoch millis in this PR. This will require a change on the Kibana side to be compatible with this version of synthetics, unless you are able to convert those two values back to the way chrome was providing it.

vigneshshanmugam · 2021-09-30T22:33:51Z

We are expecting requestStartTime and loadEndTime to be chrome's special baseline in seconds value

I remember we were using this on the UI only for calculating the offset, Why does this have to be in a special format? I can surely workaround to not break the UI, but would like to know the reason so we can solve this in a better way to never break on the future iteration.

dominiqueclarke · 2021-10-01T12:23:56Z

We are expecting requestStartTime and loadEndTime to be chrome's special baseline in seconds value

I remember we were using this on the UI only for calculating the offset, Why does this have to be in a special format? I can surely workaround to not break the UI, but would like to know the reason so we can solve this in a better way to never break on the future iteration.

We are using it for the offset, but before we use it we actually covert it to milliseconds. This allows accurately place the start of each request on the chart in milliseconds. So the code is expecting seconds to convert to milliseconds.

vigneshshanmugam · 2021-10-01T17:03:19Z

@dominiqueclarke So if I can change the epochTime in Seconds then it should be good from the UI side? We dont need it to be in the chrome devtools monotonic time format?

dominiqueclarke · 2021-10-01T17:12:15Z

monotonic time format?

Yes, I believe that should work just fine.

vigneshshanmugam · 2021-10-05T04:45:00Z

/test

dominiqueclarke

LGTM. Timing changes look good with Kibana waterfall chart.

dominiqueclarke · 2021-10-06T14:45:03Z

Timings and headers look good as well.

dominiqueclarke · 2021-10-06T15:31:50Z

src/plugins/network.ts

      loadEndTime: -1,
      responseReceivedTime: -1,
-      response: null,
-      resourceSize: 0,


We are expecting the value to be falsey if it is not able to be extracted. The change to making it truthy breaks UI.

Setting it to 0 would solve it? I had a thought since 0 is a actual size and could be for cached resource.

Yes changing it to 0 would solve it. To be fair, we are displaying 0 values right now as --, which may not be totally accurate, so perhaps we want to fix this on the UI side. The same is true for transfer size.

Gotcha, will revert it to zero. May be we can add a fix on the UI side in parallel. Do you mind creating an issue?

vigneshshanmugam · 2021-10-06T19:09:40Z

/test

vigneshshanmugam · 2021-10-07T17:17:16Z

@dominiqueclarke Can we merge it now or do you still see issue with the Request/Response headers?

dominiqueclarke · 2021-10-07T19:21:45Z

@dominiqueclarke Can we merge it now or do you still see issue with the Request/Response headers?

Yep, looks good to me.

…a-fleet-heartbeat-synthetics-e2e-tests * upstream/master: [jjbb] for scheduling e2e jobs (elastic#418) docs: release notes for beta.17 (elastic#415) 1.0.0-beta.17 feat: add --playwright-options (elastic#405) fix: ensure timestamps of events are inorder (elastic#413) fix: do not stash `.nvm` folder (elastic#414) deprecate suite-params option over params (elastic#406) fix: end timestamp should reflect duration (elastic#410) docs: release notes for 1.16.0 (elastic#403) 1.0.0-beta.16 fix: revert Playwright to 1.14.0 (elastic#402) docs: release notes for beta 15 (elastic#400) 1.0.0-beta.15 chore: update deps and fix lint issues (elastic#399) fix: handle page close during response event (elastic#398) chore: require node > 14 for usage (elastic#397) fix: fix megabytes to mbps (elastic#394) feat: enable trace as part of rich-events (elastic#387) feat: use events from PW for network data (elastic#372)

feat: use events from PW for network data

768a67d

vigneshshanmugam added the work in progress label Sep 8, 2021

vigneshshanmugam added 3 commits September 21, 2021 08:31

update pw to 1.15.0

455b9f6

Merge remote-tracking branch 'upstream/master' into switch-to-pw

990ec64

calculate size for each req

cec5944

vigneshshanmugam requested review from andrewvc and dominiqueclarke September 21, 2021 19:30

vigneshshanmugam removed the work in progress label Sep 21, 2021

vigneshshanmugam added 2 commits September 21, 2021 12:58

fix tls missing info

529e10b

remove http version

7649983

andrewvc reviewed Sep 27, 2021

View reviewed changes

dominiqueclarke reviewed Sep 28, 2021

View reviewed changes

src/plugins/network.ts Show resolved Hide resolved

src/plugins/network.ts Outdated Show resolved Hide resolved

dominiqueclarke reviewed Sep 28, 2021

View reviewed changes

src/plugins/network.ts Outdated Show resolved Hide resolved

dominiqueclarke reviewed Sep 30, 2021

View reviewed changes

address review and move to secs

90fd07a

vigneshshanmugam requested a review from dominiqueclarke October 5, 2021 04:29

dominiqueclarke approved these changes Oct 5, 2021

View reviewed changes

dominiqueclarke reviewed Oct 6, 2021

View reviewed changes

change resourceSize to 0

58f6e19

vigneshshanmugam requested a review from dominiqueclarke October 6, 2021 17:05

vigneshshanmugam merged commit 2231f3b into elastic:master Oct 7, 2021

vigneshshanmugam deleted the switch-to-pw branch October 7, 2021 20:47

This was referenced Mar 2, 2022

Content download wrong for cached requests elastic/uptime#460

Closed

remove unused fields from network information #466

Closed

vigneshshanmugam mentioned this pull request Apr 12, 2022

feat: use network events from pw #488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use events from PW for network data #372

feat: use events from PW for network data #372

vigneshshanmugam commented Sep 8, 2021 •

edited

apmmachine commented Sep 8, 2021 •

edited

Build stats

Test stats 🧪

andrewvc Sep 27, 2021

vigneshshanmugam Sep 27, 2021

andrewvc Sep 27, 2021

vigneshshanmugam Sep 27, 2021

andrewvc Sep 27, 2021

vigneshshanmugam Sep 27, 2021

shahzad31 commented Sep 28, 2021

dominiqueclarke left a comment

vigneshshanmugam commented Sep 28, 2021

vigneshshanmugam commented Sep 28, 2021

dominiqueclarke left a comment •

edited

vigneshshanmugam commented Sep 30, 2021

dominiqueclarke commented Oct 1, 2021

vigneshshanmugam commented Oct 1, 2021

dominiqueclarke commented Oct 1, 2021

vigneshshanmugam commented Oct 5, 2021

dominiqueclarke left a comment

dominiqueclarke commented Oct 6, 2021 •

edited

dominiqueclarke Oct 6, 2021

vigneshshanmugam Oct 6, 2021

dominiqueclarke Oct 6, 2021

vigneshshanmugam Oct 6, 2021

vigneshshanmugam commented Oct 6, 2021

vigneshshanmugam commented Oct 7, 2021

dominiqueclarke commented Oct 7, 2021

feat: use events from PW for network data #372

feat: use events from PW for network data #372

Conversation

vigneshshanmugam commented Sep 8, 2021 • edited

Missing data

apmmachine commented Sep 8, 2021 • edited

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzad31 commented Sep 28, 2021

dominiqueclarke left a comment

Choose a reason for hiding this comment

vigneshshanmugam commented Sep 28, 2021

vigneshshanmugam commented Sep 28, 2021

dominiqueclarke left a comment • edited

Choose a reason for hiding this comment

vigneshshanmugam commented Sep 30, 2021

dominiqueclarke commented Oct 1, 2021

vigneshshanmugam commented Oct 1, 2021

dominiqueclarke commented Oct 1, 2021

vigneshshanmugam commented Oct 5, 2021

dominiqueclarke left a comment

Choose a reason for hiding this comment

dominiqueclarke commented Oct 6, 2021 • edited

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

vigneshshanmugam commented Oct 6, 2021

vigneshshanmugam commented Oct 7, 2021

dominiqueclarke commented Oct 7, 2021

vigneshshanmugam commented Sep 8, 2021 •

edited

apmmachine commented Sep 8, 2021 •

edited

dominiqueclarke left a comment •

edited

dominiqueclarke commented Oct 6, 2021 •

edited