Closed Bug 1201492 Opened 4 years ago Closed 4 years ago

Remove extended_statistics_ok from Telemetry histograms

Categories

(Toolkit :: Telemetry, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- affected
firefox46 --- fixed

People

(Reporter: gfritzsche, Assigned: reznord, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [measurement:client] [good next bug])

Attachments

(1 file, 5 obsolete files)

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.
ni?me for setting it up as a mentored bug.
Flags: needinfo?(gfritzsche)
Priority: -- → P4
Whiteboard: [measurement:client]
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
Flags: needinfo?(gfritzsche)
Whiteboard: [measurement:client] → [measurement:client] [good next bug]
Mentor: 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.
Flags: needinfo?(robertthyberg)
yeah sounds good
Flags: needinfo?(robertthyberg)
Thanks - i overlooked that you are still working on bug 1179209, so this might be a good follow-up after that.
Mentor: alessio.placitelli
Assignee: nobody → allamsetty.anup
Removed all extended_statistics_ok entries from Telemetry histograms
Attachment #8699546 - Flags: review?(gfritzsche)
Attachment #8699546 - Flags: review?(alessio.placitelli)
(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
(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
Made some minor modifications on the above attachment which fixed one of the errors in the test results.
Attachment #8699546 - Attachment is obsolete: true
Attachment #8699546 - Flags: review?(gfritzsche)
Attachment #8699546 - Flags: review?(alessio.placitelli)
Attachment #8699821 - Flags: review?(gfritzsche)
Attachment #8699821 - Flags: review?(alessio.placitelli)
Fixed all the errors and fixed the above mentioned problem in the xpcshell tests.
Attachment #8699821 - Attachment is obsolete: true
Attachment #8699821 - Flags: review?(gfritzsche)
Attachment #8699821 - Flags: review?(alessio.placitelli)
Attachment #8699836 - Flags: review?(gfritzsche)
Attachment #8699836 - Flags: review?(alessio.placitelli)
Status: NEW → ASSIGNED
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.
Attachment #8699836 - Flags: review?(gfritzsche)
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.
Attachment #8699836 - Flags: review?(alessio.placitelli) → feedback+
Made the changes as requested in comment 12, made sure that all the bug passes all the tests.
Attachment #8699836 - Attachment is obsolete: true
Attachment #8699949 - Flags: review?(alessio.placitelli)
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.
Attachment #8699949 - Flags: review?(alessio.placitelli) → feedback+
Made the changes as required in comment 14.
Attachment #8699949 - Attachment is obsolete: true
Attachment #8700042 - Flags: review?(alessio.placitelli)
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?
Attachment #8700042 - Flags: review?(alessio.placitelli) → review+
One last note: could you change the commit message to r=dexter?
Mark, Vladan are you aware of anybody using the extended statistics?
Flags: needinfo?(vladan.bugzilla)
Flags: needinfo?(mreid)
(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?
(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 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 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.
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.
Flags: needinfo?(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.
Attached patch RemovedSplinter Review
Tested locally after building along with the patch. Passed in all the tests. 

Will push to try server.
Attachment #8700042 - Attachment is obsolete: true
Attachment #8700652 - Flags: review?(alessio.placitelli)
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!
Attachment #8700652 - Flags: review?(alessio.placitelli) → review+
(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
Flags: needinfo?(vladan.bugzilla)
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.
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
https://hg.mozilla.org/mozilla-central/rev/7ff2f511b41b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
I updated the documentation from comment 32.
You need to log in before you can comment on or make changes to this bug.