1189425 - Remove unparsable pings from the disk

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Alessio Placitelli [:Dexter]]

We should remove pings on JSON parsing errors and maybe on some/all load failures too.

Comment 1

[Georg Fritzsche [:gfritzsche]]

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.

Comment 2

[Vladan Djeric (:vladan)]

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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

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

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Remove unparsable pings from the disk

Comment 5

[Georg Fritzsche [:gfritzsche]]

Unloadable pings are covered by bug 1189425.

Comment 6

[Vladan Djeric (:vladan)]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ff71dfc40c70

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff71dfc40c70

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Remove unparsable pings from the disk

Comment 10

[Georg Fritzsche [:gfritzsche]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d9e06a2aa15