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?

(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.

(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.

ni?alessio for mentoring setup.

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

I'm interested in this. Is Alessio Placitelli the mentor?

(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?

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.

(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 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?

(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 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 ;)

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

(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 :)

(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:)

Would you mind checking if linting is fine now? > ./mach eslint toolkit/components/telemetry The command above should do the trick :)

(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 :)

(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)

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.