1342377 - store-failure-lines FailureLine.objects.create() "TypeError: 'group' is an invalid keyword argument for this function"

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Data Ingestion, defect, P2)

Description

[Ed Morley [:emorley]]

This log contains a new `group` key in each failure line:
https://public-artifacts.taskcluster.net/bjdwfJjOQx-worBDrGCoLQ/0/public/test_info//valgrind-plain_errorsummary.log

(Presumably the output of a try run for bug 1340551)

These lines are handled via:

    def create(job_log, log_list):
        failure_lines = [
            FailureLine.objects.create(repository=job_log.job.repository,
                                       job_guid=job_log.job.guid,
                                       job_log=job_log,
                                       **failure_line)
            for failure_line in log_list]
        job_log.update_status(JobLog.PARSED)
        return failure_lines

(See https://github.com/mozilla/treeherder/blob/495fc898fd29c7fe760aef93fe9151735433e70d/treeherder/log_parser/failureline.py#L84-L92)

Using `**failure_line` seems error-prone.
The fields should be listed explicitly given it's user-provided data.

Comment 1

[Ed Morley [:emorley]]

https://rpm.newrelic.com/accounts/677903/applications/14179757/filterable_errors#/show/ef99def5-fa2d-11e6-9c7c-b82a72d22a14_0_5119/stack_trace?top_facet=transactionUiName&primary_facet=error.class&barchart=barchart&_k=ghy7oa

Comment 2

[Andrew Halberstadt [:ahal]]

Just to note, in bug 1340551 it looks like we're going the route of printing all tests and their groups at the start of the error summary, rather than adding a new 'group' key to each log line. That may or may not make this bug invalid.

Comment 3

[Ed Morley [:emorley]]

I believe the bug is still valid.

There are two ways we decide to treat the failure line schema:
1) As immutable
2) As something that will evolve over time via bugs/try pushes similar to that one

For case one, we presumably want to skip parsing the log, but (a) the error message should be clearer, (b) this shouldn't result in an exception in New Relic.

For case two, new properties will require schema changes in Treeherder, so can't be auto-ingested even if we wanted, so the parser should gracefully handle them prior to someone following up with a Treeherder bug to add the new feature.

Comment 4

[James Graham [:jgraham]]

Yes, this is a real bug. I will fix it. Having said that changing the failure_line schema in the database is super-painful; I'm not sure what to do about that if we start adding more information here.

Comment 5

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:failure_line_allow_list > mozilla:master

Comment 6

[Ed Morley [:emorley]]

Comment on attachment 8840920 [details] [review]
[treeherder] mozilla:failure_line_allow_list > mozilla:master

Thanks! :-)

Comment 7

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/75b64d7c1cee5620f419a66a8d14ce2b23bdb9e3
Bug 1342377 - Only store failure_line data from known errorsummary keys (#2200)

Comment 8

[Ed Morley [:emorley]]

Bug 1340551 has now landed, which has caused ~3000 exceptions on prod in the last 3 hours, which is only going to increase as it gets merged around.

I'll deploy to prod now.

Comment 9

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:tbpl_formatter_unknown_action > mozilla:master

Comment 10

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/b6fd5778bb3e8ad0c0de8a43aa6d7ffb9c445374
Bug 1342377 - Ignore unsupported actions when crossreferencing error lines (#2210)