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

Bug 1777182 - Add a sendBeacon uploader #1834

Merged
merged 10 commits into from
Nov 30, 2023
Merged

Conversation

Dexterp37
Copy link
Contributor

@Dexterp37 Dexterp37 commented Nov 21, 2023

Before merging this PR:

  • Remove the changes to the sample app
  • Figure out with @scholtzan why these submissions are not going through

The use of that API has some implications to the rest of the ecosystem (ingestion, tooling) so we likely want to have this optionally enabled for a while. Unfortunately, there's no way to simple test this without using it on a product. So here's what I'd recommend:

  1. Review and merge this (not used anywhere!)
  2. Cut a new prerelease version
  3. Use the new prerelease in the debug view or the glean dictionary, and add URL parameters to conditionally turn this on
  4. Validate if this works in the pipeline without changing anything ?

Pull Request checklist

  • Quality: Make sure this PR builds and runs cleanly.
    • Inside the glean/ folder, run:
      • npm run test Runs all tests
      • npm run lint Runs all linters
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
  • Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work

@Dexterp37 Dexterp37 changed the title DRAFT: Sketch out a potential use of sendBeacon Bug 1777182 - DRAFT: Sketch out a potential use of sendBeacon Nov 27, 2023
@Dexterp37 Dexterp37 marked this pull request as ready for review November 29, 2023 11:23
@auto-assign auto-assign bot requested a review from travis79 November 29, 2023 11:23
@Dexterp37 Dexterp37 requested review from rosahbruno and removed request for travis79 November 29, 2023 11:23
@Dexterp37 Dexterp37 changed the title Bug 1777182 - DRAFT: Sketch out a potential use of sendBeacon Bug 1777182 - Sketch out a potential use of sendBeacon Nov 29, 2023
@Dexterp37 Dexterp37 changed the title Bug 1777182 - Sketch out a potential use of sendBeacon Bug 1777182 - Add a sendBeacon uploader Nov 29, 2023
@Dexterp37
Copy link
Contributor Author

@badboy @rosahbruno do you have thoughts on the testing plan?

Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

testing in production!
Yeah, we don't rely on glean dictionary data for anything, so testing it there gets us the most real-world testing.

@scholtzan
Copy link

Instead of having sendBeacon send an escaped JSON string, it actually shouldn't get escaped. This is working:

navigator.sendBeacon("https://incoming.telemetry.mozilla.org/submit/debug-ping-view/events/1/e2588a68-6801-45c0-86a8-03f6784b0168", '{"events":[{"category":"sample","name":"random_event","timestamp":0}],"ping_info":{"seq":4,"start_time":"2023-11-29T18:50+01:00","end_time":"2023-11-29T19:05+01:00","reason":"max_capacity"},"client_info":{"telemetry_sdk_build":"4.0.0-pre.0","client_id":"cae639c1-7462-49e6-a8a7-7eced5656820","first_run_date":"2023-11-27+01:00","os":"Windows","os_version":"Unknown","architecture":"Unknown","locale":"it-IT","app_build":"Unknown","app_display_version":"Unknown"}}');

Copy link
Contributor

@rosahbruno rosahbruno left a comment

Choose a reason for hiding this comment

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

Everything looks good. The testing plan also makes sense, we talked about this offline too.

The only call outs I have here are

  1. Just making sure that we plan on using the supported uploader later once we verify that sendbeacon is working.
  2. The ignore-await stuff in sendbeacon_uploader feels like maybe we could do it differently, but I am going to take a look and get back to you tomorrow.

@Dexterp37
Copy link
Contributor Author

Dexterp37 commented Nov 30, 2023

All right, I believe this is finally ready for review folks! I'm able to see the live data using this local hack and with a query like:

select * FROM `moz-fx-data-shared-prod.debug_ping_view_live.events_v1` AS e where date(submission_timestamp) >= "2023-11-30" order by submission_timestamp desc

I'm also able to see stuff in the debug view when setting the debug view tags, since this falls back to fetch in that case.

Copy link
Contributor

@rosahbruno rosahbruno left a comment

Choose a reason for hiding this comment

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

I think everything here looks good! Excited to get this landed and start testing

// Some options require us to submit custom headers. Unfortunately not all the
// uploaders support them (e.g. `sendBeacon`). In case headers are required, switch
// back to the default uploader that, for now, supports headers.
const needsHeaders = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be a boolean without the need for a ternary, but not a big deal.

@Dexterp37 Dexterp37 merged commit 7977cf3 into mozilla:main Nov 30, 2023
@Dexterp37 Dexterp37 deleted the send-beacon branch November 30, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants