Last Comment Bug 1305681 - Remove DEVTOOLS_HUD_* histograms
: Remove DEVTOOLS_HUD_* histograms
Status: RESOLVED FIXED
[measurement:client] [lang=js] [good ...
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: Unspecified Unspecified
P3 normal (vote)
: mozilla53
Assigned To: Pavan Karthik [:matrixisreal]
:
: Georg Fritzsche [:gfritzsche]
Mentors: Georg Fritzsche [:gfritzsche]
Depends on:
Blocks: 1136118
  Show dependency treegraph
 
Reported: 2016-09-27 03:31 PDT by Georg Fritzsche [:gfritzsche]
Modified: 2017-01-25 05:31 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
made the _logHistogram(metric) a noop (3.32 KB, patch)
2016-12-02 13:04 PST, Pavan Karthik [:matrixisreal]
no flags Details | Diff | Splinter Review
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json (10.15 KB, patch)
2016-12-02 13:25 PST, Pavan Karthik [:matrixisreal]
no flags Details | Diff | Splinter Review
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json (9.22 KB, patch)
2016-12-03 12:06 PST, Pavan Karthik [:matrixisreal]
gfritzsche: review+
Details | Diff | Splinter Review
made the _logHistogram(metric) a noop (2.85 KB, patch)
2016-12-05 11:05 PST, Pavan Karthik [:matrixisreal]
gfritzsche: feedback+
Details | Diff | Splinter Review
v4.patch (2.89 KB, patch)
2016-12-11 05:02 PST, Pavan Karthik [:matrixisreal]
gfritzsche: review+
Details | Diff | Splinter Review
Remove DEVTOOLS_HUD_* histograms (12.12 KB, patch)
2016-12-12 13:43 PST, Pavan Karthik [:matrixisreal]
no flags Details | Diff | Splinter Review
v5.patch (12.12 KB, patch)
2016-12-12 13:47 PST, Pavan Karthik [:matrixisreal]
gfritzsche: review+
Details | Diff | Splinter Review

Description User image Georg Fritzsche [:gfritzsche] 2016-09-27 03:31:46 PDT
These histograms are expiring and are probably not actively used:
https://dxr.mozilla.org/mozilla-central/search?q=DEVTOOLS_HUD_&redirect=false
Comment 1 User image Georg Fritzsche [:gfritzsche] 2016-09-27 03:33:31 PDT
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 User image Tamara Hills [:thills] 2016-09-30 06:51:09 PDT
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 User image Georg Fritzsche [:gfritzsche] 2016-09-30 08:00:34 PDT
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 User image Jacob Harrow-Mortelliti [:jakehm] 2016-10-06 09:50:32 PDT
And the convention for making the method a noop would be just: 
>_logHistogram(metric){}
?
Comment 5 User image Georg Fritzsche [:gfritzsche] 2016-10-06 14:56:49 PDT
(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 User image Pavan Karthik [:matrixisreal] 2016-12-02 05:27:14 PST
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 User image Georg Fritzsche [:gfritzsche] 2016-12-02 06:32:58 PST
(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 User image Pavan Karthik [:matrixisreal] 2016-12-02 06:47:50 PST
Awesome, Ill start working on this today and let you know the status. :D
Comment 9 User image Pavan Karthik [:matrixisreal] 2016-12-02 13:04:36 PST
Created attachment 8816566 [details] [diff] [review]
made the _logHistogram(metric) a noop
Comment 10 User image Pavan Karthik [:matrixisreal] 2016-12-02 13:25:22 PST
Created attachment 8816577 [details] [diff] [review]
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json
Comment 11 User image Pavan Karthik [:matrixisreal] 2016-12-03 12:06:04 PST
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.
Comment 12 User image Georg Fritzsche [:gfritzsche] 2016-12-04 15:49:40 PST
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 User image Georg Fritzsche [:gfritzsche] 2016-12-04 15:51:03 PST
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 User image Pavan Karthik [:matrixisreal] 2016-12-05 11:03:00 PST
Thanks for the review,I have updated the patch accordingly,
What is the next step in the process?
Comment 15 User image Pavan Karthik [:matrixisreal] 2016-12-05 11:05:43 PST
Created attachment 8816918 [details] [diff] [review]
made the _logHistogram(metric) a noop
Comment 16 User image Georg Fritzsche [:gfritzsche] 2016-12-09 15:22:44 PST
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 User image Pavan Karthik [:matrixisreal] 2016-12-11 05:02:32 PST
Created attachment 8817878 [details] [diff] [review]
v4.patch
Comment 18 User image Georg Fritzsche [:gfritzsche] 2016-12-12 07:17:36 PST
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 User image Pavan Karthik [:matrixisreal] 2016-12-12 13:43:58 PST
Created attachment 8818106 [details] [diff] [review]
Remove DEVTOOLS_HUD_* histograms
Comment 20 User image Pavan Karthik [:matrixisreal] 2016-12-12 13:47:27 PST
Created attachment 8818107 [details] [diff] [review]
v5.patch
Comment 21 User image Georg Fritzsche [:gfritzsche] 2016-12-13 02:13:29 PST
Comment on attachment 8818107 [details] [diff] [review]
v5.patch

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

Thanks!
Comment 22 User image Pulsebot 2016-12-14 00:19:23 PST
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd906e69d35
Remove DEVTOOLS_HUD_* histograms. r=gfritzsche
Comment 23 User image Carsten Book [:Tomcat] 2016-12-14 07:46:56 PST
https://hg.mozilla.org/mozilla-central/rev/1fd906e69d35
Comment 24 User image Pavan Karthik [:matrixisreal] 2016-12-14 08:15:42 PST
> Thanks!

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

Note You need to log in before you can comment on or make changes to this bug.