1469522 - Remove savedPings from simpleMeasurements

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

This bug is about removing the |savedPings| field from the |simpleMeasurements| [1]. To do this:

1 - Remove savedPings from both the docs and the TelemetrySession.jsm file as reported in [2];
2 - Remove the logic that exports and computes the saved pings count in TelemetryStorage.jsm - track back the implementation from [3] and remove all the related code.
3 - Fix any test failure by removing the offending code.

To run test coverage locally:

> ./mach xpcshell-test toolkit/components/telemetry/tests/unit

[1] - https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/docs/telemetry/data/main-ping.html#simplemeasurements
[2] - https://searchfox.org/mozilla-central/search?q=savedpings
[3] - https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/toolkit/components/telemetry/TelemetryStorage.jsm#420

Comment 1

[Issei Horie [:is2ei]]

Hi Alessio,

Could I work on this issue?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Issei Horie [:is2ei] from comment #1)
> Hi Alessio,
> 
> Could I work on this issue?

Sure! Let me know if anything is unclear.

Comment 3

[Issei Horie [:is2ei]]

Attachment: bug-1469522.patch

I created a patch. Could you review it, please?

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8987350 [details] [diff] [review]
bug-1469522.patch

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

Hi! Thank you for your patch :) It looks good overall, there's one thing that we should fix though.

Please note that there's a reference to TelemetryStorage.pendingPingCount (https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/components/telemetry/TelemetrySend.jsm#1009-1013), which was just removed. I think it's safe to change that to |this.pendingPingCount|. It should have made the test fail. Did you run the tests locally?

Comment 5

[Issei Horie [:is2ei]]

Thanks for your comment! :Dexter

> I think it's safe to change that to |this.pendingPingCount|.

I see! I will fix it.

> Did you run the tests locally?

I run `./mach xpcshell-test toolkit/components/telemetry/tests/unit` and it passed. 
Is there other tests do I need to run?

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Issei Horie [:is2ei] from comment #5)
> Thanks for your comment! :Dexter

Thank you for your work! ;)

> > Did you run the tests locally?
> 
> I run `./mach xpcshell-test toolkit/components/telemetry/tests/unit` and it
> passed. 
> Is there other tests do I need to run?

No, that should be enough. It probably means that the part that should have failed wasn't covered by tests :'( Eugh :(

Comment 7

[Issei Horie [:is2ei]]

Attachment: bug-1469522.patch

I updated my patch. Could you review it, please?

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8988516 [details] [diff] [review]
bug-1469522.patch

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

This looks good to me now, thanks! Let me push it to try for you!

Comment 9

[Alessio Placitelli [:Dexter]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de900ae11c02
Remove savedPings from simpleMeasurements. r=Dexter

Comment 11

[Raul Gurzau (:RaulG)]

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