Issue 793433  has been merged into this issue.

IIUC these artifacts need to be stored in test-results with the rest of the test result JSON. martiniss@: Is test-results server already set up to store and return the new artifact fields?

#4: Telemetry benchmark's test results already have the artifact fields. There are still work to be done on recipe side to upload those artifacts to cloud storage (issue 772216) but I think SOM can just use some example test data to not be blocked?

Lowering the priority on this since I believe we can't immediately act on this on the SoM side yet? Let me know if this is incorrect. 

We can raise the priority again once the data is available. I think I'd prefer to wait until we have the artifact data is available to implement this to keep things simple. 

But short-term, happy to discuss how we can show this data on SoM's UI. If you have any ideas about how you want this data to look on SoM's side, that would be useful for us to keep in mind once we implement this. General information on which pieces of the data are most useful to present to sheriffs/how sheriffs will generally be expected to act on the data would also be useful to help us figure out the UI. 

I'd also be happy to try to create some UI mockups for how the data could be surfaced in SoM but have limited understanding of how artifacts fit into the perf workflow. 

Thanks for the update, Tiffany. I will block this on issue 772216

The CL to add the data to json test results has landed.  Raising the priority for this in Q2.  

When we consolidate the perf test recipe, logs will be hard to consume from the buildbot ui.  We would like to be able to consume these in each alert from SOM.

In terms of presentation, in the json test results format the artifacts have a hierarchical list of type to cloud storage link.  See design doc above for the exact format.  

For each alert even just adding these links in a tree view to each entry so we can click through to the data would be a good v1. 

ping on this bug to see what the eta is

No change in status at this time, but I'll put this on my list of things to work on next week.

Hi Sean, any update on this bug?

No, but it's on my list of things to work on this week.

The spec is wrong or the implementation is wrong.

Spec doc example:
 {“testNodeBase”:
      “expected”: “PASS PASS”,
      “actual”: “PASS FAIL”,
      “artifacts”: [
       [
         {
           “name”: “android logcat”,
		# This is what the location field looks like pre-upload
           “location”: “/logcat/testNodeBase_1st_run.txt”,
         },
         {
           “name”: “battor crash log”,
		# This is what the location field looks like post-upload.
           “location”: “deadbeef123123123123”,
         },
         {
           “name”: “Test page screenshot”,
           “location”: “/logcat/testNodeBase_1st_run.txt”,
         },  
       ]
       [
          {
           “name”: “android logcat”,
           “location”: “/logcat/testNodeBase_2nd_run.txt”,
         },         ...
       ],


Reality, from https://ci.chromium.org/buildbot/chromium.perf/Mac%20Air%2010.11%20Perf/2213     

"IdleStory_60s": {
        "actual": "PASS",
        "artifacts": {
          "logs": [
            "https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/7448c73a-591c-11e8-8342-38c98650ac7f"
          ]
        },
        "expected": "PASS",
        "is_unexpected": false,
        "time": 101.842453956604,
        "times": [
          101.842453956604
        ]
      }
    }

Notice that it doesn't match the format in the doc, namely that it has a map where the keys are the artifact names instead of an array of objects where the name is explicit property of the content elements.

According to the spec, the above should be this, no?

        "artifacts": {
          [ { 
"name": "logs",
           "location":"https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/7448c73a-591c-11e8-8342-38c98650ac7f"
            }
          ]
        },

I might have changed it and not updated the spec. Let me check.

FWIW I like the spec better than what's implemented. 

https://chromium.googlesource.com/chromium/src/+/master/docs/testing/json_test_results_format.md is the canonical documentation, not the google doc linked above. Sorry that wasn't communicated. That doc has the same format as the actual build.

FWIW I *still* like the original spec better than what's implemented. 

Unconstrained JSON key names are why we can't have nice things. What was the justification for letting these be another wild west?

Err, ignore my comment in #21. The reason why we have Unconstrained JSON key names are the key names are human readable description of the data. They can be anything: 
battor log
Telemetry WPR log
Telemetry Log
Chrome crash dump
Android logcat
...

The thing that need to be constrained are their data type, which is described through MIME type

re #22 why does this not work?
"artifacts": [
  {
    "name": "battor log",
    "location": "https:/...."
  },
  {
    "name": "Telemetry WPR log"
    "location": "...."
  },
  // etc
]

?
 

IIRC it made the parsing and/or generation code harder to write. But I don't remember exactly why I made the decision. It was the format in #23, but we switched it to the current format.

#23: I recall folks wanted to save space, so discard "name" & "location" text

re #24: I assume it made the generation code harder to write, because it would have made the parsing code *easier* to write.

re #25: For the record, I don't find that argument convincing at all (given the size difference, esp. after compression) but I'll move on.

Thank you both for clarifying!

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/41bd01645e4f09b1284150f2917ce87f6d716925

commit 41bd01645e4f09b1284150f2917ce87f6d716925
Author: Sean McCullough <seanmccullough@chromium.org>
Date: Thu May 17 17:33:07 2018

[som] Modify the UI to render artifact elements if present in tests.

This uses a transformed version of the artifacts JSON format
that will have to be modified/generated on the server in a separate CL.

Bug:  772212 
Change-Id: I28488729a223afdfb09c25ae9a2a077c7a41ee7f
Reviewed-on: https://chromium-review.googlesource.com/1062955
Reviewed-by: Tiffany Zhang <zhangtiff@chromium.org>
Commit-Queue: Sean McCullough <seanmccullough@chromium.org>

[modify] https://crrev.com/41bd01645e4f09b1284150f2917ce87f6d716925/go/src/infra/appengine/sheriff-o-matic/frontend/elements/som-extension-build-failure/som-extension-build-failure.html
[modify] https://crrev.com/41bd01645e4f09b1284150f2917ce87f6d716925/go/src/infra/appengine/sheriff-o-matic/frontend/elements/som-extension-build-failure/som-extension-build-failure.js
[modify] https://crrev.com/41bd01645e4f09b1284150f2917ce87f6d716925/go/src/infra/appengine/sheriff-o-matic/frontend/test/som-extension-build-failure-test.html

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/ef8403528ebd30a7791f73562582735aa89a0297

commit ef8403528ebd30a7791f73562582735aa89a0297
Author: Sean McCullough <seanmccullough@chromium.org>
Date: Sat May 19 00:19:38 2018

[som] Server-side pieces to render perf test artifacts.

Bug:  772212 
Change-Id: Ib6719ca9dd14c0ee3f84a7cae79e128791ccf3e3
Reviewed-on: https://chromium-review.googlesource.com/1066819
Reviewed-by: Tiffany Zhang <zhangtiff@chromium.org>
Commit-Queue: Sean McCullough <seanmccullough@chromium.org>

[modify] https://crrev.com/ef8403528ebd30a7791f73562582735aa89a0297/go/src/infra/appengine/sheriff-o-matic/frontend/elements/som-extension-build-failure/som-extension-build-failure.html
[modify] https://crrev.com/ef8403528ebd30a7791f73562582735aa89a0297/go/src/infra/appengine/sheriff-o-matic/som/analyzer/step/test_step_test.go
[modify] https://crrev.com/ef8403528ebd30a7791f73562582735aa89a0297/go/src/infra/appengine/sheriff-o-matic/som/analyzer/step/test_step.go

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/e6b6983a1a2211635ce9e7045888bca2f5a0c4b0

commit e6b6983a1a2211635ce9e7045888bca2f5a0c4b0
Author: Sean McCullough <seanmccullough@chromium.org>
Date: Mon May 21 17:29:45 2018

[som] Remove unnecessary text.

Bug:  772212 
Change-Id: I7eec188593c7bcbbe44c8131bfd0a6e7598b00ae
Reviewed-on: https://chromium-review.googlesource.com/1067025
Reviewed-by: Tiffany Zhang <zhangtiff@chromium.org>
Commit-Queue: Sean McCullough <seanmccullough@chromium.org>

[modify] https://crrev.com/e6b6983a1a2211635ce9e7045888bca2f5a0c4b0/go/src/infra/appengine/sheriff-o-matic/frontend/elements/som-extension-build-failure/som-extension-build-failure.html

This is now live in prod: https://sheriff-o-matic-staging.appspot.com/chromium.perf

Awesome! Really cool to see this finally land :)