Closed Bug 1469233 Opened 4 years ago Closed 4 years ago

Remove pingsOverdue from simpleMeasurements

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: gfritzsche, Assigned: arjunkrishnababu96, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js][good-first-bug])

Attachments

(1 file, 1 obsolete file)

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/
Alessio, Chris, do you see us needing these?

If not this, could one of you set it up for a mentored bug?
Flags: needinfo?(chutten)
Flags: needinfo?(alessio.placitelli)
(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.
Flags: needinfo?(alessio.placitelli)
(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.
Flags: needinfo?(chutten)
ni?alessio for mentoring setup.
Flags: needinfo?(alessio.placitelli)
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
Mentor: alessio.placitelli
Flags: needinfo?(alessio.placitelli)
Priority: -- → P3
Summary: Remove pingsOverdue & savedPings from simpleMeasurements → Remove pingsOverdue from simpleMeasurements
Whiteboard: [lang=js][good-first-bug]
I'm interested in this. 

Is Alessio Placitelli the mentor?
Flags: needinfo?(alessio.placitelli)
(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?
Assignee: nobody → arjunkrishnababu96
Flags: needinfo?(alessio.placitelli)
Attached patch bug1469233-intended-changes.diff (obsolete) — Splinter Review
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.
Flags: needinfo?(alessio.placitelli)
(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 ;)
Flags: needinfo?(alessio.placitelli)
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.
(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?
Flags: needinfo?(alessio.placitelli)
(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!
Flags: needinfo?(alessio.placitelli)
Attachment #8986174 - Attachment is obsolete: true
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 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 ;)
Attachment #8986441 - Flags: review?(alessio.placitelli) → review+
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
Flags: needinfo?(arjunkrishnababu96)
(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 :)
Flags: needinfo?(arjunkrishnababu96) → needinfo?(alessio.placitelli)
(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:)
Flags: needinfo?(alessio.placitelli)
Would you mind checking if linting is fine now?

> ./mach eslint toolkit/components/telemetry

The command above should do the trick :)
Flags: needinfo?(arjunkrishnababu96)
(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 :)
Flags: needinfo?(arjunkrishnababu96)
(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)
Flags: needinfo?(alessio.placitelli)
Flags: needinfo?(alessio.placitelli)
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e97d629e0b08
remove pingsOverdue from simpleMeasurements; r=Dexter
(This is probably a stupid question.)
So what happens now?
(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.
https://hg.mozilla.org/mozilla-central/rev/e97d629e0b08
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.