-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
1cc639d
to
ffe680c
Compare
ffe680c
to
4167bdf
Compare
sendBeacon
sendBeacon
4167bdf
to
4a83d02
Compare
sendBeacon
sendBeacon
sendBeacon
sendBeacon
uploader
@badboy @rosahbruno do you have thoughts on the testing plan? |
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"}}'); |
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 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. |
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 test
Runs all testsnpm run lint
Runs all lintersCHANGELOG.md
or an explanation of why it does not need onemozilla/glean
repository