1305681 - Remove DEVTOOLS_HUD_* histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

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?

Comment 2

[Tamara Hills [: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

Comment 3

[Georg Fritzsche [:gfritzsche]]

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

Comment 4

[Jacob Harrow-Mortelliti [:jakehm]]

And the convention for making the method a noop would be just: 
>_logHistogram(metric){}
?

Comment 5

[Georg Fritzsche [:gfritzsche]]

(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.
> }

Comment 6

[Pavan Karthik [:matrixisreal]]

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

Comment 7

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 8

[Pavan Karthik [:matrixisreal]]

Awesome, Ill start working on this today and let you know the status. :D

Comment 9

[Pavan Karthik [:matrixisreal]]

Attachment: made the _logHistogram(metric) a noop

Comment 10

[Pavan Karthik [:matrixisreal]]

Attachment: removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json

Comment 11

[Pavan Karthik [:matrixisreal]]

Attachment: removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json

Resubmitting as i submitted a wrong patch before.

Comment 12

[Georg Fritzsche [:gfritzsche]]

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.

Comment 13

[Georg Fritzsche [:gfritzsche]]

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!

Comment 14

[Pavan Karthik [:matrixisreal]]

Thanks for the review,I have updated the patch accordingly,
What is the next step in the process?

Comment 15

[Pavan Karthik [:matrixisreal]]

Attachment: made the _logHistogram(metric) a noop

Comment 16

[Georg Fritzsche [:gfritzsche]]

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

Comment 17

[Pavan Karthik [:matrixisreal]]

Attachment: v4.patch

Comment 18

[Georg Fritzsche [:gfritzsche]]

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.

Comment 19

[Pavan Karthik [:matrixisreal]]

Attachment: Remove DEVTOOLS_HUD_* histograms

Comment 20

[Pavan Karthik [:matrixisreal]]

Attachment: v5.patch

Comment 21

[Georg Fritzsche [:gfritzsche]]

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

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

Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd906e69d35
Remove DEVTOOLS_HUD_* histograms. r=gfritzsche

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fd906e69d35

Comment 24

[Pavan Karthik [:matrixisreal]]

> Thanks!

Hi Could you guide me one what to do next/suggest me some other bugs to work on? =)