Closed
Bug 1453667
Opened 7 years ago
Closed 6 years ago
Remove BrowserUITelemetry
Categories
(Toolkit :: Telemetry, enhancement, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [lang=js])
Attachments
(4 files)
BrowserUITelemetry stopped being useful when bug 1443609 landed.
Unfortunately, there's still a reasonable amount of data that is being collated by it but never sent nor used. There's also occasional maintenance overhead (e.g. I found it as I'm looking at changing something).
Unless there are imminent plans to re-use this code for something else, I think we should remove it. If it is later needed, we can always use history to recover it if necessary.
:phlsa - we believe you might be working on a project related to the functionality it used to provide, do you have plans to re-use the code soon?
Flags: needinfo?(philipp)
Comment 1•7 years ago
|
||
Yes, cmore, Romain and I have been working on a thing that's related but doesn't use any of the same technology.
Just to make sure that we salvage any useful insights before removing it: can you point me to a summary of what kind of date is being collected right now? Thanks!
Flags: needinfo?(philipp)
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #1)
> Just to make sure that we salvage any useful insights before removing it:
> can you point me to a summary of what kind of date is being collected right
> now? Thanks!
I've just sent this direct.
Assignee | ||
Comment 3•7 years ago
|
||
Setting as a mentored bug - I'm happy to mentor this.
Rough summary of what to do:
- BrowserUITelemetry.jsm is to be removed completely. Here's a link where you can find where the current references to it are:
https://searchfox.org/mozilla-central/search?q=BrowserUITelemetry&case=false®exp=false&path=
- Checkout Firefox's source code & make sure you can build it if you haven't already.
- Go through the code to remove the references to BrowserUITelemetry, making sure you clean up unused variables/functions along the way.
- For the tests browser_BrowserUITelemetry_* and browser_UITour_registerPageID.js can be removed (remove from the browser.ini file as well), the other tests will need just the BrowserUITelemetry parts removing from them.
- Make sure `./mach eslint` gives no errors/warnings (note: you can also do `./mach eslint path/to/files` if you need to repeat it)
- For the tests you've changed, make sure that `./mach mochitest path/to/test` runs fine and passes.
Push the patch using mozreview if possible, include the bug number a message and "r?Standard8" in the first line of the commit message.
Mentor: standard8
Whiteboard: [lang=js]
(In reply to Mark Banner (:standard8) from comment #3)
> Setting as a mentored bug - I'm happy to mentor this.
>
> Rough summary of what to do:
>
> - BrowserUITelemetry.jsm is to be removed completely. Here's a link where
> you can find where the current references to it are:
>
> https://searchfox.org/mozilla-central/
> search?q=BrowserUITelemetry&case=false®exp=false&path=
>
> - Checkout Firefox's source code & make sure you can build it if you haven't
> already.
>
> - Go through the code to remove the references to BrowserUITelemetry, making
> sure you clean up unused variables/functions along the way.
>
> - For the tests browser_BrowserUITelemetry_* and
> browser_UITour_registerPageID.js can be removed (remove from the browser.ini
> file as well), the other tests will need just the BrowserUITelemetry parts
> removing from them.
>
> - Make sure `./mach eslint` gives no errors/warnings (note: you can also do
> `./mach eslint path/to/files` if you need to repeat it)
>
> - For the tests you've changed, make sure that `./mach mochitest
> path/to/test` runs fine and passes.
>
> Push the patch using mozreview if possible, include the bug number a message
> and "r?Standard8" in the first line of the commit message.
I'll take you up on this. I'd be happy to take on the task and have you as a mentor!
Assignee | ||
Comment 5•7 years ago
|
||
Hi Michael, please start working on it, generally we only assign bugs once the first patch is attached.
Comment 6•7 years ago
|
||
FYI for Jan-Erik to check how that lines up with our current work.
Jan-Erik is currently coordinating the removal of these legacy components on our team.
Flags: needinfo?(jrediger)
Comment 7•7 years ago
|
||
I'd be happy to see the code go away.
It might be a bit tricky in UITour, where it's still used. We collected some feedback regarding UITour here[1].
I am not too familiar with that code, so I don't know what exactly BrowserUITelemetry is used for in UITour. You might want to double-check how/if it affects other uses of UITour.
Once BrowserUITelemetry is gone, that only leaves us with direct usage of UITelemetry on mobile.
[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1443605#c19
Flags: needinfo?(jrediger)
Comment 8•7 years ago
|
||
Could we please put a hold on this for a bit?
There are a couple of things in the list that Mark sent me that might be vital for certain teams. Let's make sure that these teams can get what they need from other places before removing the code.
Thanks!
Updated•7 years ago
|
Priority: -- → P2
Comment 9•7 years ago
|
||
Issue resolved! Please go ahead :)
Updated•6 years ago
|
Priority: P2 → P3
Assignee | ||
Updated•6 years ago
|
Summary: Possibly remove BrowserUITelemetry → Remove BrowserUITelemetry
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → standard8
Mentor: standard8
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8984053 [details]
Bug 1453667 - Remove BrowserUITelemetry from other parts of browser/
https://reviewboard.mozilla.org/r/249918/#review256184
Attachment #8984053 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8984051 [details]
Bug 1453667 - Remove BrowserUITelemetry from search.
https://reviewboard.mozilla.org/r/249914/#review256320
Attachment #8984051 -
Flags: review?(adw) → review+
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8984052 [details]
Bug 1453667 - Remove BrowserUITelemetry from UITour.
https://reviewboard.mozilla.org/r/249916/#review256372
yay! I've been wanting to rid the telemetry code out of UITour for a while…
::: browser/components/uitour/UITour.jsm:68
(Diff revision 1)
> // This map is not persisted and is used for
> // building the content source of a potential tour.
> pageIDsForSession: new Map(),
This should probably also go since it's unused already… then the whole `registerPageID` API should maybe go (but left in the content API for backwards-compat) but it's not clear to me whether this is still used for analysis. It seems to me like it should be replaced with event telemetry but there is also an interaction with the `*TreatmentTag` APIs which duplicated the page ID logic (because I wasn't involved in review to prevent it).
Ideally this should be in a separate Firefox::Tours bug with a needinfo for agibson and cmore to figure out what kind of tour tracking we need anymore. If you think you can get the info from them in this bug without it this bug getting too busy then that's fine.
Attachment #8984052 -
Flags: review?(MattN+bmo) → review+
Comment 18•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984052 [details]
Bug 1453667 - Remove BrowserUITelemetry from UITour.
https://reviewboard.mozilla.org/r/249916/#review256372
> This should probably also go since it's unused already… then the whole `registerPageID` API should maybe go (but left in the content API for backwards-compat) but it's not clear to me whether this is still used for analysis. It seems to me like it should be replaced with event telemetry but there is also an interaction with the `*TreatmentTag` APIs which duplicated the page ID logic (because I wasn't involved in review to prevent it).
>
> Ideally this should be in a separate Firefox::Tours bug with a needinfo for agibson and cmore to figure out what kind of tour tracking we need anymore. If you think you can get the info from them in this bug without it this bug getting too busy then that's fine.
To clarify, I think this is fine to land (ideally with `pageIDsForSession` also removed) as the data is already not being collected but we should have a separate bug to figure out what the onboarding/growth/webdev teams are expecting us to be tracking so we can figure out the future of `*TreatmentTag` and a potential replacement/deprecation for registerPageID.
Assignee | ||
Comment 19•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984052 [details]
Bug 1453667 - Remove BrowserUITelemetry from UITour.
https://reviewboard.mozilla.org/r/249916/#review256372
> To clarify, I think this is fine to land (ideally with `pageIDsForSession` also removed) as the data is already not being collected but we should have a separate bug to figure out what the onboarding/growth/webdev teams are expecting us to be tracking so we can figure out the future of `*TreatmentTag` and a potential replacement/deprecation for registerPageID.
Filed bug 1468588
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8984054 [details]
Bug 1453667 - Remove BrowserUITelemetry.
https://reviewboard.mozilla.org/r/249920/#review257228
Sorry for the slow turnaround.
Thanks for cleaning this up!
Attachment #8984054 -
Flags: review?(gfritzsche) → review+
Comment 25•6 years ago
|
||
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42525128a962
Remove BrowserUITelemetry from search. r=adw
https://hg.mozilla.org/integration/autoland/rev/09800ee2e5a9
Remove BrowserUITelemetry from UITour. r=MattN
https://hg.mozilla.org/integration/autoland/rev/4e8bd9fe8028
Remove BrowserUITelemetry from other parts of browser/ r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1768cef6a099
Remove BrowserUITelemetry. r=gfritzsche
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42525128a962
https://hg.mozilla.org/mozilla-central/rev/09800ee2e5a9
https://hg.mozilla.org/mozilla-central/rev/4e8bd9fe8028
https://hg.mozilla.org/mozilla-central/rev/1768cef6a099
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•