Remove unparsable pings from the disk

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: Dexter, Assigned: gfritzsche)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 wontfix, firefox42 fixed, firefox43 fixed)

Details

(Whiteboard: [unifiedTelemetry] [uplift5])

Attachments

(1 attachment, 1 obsolete attachment)

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: 4 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.