Closed Bug 1453667 Opened 6 years ago Closed 6 years ago

Remove BrowserUITelemetry

Categories

(Toolkit :: Telemetry, enhancement, P3)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [lang=js])

Attachments

(4 files)

Bug 1453667 - Remove BrowserUITelemetry from search.
59 bytes, text/x-review-board-request
adw
: review+
Details
Bug 1453667 - Remove BrowserUITelemetry from UITour.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1453667 - Remove BrowserUITelemetry from other parts of browser/
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1453667 - Remove BrowserUITelemetry.
59 bytes, text/x-review-board-request
gfritzsche
: review+
Details 
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)
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)
(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.
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&regexp=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&regexp=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!
Hi Michael, please start working on it, generally we only assign bugs once the first patch is attached.
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)
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)
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!
Priority: -- → P2
Priority: P2 → P3
Summary: Possibly remove BrowserUITelemetry → Remove BrowserUITelemetry
Assignee: nobody → standard8
Mentor: standard8
Status: NEW → ASSIGNED
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 on attachment 8984051 [details]
Bug 1453667 - Remove BrowserUITelemetry from search.

https://reviewboard.mozilla.org/r/249914/#review256320
Attachment #8984051 - Flags: review?(adw) → 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 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.
Blocks: 1468588
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 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+
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
See Also: → 1557153
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: