1469233 - Remove pingsOverdue from simpleMeasurements

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

From the main ping review sheet [0], there are two Telemetry-specific data fields in the simpleMeasurements section that i believe we never actively used:
https://searchfox.org/mozilla-central/search?q=pingsOverdue
https://searchfox.org/mozilla-central/search?q=savedpings

We should just remove them unless there is a need for them.

0: https://docs.google.com/spreadsheets/d/1y5gbtHibpEcZMObKDt1AXSXM4wLtw4vwXkN10OKV7H0/

Comment 1

[Georg Fritzsche [:gfritzsche]]

Alessio, Chris, do you see us needing these?

If not this, could one of you set it up for a mentored bug?

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Alessio, Chris, do you see us needing these?

I don't think we have a specific need for these (and we haven't really been looking at them either :E). For the saved pings, we have a few indirect measurements that we look at:

> TELEMETRY_PENDING_LOAD_FAILURE_READ
> TELEMETRY_PENDING_LOAD_FAILURE_PARSE
> TELEMETRY_PENDING_PINGS_SIZE_MB
> TELEMETRY_PENDING_PINGS_AGE
> TELEMETRY_PENDING_PINGS_EVICTED_OVER_QUOTA
> TELEMETRY_PENDING_EVICTING_OVER_QUOTA_MS
> TELEMETRY_PENDING_CHECKING_OVER_QUOTA_MS
> TELEMETRY_PENDING_LOAD_MS

For the overdue pings, we can still look at the TELEMETRY_PENDING_PINGS_AGE histogram to check if things go wrong.

> If not this, could one of you set it up for a mentored bug?

I'm happy to set this up as mentored if Chris is ok with dropping these as well.

Comment 3

[Chris H-C :chutten]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> > Alessio, Chris, do you see us needing these?

No. I have never used them. Moreover, if I wanted that information I would probably reimplement them as scalars before I remembered to check simpleMeasurements to see if they already existed :S

> I'm happy to set this up as mentored if Chris is ok with dropping these as
> well.

Please go ahead with that.

Comment 4

[Georg Fritzsche [:gfritzsche]]

ni?alessio for mentoring setup.

Comment 5

[Alessio Placitelli [:Dexter]]

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

1 - Remove pingsOverdue from both the docs and the TelemetrySession.jsm file as reported in [2];
2 - Remove the logic that exports and computes the overdue pings count in TelemetrySend.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=pingsOverdue
[3] - https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/toolkit/components/telemetry/TelemetrySend.jsm#237

Comment 6

[Arjun Krishna Babu]

I'm interested in this. 

Is Alessio Placitelli the mentor?

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Arjun Krishna Babu from comment #6)
> I'm interested in this. 
> 
> Is Alessio Placitelli the mentor?

Yes, I'm mentoring this bug. Let me assign it to you. Is anything from comment 5 unclear?

Comment 8

[Arjun Krishna Babu]

Attachment: bug1469233-intended-changes.diff

I've attached a diff file of the changes I made. Could you please take a look at it and let me know if it's good or not?

If it's good, I'll submit a review request via MozReview.

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Arjun Krishna Babu from comment #8)
> Created attachment 8986174 [details] [diff] [review]
> bug1469233-intended-changes.diff
> 
> I've attached a diff file of the changes I made. Could you please take a
> look at it and let me know if it's good or not?
> 
> If it's good, I'll submit a review request via MozReview.

Hi Arjun,

you're on the right path. However, you should really submit a patch or a review request through MozReview to have a full review :)
Please run the tests and make sure it's working as expected before you do ;)

Comment 10

[Arjun Krishna Babu]

Attachment: Bug 1469233 - remove pingsOverdue from simpleMeasurements;

pingsOverdue is a telemetry-specific data field that has never been actively
used. Therefore it is being removed from both the docs and the
TelemetrySession.jsm file.

The logic that exports and computes the overdue pings count, and all related
code, is also removed.

Associated test failure (due to performing the above) is fixed by removing the
offending test code.

Review commit: https://reviewboard.mozilla.org/r/251808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251808/

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8986441 [details]
Bug 1469233 - remove pingsOverdue from simpleMeasurements;

https://reviewboard.mozilla.org/r/251808/#review258406

::: commit-message-1e2c9:3
(Diff revision 1)
> +Bug 1469233 - remove pingsOverdue from simpleMeasurements; r?Dexter
> +
> +pingsOverdue is a telemetry-specific data field that has never been actively

That's not true :) It has been actively used, it's not being used any more ;)

::: toolkit/components/telemetry/TelemetrySend.jsm
(Diff revision 1)
>  
>      const now = Policy.now();
>  
> -    // Check for overdue pings.
> -    const overduePings = infos.filter((info) =>
> -      (now.getTime() - info.lastModificationDate) > OVERDUE_PING_FILE_AGE);

Is `OVERDUE_PING_FILE_AGE` still needed? If not, let's remove it.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js:314
(Diff revision 1)
>   * Create some recent and overdue pings and verify that they get sent.
>   */
>  add_task(async function test_overdue_pings_trigger_send() {
>    let pingTypes = [
>      { num: RECENT_PINGS },
>      { num: OVERDUE_PINGS, age: OVERDUE_PING_FILE_AGE },

If overdue pings are no longer referenced anywhere, then we don't have the concept of overdue pings any more and can remove this entry.

Comment 12

[Arjun Krishna Babu]

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> Comment on attachment 8986441 [details]
> Bug 1469233 - remove pingsOverdue from simpleMeasurements;
> 
> https://reviewboard.mozilla.org/r/251808/#review258406
> 
> ::: commit-message-1e2c9:3
> (Diff revision 1)
> > +Bug 1469233 - remove pingsOverdue from simpleMeasurements; r?Dexter
> > +
> > +pingsOverdue is a telemetry-specific data field that has never been actively
> 
> That's not true :) It has been actively used, it's not being used any more ;)
Right! I was working based off of what was in comment 0. It was my misunderstanding. I'll fix the commit message :)

> 
> ::: toolkit/components/telemetry/TelemetrySend.jsm
> (Diff revision 1)
> >  
> >      const now = Policy.now();
> >  
> > -    // Check for overdue pings.
> > -    const overduePings = infos.filter((info) =>
> > -      (now.getTime() - info.lastModificationDate) > OVERDUE_PING_FILE_AGE);
> 
> Is `OVERDUE_PING_FILE_AGE` still needed? If not, let's remove it.
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js:314
> (Diff revision 1)
> >   * Create some recent and overdue pings and verify that they get sent.
> >   */
> >  add_task(async function test_overdue_pings_trigger_send() {
> >    let pingTypes = [
> >      { num: RECENT_PINGS },
> >      { num: OVERDUE_PINGS, age: OVERDUE_PING_FILE_AGE },
> 
> If overdue pings are no longer referenced anywhere, then we don't have the
> concept of overdue pings any more and can remove this entry.

OVERDUE_PING_FILE_AGE is used quite a bit in test_TelemetrySendOldPings.js . 
I'm not sure if removing `OVERDUE_PING_FILE_AGE` will break any other tests.

However, if we don't have the concept of overdue pings anymore, I guess we could remove whole chunks of code in test_TelemetrySendOldPings.js:

1. The line where `OVERDUE_PING_FILE_AGE` gets defined. https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js#23
An associated comment describing this constant would also have to be removed.

2. The `add_task()` function which, according to the comments, creates some recent and overdue pings and verifies that they get sent. https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js#308-331

3. https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js#233-241 and perhaps a few more lines after that.

4. https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js#361 . A comment right before this line would also need to be updated.

Shall I go ahead and remove all of these?

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Arjun Krishna Babu from comment #12)
> OVERDUE_PING_FILE_AGE is used quite a bit in test_TelemetrySendOldPings.js . 
> I'm not sure if removing `OVERDUE_PING_FILE_AGE` will break any other tests.
> 
> However, if we don't have the concept of overdue pings anymore, I guess we
> could remove whole chunks of code in test_TelemetrySendOldPings.js:
> 
> 1. The line where `OVERDUE_PING_FILE_AGE` gets defined.
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/
> unit/test_TelemetrySendOldPings.js#23
> An associated comment describing this constant would also have to be removed.
> 
> 2. The `add_task()` function which, according to the comments, creates some
> recent and overdue pings and verifies that they get sent.
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/
> unit/test_TelemetrySendOldPings.js#308-331
> 
> 3.
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/
> unit/test_TelemetrySendOldPings.js#233-241 and perhaps a few more lines
> after that.
> 
> 4.
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/components/telemetry/tests/
> unit/test_TelemetrySendOldPings.js#361 . A comment right before this line
> would also need to be updated.
> 
> Shall I go ahead and remove all of these?

Yes, I think this is correct!

Comment 14

[Arjun Krishna Babu]

Comment on attachment 8986441 [details]
Bug 1469233 - remove pingsOverdue from simpleMeasurements;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251808/diff/1-2/

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8986441 [details]
Bug 1469233 - remove pingsOverdue from simpleMeasurements;

https://reviewboard.mozilla.org/r/251808/#review259114

Hey, thank you for your patch and work! There are a few changes left, mostly test issues: let's try to keep as much testing as possible and remove unused variables!

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js:21
(Diff revision 2)
>  const {OS: {File, Path, Constants}} = ChromeUtils.import("resource://gre/modules/osfile.jsm", {});
>  
>  // We increment TelemetryStorage's MAX_PING_FILE_AGE and
> -// OVERDUE_PING_FILE_AGE by 1 minute so that our test pings exceed
>  // those points in time, even taking into account file system imprecision.
>  const ONE_MINUTE_MS = 60 * 1000;

nit: `ONE_MINUTE_MS` is not used anymore. We can remove it. After this, the comment above doesn't refer to anything, so let's remove it as well.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js:25
(Diff revision 2)
>  const ONE_MINUTE_MS = 60 * 1000;
> -const OVERDUE_PING_FILE_AGE = TelemetrySend.OVERDUE_PING_FILE_AGE + ONE_MINUTE_MS;
>  
>  const PING_SAVE_FOLDER = "saved-telemetry-pings";
>  const PING_TIMEOUT_LENGTH = 5000;
>  const OVERDUE_PINGS = 6;

Since we're removign the test, `OVERDUE_PINGS` is not being used anymore. Let's remove it.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js:29
(Diff revision 2)
>  const PING_TIMEOUT_LENGTH = 5000;
>  const OVERDUE_PINGS = 6;
>  const OLD_FORMAT_PINGS = 4;
>  const RECENT_PINGS = 4;
>  
>  const TOTAL_EXPECTED_PINGS = OVERDUE_PINGS + RECENT_PINGS + OLD_FORMAT_PINGS;

Since we're removing the related test, this const is not used anymore. Let's remove it.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
(Diff revision 2)
>  });
>  
> -/**
> - * Create an overdue ping in the old format and try to send it.
> - */
> -add_task(async function test_overdue_old_format() {

Instead of removing this, let's call it `test_old_formats`.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
(Diff revision 2)
> -    getSavePathForPingId(PING_NO_INFO.slug),
> -    getSavePathForPingId(PING_NO_PAYLOAD.slug),
> -    getSavePathForPingId("no-slug-file"),
> -  ];
> -
> -  // Write the ping to file and make it overdue.

nit: drop "and make it overdue."

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
(Diff revision 2)
> -  await TelemetryStorage.savePing(PING_OLD_FORMAT, true);
> -  await TelemetryStorage.savePing(PING_NO_INFO, true);
> -  await TelemetryStorage.savePing(PING_NO_PAYLOAD, true);
> -  await TelemetryStorage.savePingToFile(PING_NO_SLUG, PING_FILES_PATHS[3], true);
> -
> -  for (let f in PING_FILES_PATHS) {

Instead of deleting the whole test, let's just drop this for loop.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
(Diff revision 2)
>    };
>  
>    const filePath =
>      Path.join(Constants.Path.profileDir, PING_SAVE_FOLDER, PING_OLD_FORMAT.slug);
>  
> -  // Write the ping to file and make it overdue.

Instead of deleting the whole line, just drop "and make it overdue"

Comment 16

[Arjun Krishna Babu]

Comment on attachment 8986441 [details]
Bug 1469233 - remove pingsOverdue from simpleMeasurements;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251808/diff/2-3/

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8986441 [details]
Bug 1469233 - remove pingsOverdue from simpleMeasurements;

https://reviewboard.mozilla.org/r/251808/#review259546

This looks good to me now, thanks! Let's see how it behaves on try ;)

Comment 18

[Alessio Placitelli [:Dexter]]

Hey! Looks like this requires a few more changes, as the try job "ES" (eslint) is failing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16f4b4b72ea2f24659b3046b3c50bcf1bf739e49&selectedJob=184866390

Comment 19

[Arjun Krishna Babu]

(In reply to Alessio Placitelli [:Dexter] from comment #18)
> Hey! Looks like this requires a few more changes, as the try job "ES"
> (eslint) is failing:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=16f4b4b72ea2f24659b3046b3c50bcf1bf739e49&selectedJob=1
> 84866390

"ES" seems fixable. What about bc2, bc6, X4?

And, thank you for all the support and help you've been providing me :)

Comment 20

[Arjun Krishna Babu]

Comment on attachment 8986441 [details]
Bug 1469233 - remove pingsOverdue from simpleMeasurements;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251808/diff/3-4/

Comment 21

[Alessio Placitelli [:Dexter]]

(In reply to Arjun Krishna Babu from comment #19)
> (In reply to Alessio Placitelli [:Dexter] from comment #18)
> > Hey! Looks like this requires a few more changes, as the try job "ES"
> > (eslint) is failing:
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=16f4b4b72ea2f24659b3046b3c50bcf1bf739e49&selectedJob=1
> > 84866390
> 
> "ES" seems fixable. What about bc2, bc6, X4?
> 
> And, thank you for all the support and help you've been providing me :)

My pleasure :) The others don't seem to be related. It can happen that some tests fail due to racy calls and stuff. We call them "intermittents", as they are not reproducible 100% of the times. Unfortunately, this is more frequent than desirable. However, you can usually tell the difference between a failure your code is causing and one intermittent: if something that you did not touch is failing and it's not calling your code, then it's *probably* not your fault:)

Comment 22

[Alessio Placitelli [:Dexter]]

Would you mind checking if linting is fine now?

> ./mach eslint toolkit/components/telemetry

The command above should do the trick :)

Comment 23

[Arjun Krishna Babu]

(In reply to Alessio Placitelli [:Dexter] from comment #22)
> Would you mind checking if linting is fine now?
> 
> > ./mach eslint toolkit/components/telemetry
> 
> The command above should do the trick :)

Err, yeah. Let me install nodejs, npm, eslint etc first. I'll figure it out :)

Comment 24

[Arjun Krishna Babu]

(In reply to Alessio Placitelli [:Dexter] from comment #22)
> Would you mind checking if linting is fine now?
> 
> > ./mach eslint toolkit/components/telemetry
> 
> The command above should do the trick :)

Apparently, linting is fine now. I ran the command, and the output is
> ✖ 0 problems (0 errors, 0 warnings)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e97d629e0b08
remove pingsOverdue from simpleMeasurements; r=Dexter

Comment 26

[Arjun Krishna Babu]

(This is probably a stupid question.)
So what happens now?

Comment 27

[Alessio Placitelli [:Dexter]]

(In reply to Arjun Krishna Babu from comment #26)
> (This is probably a stupid question.)
> So what happens now?

We sit tight and wait for the patch to land and get merged :) You can follow the progress through this link: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e97d629e0b086fe774158536704a6369cdc82af4

If all the tests are green, it will get merged within a day.

Comment 28

[Raul Gurzau (:RaulG)]

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