-
Notifications
You must be signed in to change notification settings - Fork 14
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
Set client_name metadata #64
Conversation
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.
LGTM, with one suggestion, to accept or reject at your discretion.
app/measure/measure.js
Outdated
downloadworkerfile: "/libraries/ndt7-download-worker.min.js" | ||
downloadworkerfile: "/libraries/ndt7-download-worker.min.js", | ||
metadata: { | ||
client_name: "mlab-speedtest", |
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.
To be more definitive, could we change this to "speed.measurementlab.net" or "speed-measurementlab-net"? I'm just wondering if "mlab-speedtest" might be too generic, in the case that we may launch some other speed test, like msak, at some other domain or location.
Visit the preview URL for this PR (updated for commit fc6ad12): https://mlab-speedtest--pr64-sandbox-roberto-add-hw5q8359.web.app (expires Wed, 31 Jan 2024 18:08:45 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f345dd84eac1914fdbf22d41db69d580917840b9 |
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.
LGTM as long as you are sure this is all working. It sounds like will all go away soon when we start testing msak.
Tests coming from speed.measurementlab.net don't currently set
client_name
, thus we cannot easily know how much traffic we get from there. This change should fix that.EDIT: This change turned out being a much larger change. The authentication method we used (tokens) to deploy to Firebase is deprecated and does not work anymore. I moved the deployment of this repo to Github Actions.