1201492 - Remove extended_statistics_ok from Telemetry histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

extended_statistics_ok might not not really be used:
https://dxr.mozilla.org/mozilla-central/search?q=extended_statistics_ok

Per IRC it seems vladan & rvitillo seem ok with dropping it.

Comment 1

[Georg Fritzsche [:gfritzsche]]

ni?me for setting it up as a mentored bug.

Comment 2

[Georg Fritzsche [:gfritzsche]]

We need to remove code etc. that references or uses extended_statistics_ok:
https://dxr.mozilla.org/mozilla-central/search?q=extended_statistics_ok+path%3Atoolkit%2Fcomponents%2Ftelemetry%2F&redirect=true&case=false

From there we should remove the code that uses it:
https://dxr.mozilla.org/mozilla-central/search?q=extendedStatisticsOK+path%3Atoolkit%2Fcomponents%2Ftelemetry&redirect=true&case=false
https://dxr.mozilla.org/mozilla-central/search?q=kExtendedStatisticsFlag&redirect=true&case=false

This should just call sample_.Accumulate() now:
https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/ipc/chromium/src/base/histogram.cc#561

That means log_sum and log_sum_squares are never filled with any values now and we should remove them too:
https://dxr.mozilla.org/mozilla-central/search?q=log_sum+path%3Atoolkit%2Fcomponents%2Ftelemetry&redirect=false&case=false

... this hopefully covers the changes needed here.
To confirm things work ok after the changes, we should run the Telemetry xpcshell tests:
mach xpcshell-test toolkit/components/telemetry/tests/unit

Comment 3

[Georg Fritzsche [:gfritzsche]]

Robert, are you interested in this bug?
This will probably take a bit longer than most of your previous bugs. Should be pretty straight-forward though, although there might be a little test-fallout to investigate and fix.

Comment 4

[rthyberg]

yeah sounds good

Comment 5

[Georg Fritzsche [:gfritzsche]]

Thanks - i overlooked that you are still working on bug 1179209, so this might be a good follow-up after that.

Comment 6

[Anup Kumar [:reznord]]

Attachment: Removed all extended_statistics_ok entries from Telemetry histograms

Removed all extended_statistics_ok entries from Telemetry histograms

Comment 7

[Anup Kumar [:reznord]]

(In reply to Anup Kumar [:Anupkumar] from comment #6)
> Created attachment 8699546 [details] [diff] [review]
> Removed all extended_statistics_ok entries from Telemetry histograms
> 
> Removed all extended_statistics_ok entries from Telemetry histograms

mach xpcshell test failed in two cases and those two cases along with error produced are in the following pastebin link: https://pastebin.mozilla.org/8854809

Comment 8

[Anup Kumar [:reznord]]

(In reply to Anup Kumar [:Anupkumar] from comment #7)
> (In reply to Anup Kumar [:Anupkumar] from comment #6)
> > Created attachment 8699546 [details] [diff] [review]
> > Removed all extended_statistics_ok entries from Telemetry histograms
> > 
> > Removed all extended_statistics_ok entries from Telemetry histograms
> 
> mach xpcshell test failed in two cases and those two cases along with error
> produced are in the following pastebin link:
> https://pastebin.mozilla.org/8854809

Fixed one of the errors which came in the xpcshell test and not able to understand the reason why the other test is being failed, I didn't modify the file * test_TelemetrySession.js *, but it failed in the test.

The resulting error is in the pastebin link -> https://pastebin.mozilla.org/8854861

Comment 9

[Anup Kumar [:reznord]]

Attachment: Removed all extended_statistics_ok entries from Telemetry histograms

Made some minor modifications on the above attachment which fixed one of the errors in the test results.

Comment 10

[Anup Kumar [:reznord]]

Attachment: Removed all extended_statistics_ok entries from Telemetry histograms

Fixed all the errors and fixed the above mentioned problem in the xpcshell tests.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8699836 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

This looks pretty good on a quick first glance!
I'll leave the review here to Alessio.

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8699836 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

It looks like we're almost there!

Just a couple of changes. Please make sure tests are still ok after those.

::: ipc/chromium/src/base/histogram.cc
@@ +739,5 @@
>    Accumulate(value, count, index);
>    sum_squares_ += static_cast<int64_t>(count) * value * value;
>  }
>  
>  void Histogram::SampleSet::AccumulateWithExponentialStats(Sample value,

I don't think anybody else is calling this function. Could you please check and eventually remove it from here and histogram.h?

::: toolkit/components/telemetry/gen-histogram-data.py
@@ +59,5 @@
>  def print_array_entry(output, histogram, name_index, exp_index):
>      cpp_guard = histogram.cpp_guard()
>      if cpp_guard:
>          print("#if defined(%s)" % cpp_guard, file=output)
>      print("  { %s, %s, %s, %s, %d, %d, %s, %s, %s }," \

You removed a parameter and should also remove the corresponding formatter option from print.

Comment 13

[Anup Kumar [:reznord]]

Attachment: Removed all extended_statistics_ok entries from Telemetry histograms

Made the changes as requested in comment 12, made sure that all the bug passes all the tests.

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8699949 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

Thank you for such a quick update!

There are a couple of things that I didn't pick up on the first round, sorry about that.

::: ipc/chromium/src/base/histogram.h
@@ +333,5 @@
>      void CheckSize(const Histogram& histogram) const;
>  
>      // Accessor for histogram to make routine additions.
>      void AccumulateWithLinearStats(Sample value, Count count, size_t index);
>      // Alternate routine for exponential histograms.

nit: This part of the comment should be removed as well.

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +31,4 @@
>    for(var i=0;i<r.length;i++) {
>      var v = r[i];
>      sum += v;
>      if (histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL) {

I think this if branch is not needed anymore, as |log_v| is not being used anymore. Right?

@@ +407,5 @@
>  }
>  
>  // Check that histograms that aren't flagged as needing extended stats
>  // don't record extended stats.
>  function test_extended_stats() {

Can you remove this test, since we no longer have extended stats? The "sum" itself is tested elsewhere in the file.

Comment 15

[Anup Kumar [:reznord]]

Attachment: Removed all extended_statistics_ok entries from Telemetry histograms

Made the changes as required in comment 14.

Comment 16

[Alessio Placitelli [:Dexter]]

Comment on attachment 8700042 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

This looks good, thank you for your persistence!

Could you push this to Try, please?

Comment 17

[Alessio Placitelli [:Dexter]]

One last note: could you change the commit message to r=dexter?

Comment 18

[Alessio Placitelli [:Dexter]]

Mark, Vladan are you aware of anybody using the extended statistics?

Comment 19

[Anup Kumar [:reznord]]

(In reply to Alessio Placitelli [:Dexter] from comment #16)
> Comment on attachment 8700042 [details] [diff] [review]
> Removed all extended_statistics_ok entries from Telemetry histograms
> 
> Review of attachment 8700042 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thank you for your persistence!
> 
> Could you push this to Try, please?

What are the tests that I should run on tryserver? Can you give me the syntax for that?

Comment 20

[Alessio Placitelli [:Dexter]]

(In reply to Anup Kumar [:Anupkumar] from comment #19)
> (In reply to Alessio Placitelli [:Dexter] from comment #16)
> > Comment on attachment 8700042 [details] [diff] [review]
> > Removed all extended_statistics_ok entries from Telemetry histograms
> > 
> > Review of attachment 8700042 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good, thank you for your persistence!
> > 
> > Could you push this to Try, please?
> 
> What are the tests that I should run on tryserver? Can you give me the
> syntax for that?

I'd say "try: -b do -p linux,linux64-asan,macosx64,win32,android-api-9,android-api-11,android-x86,emulator,emulator-jb,linux32_gecko -u all -t none" to be safe.

Comment 21

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e870ff1e0491

Comment 22

[Anup Kumar [:reznord]]

Comment on attachment 8700042 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +2863,5 @@
>                           JSPROP_ENUMERATE)) {
>      return nullptr;
>    }
> +  // TODO: calculate "sum"
> +  if (!JS_DefineProperty(cx, ret, "sum", 0, JSPROP_ENUMERATE) {

missing a ")" here which closes the 'if' condition

Comment 23

[Alessio Placitelli [:Dexter]]

Comment on attachment 8700042 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

Please, make sure your patch works locally before pushing to try again:

- ./mach build ipc
- ./mach build toolkit/components/telemetry
- ./mach xpcshell-test toolkit/components/telemetry

::: ipc/chromium/src/base/histogram.cc
@@ +557,5 @@
>  
>  // Update histogram data with new sample.
>  void Histogram::Accumulate(Sample value, Count count, size_t index) {
>    // Note locking not done in this version!!!
> +  sample_.Accumulate(value, count, index);

Please, change this line to |sample_.AccumulateWithLinearStats(value, count, index);| as |Accumulate| is not visible to Histogram.

Sorry for not catching this earlier.

Comment 24

[Mark Reid [:mreid]]

If present, the extended stats fields (log_sum and friends) will be stored by the "old" telemetry pipeline. If they're missing that's OK too. 

I don't know of any actual uses of the data itself, it doesn't appear to be used in the latest aggregation code. Vladan might know more about whether it's in use.

Comment 25

[Mark Reid [:mreid]]

Comment on attachment 8700042 [details] [diff] [review]
Removed all extended_statistics_ok entries from Telemetry histograms

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

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ -45,5 @@
>    do_check_eq(sum, s.sum);
>    if (histogram_type == Telemetry.HISTOGRAM_EXPONENTIAL) {
>      // We do the log with float precision in C++ and double precision in
>      // JS, so there's bound to be tiny discrepancies.  Just check the
>      // integer part.

Drive-by review :)

This comment should be removed as well, it doesn't make sense after the next 2 lines are gone.

Comment 26

[Anup Kumar [:reznord]]

Attachment: Removed

Tested locally after building along with the patch. Passed in all the tests. 

Will push to try server.

Comment 27

[Alessio Placitelli [:Dexter]]

Comment on attachment 8700652 [details] [diff] [review]
Removed

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

Good job, thank you. Would you change the reviewer in the commit message? r=dexter instead of r=gfritzsche.

Let's also see if that's all green on try!

Comment 28

[Anup Kumar [:reznord]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bfabfdc75fe

Comment 29

[Vladan Djeric (:vladan)]

(In reply to Alessio Placitelli [:Dexter] from comment #18)
> Mark, Vladan are you aware of anybody using the extended statistics?

Metrics used it in the Telemetry dash a few years ago to come up with a better measure of mean for exponential histograms. I really doubt that they've used it since then. You could always check with them on #metrics

Comment 30

[Alessio Placitelli [:Dexter]]

Nothing from #metrics, but I was pointed to https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe (thanks @mreid).

"The extended statistics are a leftover from a different Telemetry backend where they were used to calculate a better mean for bucketed measurements in exponential histograms. The extended statistics are not used in the current Telemetry backend, and since it carries an additional overhead, you should not use this field in your histogram declaration."

So I think it's safe to land this.

Comment 31

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/7ff2f511b41bee04d9033e96d13b53a9683b482c
Bug 1201492 - Remove extended_statistics_ok from Telemetry histograms. r=dexter

Comment 32

[Alessio Placitelli [:Dexter]]

Pushed to fxteam, as the try failures look unrelated. Once this lands, we will need to update the documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ff2f511b41b

Comment 34

[Alessio Placitelli [:Dexter]]

I updated the documentation from comment 32.