Whoopsie

Merge lp://staging/~ted/whoopsie/whitelist-large-fields into lp://staging/whoopsie

whitelist-large-fields
Merge into trunk

Proposed by Ted Gould on 2014-08-15

Status:	Merged
Merged at revision:	630
Proposed branch:	lp://staging/~ted/whoopsie/whitelist-large-fields
Merge into:	lp://staging/whoopsie
Diff against target:	331 lines (+177/-46) 3 files modified src/tests/data/crash/valid (+110/-0) src/tests/test_parse_report.c (+4/-0) src/whoopsie.c (+63/-46)
To merge this branch:	bzr merge lp://staging/~ted/whoopsie/whitelist-large-fields
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Brian Murray (community)		Approve on 2014-08-28
Evan (community)	2014-08-15	Needs Information on 2014-08-17
Review via email: mp+231011@code.staging.launchpad.net

Commit message

Allow whitelist to specify which fields can be large, not which are included

Description of the change

This branch changes the behavior of the whitelist of fields slightly. Instead of specifying which ones are included at all, it specifies which ones have unrestricted size. All of the fields get included if they have a size of less that 1KB. For the record:

<ev> tedg: sometimes we have big fields we want to carefully split
<ev> like Stacktrace :)
<tedg> ev, Sure, so let the whitelist be to avoid the size limits not to be in/out.
<tedg> ev, So, < 1KB or in whitelist
<ev> so any field below a certain size plus these big ones we care about?
<ev> yeah
<ev> works for me
<ev> I'll +1 that MP
<ev> but remind me I said this :)
<tedg> ev, K, I'll work on that MP

Revision history for this message

Evan (ev) wrote on 2014-08-17:

Looks good, but I'm not yet sold on pruning unwanted keys out. Question inline.

review: Needs Information

Revision history for this message

Ted Gould (ted) wrote on 2014-08-18:

On Sun, 2014-08-17 at 19:35 +0000, Evan Dandrea wrote:

> > +
> > + /* Remove entries that we don't want to send */
>
> I'm curious to know your motivation for this approach. Why load everything into memory and prune out the keys we don't want rather than avoiding adding them to the hash table in the first place?

Started going that route, but the problem was that because of multi-line
entries we don't know the length of the item until we process the next
key, so it started to get tricky and feel "clever." Opted for the
simpler code, which allowed for taking out the ignore_key field as well,
simplifying the loop. It seems like, for the most part, the largest
fields of the crash file are the ones we want anyway (coredump, stack
trace, etc.) so the cost wasn't that high.

Revision history for this message

Brian Murray (brian-murray) wrote on 2014-08-19:

Some fields are deliberately not included in the whitelist, such as Stacktrace and ThreadStacktrace, because they aren't useful until after the crash report has been retraced. Additionally, there are some we don't care about (at least according to the comments in acceptable_fields) like: Title, CrashCounter, and UnreportableReason.

I certainly don't think its useful to be sending Stacktrace and ThreadStacktrace, so I think there should probably be an unacceptable_fields which would include fields we'd never want from apport.

review: Needs Fixing

lp://staging/~ted/whoopsie/whitelist-large-fields updated on 2014-08-28

630. By Ted Gould on 2014-08-28: Add a list of fields that we never want in a report

Revision history for this message

Ted Gould (ted) wrote on 2014-08-28:

On Tue, 2014-08-19 at 19:22 +0000, Brian Murray wrote:

> I certainly don't think its useful to be sending Stacktrace and
> ThreadStacktrace, so I think there should probably be an
> unacceptable_fields which would include fields we'd never want from
> apport.

There is now such a list, those fields are removed always.

Revision history for this message

Brian Murray (brian-murray) wrote on 2014-08-28:

Thanks for making that change, I'll go ahead and merge this now.

review: Approve