Bug 199992 – [ews-build] EWS fails to parse multi-line full_results.json

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

Buildbot split the full_results.json output (or any log line for that matter) into multiple lines when it is longer than 4096 characters (as per https://github.com/buildbot/buildbot/blob/master/master/buildbot/util/lineboundaries.py#L31). This happens when there are lot of layout-test failures and so json output is long.

Due to multi-line, json.loads() fails to parse the content, and fails with an exception. e.g.: https://ews-build.webkit-uat.org/#/builders/37/builds/110

EWS should gracefully handle the multi-line json content.

[Aakash Jain]

Created attachment 374576 [details]
Patch

[EWS Watchlist]

Attachment 374576 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1086:  [TestRunWebKitTests.test_parse_results_json_with_newlines] Passing unexpected keyword argument 'state_string' in function call  [pylint/E1123] [5]
ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1086:  [TestRunWebKitTests.test_parse_results_json_with_newlines] No value passed for parameter 'status_text' in function call  [pylint/E1120] [5]
ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1088:  [TestRunWebKitTests.test_parse_results_json_with_newlines] Instance of 'TestRunWebKitTests' has no 'assertEqual' member  [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1089:  [TestRunWebKitTests.test_parse_results_json_with_newlines] Instance of 'TestRunWebKitTests' has no 'assertEqual' member  [pylint/E1101] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 374576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374576&action=review

> Tools/BuildSlaveSupport/ews-build/layout_test_failures.py:54
> +        # Concatenate content into single line since Buildbot split lines longer than 4096 characters

Nitpick: s/split/splits/, and there should be a period at the end.

Since the root cause of the problem is at JSON generation side, this should probably have a link to a bug about fixing that, or a brief explanation of why it's not possible, and why the workaround is permanent. After all, if we produce something that we call "JSON", that should be valid JSON, not a broken one.

> Tools/BuildSlaveSupport/ews-build/layout_test_failures.py:56
> +        content_string = ''.join(content_string.splitlines())

Is this better than .translate(None, '\r\n')?

[Aakash Jain]

> Is this better than .translate(None, '\r\n')?
It seems to work for strings, but not for unicode, noticing following error (similar to https://stackoverflow.com/a/23306505)
>>> 'a\nb'.translate(None, '\r\n')
'ab'
>>> u'a\nb'.translate(None, '\r\n')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: translate() takes exactly one argument (2 given)

[Aakash Jain]

> Since the root cause of the problem is at JSON generation side, this should
> probably have a link to a bug about fixing that, or a brief explanation of
> why it's not possible, and why the workaround is permanent. After all, if we
> produce something that we call "JSON", that should be valid JSON, not a broken one.
Actually the generation of json is fine, since that's generated and written to disk directly by run-webkit-tests. When buildbot reads the file and interprets its contents as logs, it splits them, if the line is longer than 4096 characters.

To confirm, if you look at the test run with this patch applied, which is https://ews-build.webkit-uat.org/#/builders/37/builds/115, the json in Buildbot logs is split in two lines in https://ews-build.webkit-uat.org/#/builders/37/builds/115/steps/9/logs/json
however the raw full_results.json (from the uploaded archive) is single line: https://ews-build.webkit-uat.org/results/macOS-High-Sierra-Debug-WK1-Tests-EWS/r374496-115/full_results.json

[Aakash Jain]

Created attachment 374585 [details]
Patch for landing

[Alexey Proskuryakov]

> TypeError: translate() takes exactly one argument (2 given)

For Unicode strings the translate function works like this:

u'a\r\nb'.translate({10: None, 13: None})

Again, I don't know if it's actually better. Assuming a reasonable implementation, it may be faster and less memory hungry.

> When buildbot reads the file and interprets its contents as logs, it splits them,
> if the line is longer than 4096 characters.

I think that my comment stands - this is a bug whose root cause is not here, this is a workaround.

[Alexey Proskuryakov]

Is it the .txt extension that makes Buildbot do this? How exactly does it decide that it's OK to corrupt the file?

[Aakash Jain]

(In reply to Alexey Proskuryakov from comment #7)
> > TypeError: translate() takes exactly one argument (2 given)
> 
> For Unicode strings the translate function works like this:
> 
> u'a\r\nb'.translate({10: None, 13: None})
> 
> Again, I don't know if it's actually better. Assuming a reasonable implementation, it may be faster and less memory hungry.
ok. I'll prefer to stick with the splitlines() and join approach as it works with both str and unicode, the json file isn't so large that the two approach would make any significant memory/speed difference.


> I think that my comment stands - this is a bug whose root cause is not here, this is a workaround.
Yeah, it's kind of a Buildbot bug, I just filed https://github.com/buildbot/buildbot/issues/4906. I'll add a comment.


> Is it the .txt extension that makes Buildbot do this? How exactly does it decide that it's OK to corrupt the file?
It's irrespective of extension and file type. If Buildbot is reading the file as a log file, any line longer than 4096 character would be split in multiple line. This splitting logic was added to fix a performance issue we were facing in earlier version of Buildbot (https://github.com/buildbot/buildbot/issues/3444#issuecomment-322133208).

[Alexey Proskuryakov]

Do we have any control over whether Buildbot reads it as a log file?

[Aakash Jain]

(In reply to Alexey Proskuryakov from comment #10)
> Do we have any control over whether Buildbot reads it as a log file?
Nope, the other option is to just directly read the file from disk.

Note that this issue is for other tests as well, like webkitpy, webkitperl, api, jsc etc. e.g.: https://ews-build.webkit.org/#/builders/5/builds/3486/steps/7/logs/json

But we already have fix similar to one proposed in this patch for those tests (although that can be slightly improved), e.g.: https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-build/steps.py#L536
https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-build/steps.py#L596

[Aakash Jain]

Created attachment 374636 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 374636 [details]
Patch for landing

Clearing flags on attachment: 374636

Committed r247710: <https://trac.webkit.org/changeset/247710>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53424229>