Bug 1777182 - Add a sendBeacon uploader#1834
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.
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.
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.
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.
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