Closed Bug 1189425 Opened 5 years ago Closed 5 years ago

Remove unparsable pings from the disk

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: Dexter, Assigned: gfritzsche)

References

Details

(Whiteboard: [unifiedTelemetry] [uplift5])

Attachments

(1 file, 1 obsolete file)

We should remove pings on JSON parsing errors and maybe on some/all load failures too.
Blocks: 1122482
Whiteboard: [unifiedTelemetry]
Vladan, do you see any big problems with removing unloadable & unparsable pings from disk right away?

There is a slight risk of removing pings that are only "temporary" not readable (say for weird AV behavior etc.).
On the other hand, we can get stuck storing and trying to send pings that we can never load.

Implementing limited retry means complicating the implementation (further data to persist etc.), i'd rather avoid that unless there are pressing reasons.
Flags: needinfo?(vdjeric)
I don't have a problem with removing unparseable pings, I think that behaviour was in the old TelemetryPing.jsm telemetry too.

I wonder if you can actually remove an unreadable ping. Seems like if you can't read a ping, you probably can't remove it either.

Also, I'd be worried about adding in either change while we're trying to figure out holes in subsession chains. Either land it in Nightly 43 and don't uplift, or wait for the validation to be complete and then evaluate the data loss (if any) caused by this change.
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2)
> Also, I'd be worried about adding in either change while we're trying to
> figure out holes in subsession chains. Either land it in Nightly 43 and
> don't uplift, or wait for the validation to be complete and then evaluate
> the data loss (if any) caused by this change.

Yes, i'm not planning to uplift that. This should ride on 43 or 42.
Assignee: nobody → gfritzsche
Blocks: 1195426
See Also: → 1195160
Status: NEW → ASSIGNED
Attachment #8648891 - Flags: review?(vdjeric)
Unloadable pings are covered by bug 1189425.
Comment on attachment 8648891 [details] [diff] [review]
Remove unparsable pings from the disk

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

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +1443,5 @@
>      let array;
>      try {
>        array = yield OS.File.read(aFilePath, options);
>      } catch(e) {
>        throw new PingReadError(e.message);

log that you're ignoring it cause it's unreadable, it might make debugging easier

@@ +1457,5 @@
>          ping.payload = JSON.parse(ping.payload);
>        }
>      } catch (e) {
> +      yield OS.File.remove(aFilePath).catch((ex) => {
> +        this._log.trace("loadPingFile - failed removing unparseable ping file", ex);

i would log this as a warning or error
Attachment #8648891 - Flags: review?(vdjeric) → review+
Whiteboard: [unifiedTelemetry] → [unifiedTelemetry] [uplift5]
Iteration: --- → 43.1 - Aug 24
https://hg.mozilla.org/mozilla-central/rev/ff71dfc40c70
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Attachment #8648891 - Attachment is obsolete: true
Comment on attachment 8653368 [details] [diff] [review]
Remove unparsable pings from the disk

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry
[User impact if declined]:
Telemetry keeps trying to load & send broken pings forever, this fixes it to remove pings it can not parse.
[Describe test coverage new/current, TreeHerder]:
Automation coverage, fine on Nightly.
[Risks and why]:
Low-risk, small isolated change and on Nightly for a while.
[String/UUID change made/needed]:
None.
Attachment #8653368 - Flags: review+
Attachment #8653368 - Flags: approval-mozilla-aurora?
Attachment #8653368 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1195426
You need to log in before you can comment on or make changes to this bug.