ping files that can't be read should be deleted

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla18
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Currently they'll just sit around and sit around and we'll keep error on reading them.
(Assignee)

Comment 1

6 years ago
Created attachment 652172 [details] [diff] [review]
patch
Attachment #652172 - Flags: review?(taras.mozilla)

Comment 2

6 years ago
Comment on attachment 652172 [details] [diff] [review]
patch

99% sure this .remove will fail on windows since the file is still open. Should add a testcase
Attachment #652172 - Flags: review?(taras.mozilla) → review-
Since the stream is closed, this _might_ work. However, indeed, a testcase should remove all doubt.
(Assignee)

Comment 4

6 years ago
Created attachment 663408 [details] [diff] [review]
part 1 - a bit of refactoring

We're going to need the ability to write fake ping files in test_TelemetryPing.js; we might as well have a function for just doing it.
Attachment #652172 - Attachment is obsolete: true
Attachment #663408 - Flags: review?(taras.mozilla)
(Assignee)

Comment 5

6 years ago
Created attachment 663409 [details] [diff] [review]
part 2 - delete ping files if there's an error whilst reading them

Closing streams appears to be idempotent, so making sure the stream is closed prior to deleting the file shouldn't cause any problems.
Attachment #663409 - Flags: review?(taras.mozilla)
(Assignee)

Comment 6

6 years ago
Created attachment 663410 [details] [diff] [review]
part 3 - add tests

This became much easier when I realized I didn't have to shoehorn it into the ping/response logic earlier in the file.
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #663410 - Flags: review?(taras.mozilla)

Updated

6 years ago
Attachment #663408 - Flags: review?(taras.mozilla) → review+

Comment 7

6 years ago
Comment on attachment 663409 [details] [diff] [review]
part 2 - delete ping files if there's an error whilst reading them

This is good. We should however record success/fail ratios here.
Attachment #663409 - Flags: review?(taras.mozilla) → review+

Comment 8

6 years ago
Comment on attachment 663410 [details] [diff] [review]
part 3 - add tests

This looks good, but as I mentioned above we should record success/failure in a histogram and check the histogram gets recorded correctly in the testcase. Please post that as a followup patch.
Attachment #663410 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 664569 [details] [diff] [review]
part 4 - add telemetry for reading saved ping files

I can add more complete checking of the contents of the histogram if you like, but it didn't seem worth it.
Attachment #664569 - Flags: review?(taras.mozilla)

Updated

6 years ago
Attachment #664569 - Flags: review?(taras.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.