Remove DEVTOOLS_HUD_* histograms

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: gfritzsche, Assigned: matrixisreal, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [measurement:client] [lang=js] [good first bug])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 months ago
These histograms are expiring and are probably not actively used:
https://dxr.mozilla.org/mozilla-central/search?q=DEVTOOLS_HUD_&redirect=false
(Reporter)

Comment 1

7 months ago
Tamara, i think this is the only code module that uses them:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/b2g/chrome/content/devtools/hud.js#395

Is it feasible to just make `_logHistogram()` a no-op and remove the Histograms.json entries?
Flags: needinfo?(thills)
Hi Georg,

Yes, I think that's a good idea to make _logHistogram a no-op and remove the Histograms.json entries.  I don't believe anything other than FxOS was using this.

Thanks,
-tamara
Flags: needinfo?(thills)
(Reporter)

Comment 3

7 months ago
Thanks Tamara.

So, the plan here is to:
* make _logHistogram() [1] a no-op
* remove all the DEVTOOLS_HUD_* entries in Histograms.json and histogram-whitelists.json [2]

1: https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/b2g/chrome/content/devtools/hud.js#395
2: https://dxr.mozilla.org/mozilla-central/search?q=DEVTOOLS_HUD_&redirect=false
Mentor: gfritzsche@mozilla.com
Whiteboard: [measurement:client] → [measurement:client] [lang=js] [good first bug]
And the convention for making the method a noop would be just: 
>_logHistogram(metric){}
?
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 5

7 months ago
(In reply to Jacob Mortelliti[:earlgreyhot] from comment #4)
> And the convention for making the method a noop would be just: 
> >_logHistogram(metric){}
> ?

I'm not aware of a convention, but i think this ok if we add a comment as well. I.e.:

> _logHistogram(metric) {
>   // ... explanation on why it's a no-op.
> }
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 6

5 months ago
Hi, I would like to work on this bug.
these are the things I am planning to do:
1. Making the entire _logHistogram() method a noop
2. delete the DEVTOOLS_HUD_* entries in repctive json files.
Correct me if iam wrong.
Thanks :)
(Reporter)

Comment 7

5 months ago
(In reply to Pavan Karthik from comment #6)
> Hi, I would like to work on this bug.
> these are the things I am planning to do:
> 1. Making the entire _logHistogram() method a noop
> 2. delete the DEVTOOLS_HUD_* entries in repctive json files.

That sounds good to me :)
Let me know if you need help with getting started.
Assignee: nobody → pavankarthikboddeda
(Assignee)

Comment 8

5 months ago
Awesome, Ill start working on this today and let you know the status. :D
(Assignee)

Comment 9

5 months ago
Created attachment 8816566 [details] [diff] [review]
made the _logHistogram(metric) a noop
Attachment #8816566 - Flags: review?(gfritzsche)
(Assignee)

Comment 10

5 months ago
Created attachment 8816577 [details] [diff] [review]
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json
Attachment #8816577 - Flags: review?(gfritzsche)
(Assignee)

Comment 11

5 months ago
Created attachment 8816709 [details] [diff] [review]
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json

Resubmitting as i submitted a wrong patch before.
Attachment #8816577 - Attachment is obsolete: true
Attachment #8816577 - Flags: review?(gfritzsche)
Attachment #8816709 - Flags: review?(gfritzsche)
(Reporter)

Comment 12

5 months ago
Comment on attachment 8816566 [details] [diff] [review]
made the _logHistogram(metric) a noop

Review of attachment 8816566 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!
This looks good overall, but needs some changes.

::: b2g/chrome/content/devtools/hud.js
@@ +317,4 @@
>    },
>  
>    _logHistogram(metric) {
> +             // method left as no-op as histograms are not in use no more.

The indentation here seems wrong - it should use 2 spaces per indentation level.

@@ -372,5 @@
> -      } catch (err) {
> -        console.error('Histogram error: ' + err);
> -      }
> -    }
> -  }

We should not remove this brace ('}'), it closes the scope of the function.

::: browser/themes/linux/browser.css
@@ +1292,4 @@
>  #tabbrowser-tabs {
>    /* override the global style to allow the selected tab to be above the nav-bar */
>    z-index: auto;
> +

The changes in this file are not needed.
Attachment #8816566 - Flags: review?(gfritzsche)
(Reporter)

Comment 13

5 months ago
Comment on attachment 8816709 [details] [diff] [review]
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json

Review of attachment 8816709 [details] [diff] [review]:
-----------------------------------------------------------------

This part looks fine, thanks!
Attachment #8816709 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 14

5 months ago
Thanks for the review,I have updated the patch accordingly,
What is the next step in the process?
(Assignee)

Comment 15

5 months ago
Created attachment 8816918 [details] [diff] [review]
made the _logHistogram(metric) a noop
Attachment #8816566 - Attachment is obsolete: true
Attachment #8816918 - Flags: review?(gfritzsche)
(Reporter)

Comment 16

5 months ago
Comment on attachment 8816918 [details] [diff] [review]
made the _logHistogram(metric) a noop

Review of attachment 8816918 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks close.
There is a small nit to fix here.
Also we should improve the message of the patch, e.g.: "Bug 1305681 - Remove DEVTOOLS_HUD_* histograms."

::: b2g/chrome/content/devtools/hud.js
@@ +316,5 @@
>      shell.sendEvent(frame, 'advanced-telemetry-update', Cu.cloneInto(payload, frame));
>    },
>  
>    _logHistogram(metric) {
> +    //method left as no-op as histograms are not in use anymore. 

Small nit: This line has a trailing space at the end.
You can follow the review link for the patch to see it highlighted (or use local tools).
Attachment #8816918 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 17

5 months ago
Created attachment 8817878 [details] [diff] [review]
v4.patch
Attachment #8816918 - Attachment is obsolete: true
Attachment #8817878 - Flags: review?(gfritzsche)
(Reporter)

Comment 18

5 months ago
Comment on attachment 8817878 [details] [diff] [review]
v4.patch

Review of attachment 8817878 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks!
The other patch should also have a commit message of this form, then we can land these patches.
Alternatively you could also put the changes together in one patch.
Attachment #8817878 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 19

5 months ago
Created attachment 8818106 [details] [diff] [review]
Remove DEVTOOLS_HUD_* histograms
Attachment #8816709 - Attachment is obsolete: true
Attachment #8817878 - Attachment is obsolete: true
(Assignee)

Comment 20

5 months ago
Created attachment 8818107 [details] [diff] [review]
v5.patch
Attachment #8818106 - Attachment is obsolete: true
Attachment #8818107 - Flags: review?(gfritzsche)
(Reporter)

Comment 21

5 months ago
Comment on attachment 8818107 [details] [diff] [review]
v5.patch

Review of attachment 8818107 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8818107 - Flags: review?(gfritzsche) → review+
(Reporter)

Updated

5 months ago
Keywords: checkin-needed

Comment 22

4 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd906e69d35
Remove DEVTOOLS_HUD_* histograms. r=gfritzsche
Keywords: checkin-needed

Comment 23

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1fd906e69d35
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 24

4 months ago
> Thanks!

Hi Could you guide me one what to do next/suggest me some other bugs to work on? =)
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.