1252481 - Remove "sum_squares_lo" and "sum_squares_hi" from Histogram serialization

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

We don't use the "sum_squares_lo" and "sum_squares_hi" properties anywhere in the aggregator or other analysis code that i know of.

We should just remove those properties:
https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A(sum_squares_lo|sum_squares_hi)&case=true

Comment 1

[Vishnu (:malayaleecoder)]

Attachment: Bug1252481_v1.diff

Hello Georg,

I tried to ask a doubt in #telemetry regarding this Bug and I did not get a response. So I am submitting a patch with reference to what you have said in comment#1. BTW I could not build the code and see after the changes have been made. Please have a look and suggest further improvements.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8725421 [details] [diff] [review]
Bug1252481_v1.diff

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

This looks promising.
Before full review, you should make sure that:
1) this builds properly on your machine
2) that the tests pass, using: ./mach toolkit/components/telemetry/tests/unit/

::: toolkit/components/telemetry/Telemetry.cpp
@@ -1242,2 @@
>  
>    if (h->histogram_type() != Histogram::HISTOGRAM) {

We should remove this whole section, it only prepares data for sum_squares_lo/_hi below.

Comment 3

[Georg Fritzsche [:gfritzsche]]

If you have problems building you could ask for help with that in #introduction or #telemetry or drop me a mail.

Comment 4

[Vishnu (:malayaleecoder)]

Georg, Currently self assigning this. Will fix this within a day or two and submit an updated patch. Hope it won't be a problem.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Sure, that sounds good.

Comment 6

[Vishnu (:malayaleecoder)]

Attachment: Bug1252481_v2.diff

Georg, have done the said changes. The build was a success locally. Sorry for the delay. Please have a look :)

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8729673 [details] [diff] [review]
Bug1252481_v2.diff

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

This looks good, thanks!

I realized that now the backing code for sum_squares in histogram.cc/.h is not needed anymore, but we can remove that over in bug 1256366.

Comment 8

[Georg Fritzsche [:gfritzsche]]

I built & ran the tests successfully locally and don't expect any fallout, so this should be fine to check in.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bc86dd459e67

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc86dd459e67