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.

Sign up for GitHub

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

Jump to bottom

test: Test reliability of navigator.sendBeacon() #888

Merged
merged 11 commits into from
Jun 8, 2023
Merged

test: Test reliability of navigator.sendBeacon() #888

merged 11 commits into from
Jun 8, 2023

Conversation

emma-imber
Copy link
Contributor

@emma-imber emma-imber commented Jun 1, 2023 •
edited
Loading

What does this change?

For 1% of calls to amiused, send a fetch keepalive = true POST request as well as navigator.sendBeacon() for amiused logging. Each will be labelled so that we can query the data for these specific test calls. Keeping the sampling percent low to start so we don't crowd the logs too much, and we can always increase later on if not enough data is coming through.

Why?

This will allow us to compare the number of logs received for each send method, giving us an idea of how reliable navigator.sendBeacon() is compared to sending a fetch request. Some articles online suggest that navigator.sendBeacon() may not be as reliable as we think: https://volument.com/blog/sendbeacon-is-broken

@emma-imber emma-imber added the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jun 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

🚀 @guardian/commercial-v7.1.1-beta.2 published to npm as a beta release

@emma-imber emma-imber added [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR and removed [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR labels Jun 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

🚀 @guardian/commercial-v7.1.1-beta.3 published to npm as a beta release

@emma-imber emma-imber added [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR and removed [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR labels Jun 1, 2023
@arelra
Copy link
Member

arelra commented Jun 1, 2023

🚀 @guardian/commercial-v7.1.1-beta.3 published to npm as a beta release

Hmm the beta labelling should be v8.x-beta.x 🤔

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

🚀 @guardian/commercial-v7.1.1-beta.4 published to npm as a beta release

@emma-imber
Copy link
Contributor Author

emma-imber commented Jun 1, 2023 •
edited
Loading

🚀 @guardian/commercial-v7.1.1-beta.3 published to npm as a beta release

Hmm the beta labelling should be v8.x-beta.x 🤔

Yes I noticed this too! I tried running the grep statement locally that the beta release code uses, and it seems to think the latest version is the beta release from your bundle + core merge PR and it's incrementing from that. It doesn't seem to be able to pick up the v8 release?

@arelra
Copy link
Member

arelra commented Jun 1, 2023

🚀 @guardian/commercial-v7.1.1-beta.3 published to npm as a beta release

Hmm the beta labelling should be v8.x-beta.x 🤔

Yes I noticed this too! I tried running the grep statement locally that the beta release code uses, and it seems to think the latest version is the beta release from your bundle + core merge PR and it's incrementing from that. It doesn't seem to be able to pick up the v8 release?

Thanks for the fix :)

arelra
arelra approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@arelra arelra left a comment

Choose a reason for hiding this comment

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

Just wondering if we are capturing or able to join browser data later as this might be useful to @guardian/open-journalism @mxdvl and their investigation of holes in CWV collection.

@emma-imber emma-imber removed the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jun 1, 2023
@mxdvl
Copy link
Contributor

mxdvl commented Jun 1, 2023 •
edited
Loading

Just wondering if we are capturing or able to join browser data later as this might be useful to @guardian/open-journalism @mxdvl and their investigation of holes in CWV collection.

I doubt the issue is with the mechanism for sending data over the wire, as we still get a lot information for TTFB and LCP.

@changeset-bot
Copy link

changeset-bot bot commented Jun 6, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 8f6c1c3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@emma-imber emma-imber added [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR and removed [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR labels Jun 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

🚀 @guardian/commercial-v8.0.2-beta.1 published to npm as a beta release

@emma-imber emma-imber removed the [beta] @guardian/commercial Add this label to publish an @guardian/commercial beta npm release from a PR label Jun 7, 2023
@emma-imber emma-imber marked this pull request as ready for review June 8, 2023 09:17
@emma-imber emma-imber requested a review from a team as a code owner June 8, 2023 09:17
@emma-imber emma-imber merged commit 613e017 into main Jun 8, 2023
11 of 12 checks passed
@emma-imber emma-imber deleted the ei/test-send-beacon branch June 8, 2023 11:06
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2023

🎉 This PR is included in version 9.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Jakeii pushed a commit that referenced this pull request Jun 26, 2023
@emma-imber @Jakeii
test: Test reliability of navigator.sendBeacon() (#888)
205b2fa 
* Send beacon and fetch event at sampling rate of 5%

* Add sampling rate parameter

* Set test sample rate to zero to fix failing tests

* Fix property addition and increase sampling

* Get the sendBeacon test logs reliably firing

* Add amiused to page targeting to create test logs

* Set all amiused.spec beacon test sampling to 0

* Update beta release to find latest beta version

* Set sampling to 1%

* Remove testing function

* Undo beta release workflow command change
@Jakeii Jakeii mentioned this pull request May 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arelra arelra arelra approved these changes

@domlander domlander domlander approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@emma-imber @arelra @mxdvl @domlander