Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 3014663002: [pinpoint] Add trace links.

Created:
2 years ago by dtu
Modified:
2 years ago
Reviewers:
perezju, shatch, sullivan
CC:
catapult-reviews_chromium.org, perf-dashboard-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

[pinpoint] Add trace links. BUG=catapult:#3887

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -39 lines) Patch
M dashboard/dashboard/pinpoint/elements/execution-status.html View 2 chunks +4 lines, -0 lines 0 comments Download
M dashboard/dashboard/pinpoint/models/quest/read_value.py View 3 chunks +15 lines, -8 lines 3 comments Download
M dashboard/dashboard/pinpoint/models/quest/read_value_test.py View 5 chunks +46 lines, -31 lines 0 comments Download

Messages

Total messages: 8 (1 generated)
dtu
2 years ago (2017-09-26 22:45:46 UTC) #2
sullivan
lgtm \o/
2 years ago (2017-09-27 01:44:32 UTC) #3
perezju
lgtm
2 years ago (2017-09-27 08:33:07 UTC) #4
shatch
https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoint/models/quest/read_value.py File dashboard/dashboard/pinpoint/models/quest/read_value.py (right): https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoint/models/quest/read_value.py#newcode51 dashboard/dashboard/pinpoint/models/quest/read_value.py:51: if not self._trace_url: Getting a failure here with no ...
2 years ago (2017-09-27 11:55:50 UTC) #5
perezju
https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoint/models/quest/read_value.py File dashboard/dashboard/pinpoint/models/quest/read_value.py (right): https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoint/models/quest/read_value.py#newcode77 dashboard/dashboard/pinpoint/models/quest/read_value.py:77: if self._trace: On 2017/09/27 11:55:50, shatch wrote: > The ...
2 years ago (2017-09-27 12:24:06 UTC) #6
shatch
On 2017/09/27 12:24:06, perezju wrote: > https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoint/models/quest/read_value.py > File dashboard/dashboard/pinpoint/models/quest/read_value.py (right): > > https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoint/models/quest/read_value.py#newcode77 > ...
2 years ago (2017-09-27 14:48:44 UTC) #7
perezju
2 years ago (2017-09-27 15:38:13 UTC) #8
On 2017/09/27 14:48:44, shatch wrote:
> On 2017/09/27 12:24:06, perezju wrote:
> >
>
https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoin...
> > File dashboard/dashboard/pinpoint/models/quest/read_value.py (right):
> > 
> >
>
https://codereview.chromium.org/3014663002/diff/1/dashboard/dashboard/pinpoin...
> > dashboard/dashboard/pinpoint/models/quest/read_value.py:77: if self._trace:
> > On 2017/09/27 11:55:50, shatch wrote:
> > > The bug linked (technically the one duped and closed) is for surfacing all
> the
> > > trace links for perf try jobs, but this looks like it'll only surface a
> single
> > > trace link?
> > 
> > chartjson (I think) only keeps a link to one of the traces. :(
> > 
> > And, in most cases, since we're using --pageset-repeat 1 this should be OK
(I
> > think).
> > 
> 
> I thought there's a trace produced for every story for every repeat?

Yes, there is one trace for every story repeat, but only one of them survives to
the chartjson file due to:
https://github.com/catapult-project/catapult/blob/1b6b78dad5017a6279f9f02a553...

See e.g.:
https://luci-logdog.appspot.com/v/?s=chrome%2Fbb%2Fchromium.perf%2FAndroid_Ne...

There is only one trace per story, despite each story running 3 times.

This is not an issue in histograms json (AFAIK).

> So this
> approach would work fine for bisect but presumably a perf try job could run
> without a story filter.

Yes, but note this chartjson reader only gets either:

- values for a particular story (confusingly named "trace" here due to legacy
chartjson-reasons)
- or the summary values (i.e. averages over all stories for the selected
metric).

In the later case I think this code gets no traces at all.

Aside, it breaks my head trying to remember this mapping, probably should be
added to some docstring here:

chart -> metric (e.g.
"memory:chrome:browser_process:reported_by_os:proportional_resident_size_avg")
tir_label -> story_group (e.g. "load_news")
trace -> story (e.g. "load:news:cnn") or "summary"

Also I known they don't always match exactly for all benchmarks, but maybe we
could use the "new" names in the code here? (follow up CL?)

Powered by Google App Engine
This is Rietveld 408576698