Skip to content
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

Merged
merged 6 commits into from
Mar 21, 2024

Conversation

abhi-agg
Copy link
Contributor

@abhi-agg abhi-agg commented Mar 20, 2024

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1885857

Pull Request checklist

  • The pull request has a descriptive title (and a reference to an issue it
    fixes, if applicable)
  • All tests and linter checks are passing
  • The pull request is free of merge conflicts

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.

Copy link
Contributor

@Dexterp37 Dexterp37 left a 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

src/telemetry/metrics.yaml Show resolved Hide resolved
Copy link
Contributor

@rosahbruno rosahbruno left a 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?

@Dexterp37
Copy link
Contributor

@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)

Copy link
Contributor

@rosahbruno rosahbruno left a 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.

  - 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
@abhi-agg
Copy link
Contributor Author

@rosahbruno @Dexterp37 I have made the required changes as per the discussion above 👍🏾

src/telemetry/index.js Show resolved Hide resolved
@abhi-agg abhi-agg requested a review from Dexterp37 March 21, 2024 11:26
@abhi-agg abhi-agg merged commit a6caeb6 into mozilla:main Mar 21, 2024
7 checks passed
@abhi-agg abhi-agg deleted the remove-non-standard-page-load-1885857 branch March 21, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants