842824 - Some Health Report payloads are over 1MB

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

The Telemetry Dashboard shows that some uncompressed FHR payloads are over 1MB. There even appears to be at least 1 over 2MB! I'd love to see the content of one of these payloads so we can see where the space is used.

Over to Daniel to procure a raw document or two.

Comment 1

[Mark Reid [:mreid]]

Attachment: A few samples of FHR payloads that are >1MB uncompressed

Here are a few sample docs.  Let me know if you need more / different ones.  There were 315 docs in total with sizes > 1M bytes.

At a glance, the culprit appears to be the session info.

Comment 2

[Gregory Szorc [:gps]]

WTF. There are hundreds of sessions in here. And, it looks like they are unique! Here are some "main" times:

[1002,1002,1062,1002,1062,1038,1002,1062,1038,1008,1002,1062,1038,1008,1103,1002,1062,1038,1008,1103,1015,1002,1062,1038,1008,1103,1015,1092,1002,1062,1038,1008,1103,1015,1092,1002,1002,1062,1038,1008,1103,1015,1092,1002,1136,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,843,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,843,1024,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,843,1024,1062,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,843,1024,1062,1071,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,843,1024,1062,1071,979,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064,1024,1006,1049,1002,1010,1020,1068,1012,1100,1264,1010,1010,994,1008,1006,843,1024,1062,1071,979,990,1002,1062,1038,1008,1103,1015,1092,1002,1136,1030,1061,1013,963,1006,979,1022,863,1064

My guess is these are submissions from an automation/testing system.

Comment 3

[Gregory Szorc [:gps]]

Upon further examination of the payloads, I'm seeing some repeated sequences. Specifically, I'm seeing a lot of aborted sessions.

What I think is happening is that we move the sessions from prefs to the DB. However, before the prefs changes are flushed to disk, the session crashes or pref flush does not occur properly.

When the new session comes online, the old, already-added-to-DB preferences reappear. We then proceed to add these to the database yet again (we blindly insert into the database - there is no duplication detection here). Ad infinitum.

The general problem is FHR makes changes to prefs. Prefs somehow aren't saved. Then, FHR proceeds to perform the same actions over and over (including possibly resubmitting a payload multiple times per day).

If we want to address this issue, we have to ensure state changes are persisted. I hate to say it, but we may need to incur a preferences flush (currently synchronous I/O) after key FHR preference changes. Sad panda.

Comment 4

[Gregory Szorc [:gps]]

It's worth noting that a more immediate solution to address just the session problem is to record the last saved session number in FHR. If FHR sees a session number less than what it has already stored, it will not re-add it. Since FHR is using a robust data store (not preferences), we can rely on this value.

Comment 5

[Richard Newman [:rnewman]]

Heh, I clicked the bug link to leave basically that comment. We should be able to use or add some field that we're putting into prefs to decide whether it was previously flushed.

Comment 6

[(dormant account)]

(In reply to Gregory Szorc [:gps] from comment #3)
> 
> If we want to address this issue, we have to ensure state changes are
> persisted. I hate to say it, but we may need to incur a preferences flush
> (currently synchronous I/O) after key FHR preference changes. Sad panda.

No. The proper way to do this is to flush things off main thread and not use prefs. Don't perpetuate badness.

Comment 7

[Gregory Szorc [:gps]]

I just landed bug 843816 in inbound. This should prevent us from getting into a cycle where we continually add the same sessions to FHR. And best of all, we don't need to incur a main thread flush of the prefs!

It might be worth adding a follow-up patch to clean FHR's database of bad entries. Otherwise, impacted users will have to live with the side-effects for up to 180 days, when FHR will prune the old/bad data. Since this bug only bit us on Nightly, I'm very tempted to do something crude like "if N > 10 sessions per day, clear all sessions for that day." Or, we could just do a one-time truncate of all the sessions data for Nightly. Somebody from Metrics should weigh in here. Essentially, I want you to tell me the crudest (read: easiest) method of cleanup I can get away with. Daniel?

I'd like to remind people that this bug is part of a much larger one: preference writes aren't robust against failure. People everywhere rely on preferences to persist state. When this fundamental assumption of preferences doesn't hold, components fail in many ways. From talking with a number of developers, I believe there is a void in Gecko for a robust key-value store that fits somewhere between preferences and SQLite, satisfies performance requirements, and is easy to use. Until we have such a system and features start using it, we'll be left with many bugs like this one and/or a lot of code inventing their own data stores on top of file I/O (which is especially fragile when you consider performance on mobile devices). I don't like this status quo.

Comment 8

[Richard Newman [:rnewman]]

We should call it PDS.

Comment 9

[Daniel Einspanjer [:dre] [:deinspanjer]]

I always preferred to have the session info stored as histograms, but that isn't a quick fix either.

Let's go with truncating more than 100 sessions per day.  I don't want it to be too small since it isn't something we can easily change our mind on later.

Comment 10

[Gregory Szorc [:gps]]

(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #9)
> I always preferred to have the session info stored as histograms, but that
> isn't a quick fix either.

It could be. Write up a proposal in a new bug and we'll look at it.

Keep in mind we also had bug 840206 and https://metrics.etherpad.mozilla.org/95 talking about major changes to how session data is reported. I'm not sure if you are talking about jumping straight to that or doing it piecemeal. Plus Perf wants us to do session recording in Gecko/C++ (bug 841561). We should probably all discuss how we want all of this to play out. Will the new session reporting format be proved out in JS? Or, will we jump straight to C++? If the latter, will we secure a resource to write the C++ in time to meet deadlines (I'm assuming you'd like session improvements in 22 and /possibly/ backported to 21, if possible). This might also impact our JS/C++ decision - I imagine we could swing a JS backport easier than new C++ code. Yay fodder for the next status meeting!

Comment 11

[Gregory Szorc [:gps]]

I filed bug 848461 to get some data on pref write persistence into Telemetry.

Comment 12

[Richard Newman [:rnewman]]

Looks good now, I think.