-
Notifications
You must be signed in to change notification settings - Fork 45
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 1885857] Remove non standard page load events #2158
[Bug 1885857] Remove non standard page load events #2158
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.
Please check whether or not these changes will break the probe-scraper before merging and deployinh them
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.
Hmm.. I understand that we no longer have any Glean metrics that we are recording, but I feel like I am unsure about removing the Glean instrumentation entirely. Unless we are wanting to showcase collecting telemetry without actually needing to do anything with the Glean files themselves.
@Dexterp37
What are your thoughts here?
I'm torn about this: on one end, leaving code around for no purpose is bad. On the other end, we might end up adding more context to events soon-ish, so we might as well leave it around (we'll need to add most of this back if we want to add a lifetime: application metric) |
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.
Let's continue to remove the code that calls the page load events, but for now lets not take out the Glean metrics and set up entirely.
This reverts commit 35e69d9.
This reverts commit c10304d.
- Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1885857 - Leave the metrics and pings definitions as well as probe scraper untouched to allow experimenting with events in future without making changes to builtin Glean events
@rosahbruno @Dexterp37 I have made the required changes as per the discussion above 👍🏾 |
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1885857
Pull Request checklist
fixes, if applicable)
Test
Tested the change locally by running
npm run dev
step mentioned in https://github.com/mozilla/glean-dictionary/blob/main/docs/development.md#development and observing log pings on browser's console. I don't see non-standard page load events anymore.#1015 was very handy as a reference in doing this task.