-
Notifications
You must be signed in to change notification settings - Fork 34
Bug 1777182 - Add a sendBeacon uploader
#1834
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
Conversation
1cc639d to
ffe680c
Compare
ffe680c to
4167bdf
Compare
sendBeaconsendBeacon
4167bdf to
4a83d02
Compare
sendBeaconsendBeacon
sendBeaconsendBeacon uploader
|
@badboy @rosahbruno do you have thoughts on the testing plan? |
badboy
left a comment
There was a problem hiding this 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.
|
Instead of having 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"}}'); |
rosahbruno
left a comment
There was a problem hiding this 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
- Just making sure that we plan on using the supported uploader later once we verify that sendbeacon is working.
- 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.
4a83d02 to
a97f3b6
Compare
|
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 descI'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. |
rosahbruno
left a comment
There was a problem hiding this 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
glean/src/core/upload/worker.ts
Outdated
| // 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 = ( |
There was a problem hiding this comment.
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.
Before merging this PR:
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:
Pull Request checklist
glean/folder, run:npm run testRuns all testsnpm run lintRuns all lintersCHANGELOG.mdor an explanation of why it does not need onemozilla/gleanrepository