Dashboard - Dead code in testselection.html?

[simonhatch]

Came across this while investigating the monitored property on TestMetadata, looks like there's a lot of dead code in this class. The isValid field isn't isn't anywhere that I can find, and it calls getTests() which calls a whole bunch of other stuff.

@eakuefner @anniesullie

[anniesullie]

Looking at the code, I don't see how this could possibly be used anymore either. The original intent was to check if the "Add" button should be greyed out, but I can't see where this function is called, and the "Add" button clearly works properly. Delete away!

[simonhatch]

Yeah, I stripped away most of it, can't find accesses for anything except isMonitored. Mostly trying to strip away code to see where the monitored property is getting used more clearly.

[anniesullie]

The only place i know for sure the monitored property is used is in the suite menu, to give a hint to users. I don't think users find the hint valuable enough to keep it on just for that purpose. @benshayden is 'monitored' in the test listing ui for v2 spa? If not, did anyone ask you for it? If not, we should definitely get rid of this.

[simonhatch]

Been checking out/filing bugs on where it's used. There's a usage in list_tests but looks like it can be trivially replaced. There's 2 uses in the UI that I can see so far. The suite level one that you just mentioned, and something in chart-container uses the actual monitored list to decide if a test is "important" or not, haven't dug into it yet.

If the last one is replaced easily, we can talk about outright removing this property. It's annoying to maintain, it's not done transactionally so I'm pretty sure it get's out of sync, adds complexity via _pre_put_hook, and down the road will be a blocker for larger/collapsing multiple benchmarks since we'll blow entity size on that property.

[anniesullie]

Oh! I know what the "important" thing is about. It's trying to sort out what to show by default, and what to load in the background when you click a chart.

Let's say that you've asked to see the loading.desktop benchmark, and it has 80 stories. You probably want to look at the summary metric, and the ref for the summary metric if those exist, with the option to turn more on later. Most of the time, those are figured out by some naming convention. But we fall back on just showing the monitored metrics and their ref builds if nothing meets the naming convention.

But now that we're strongly recommending that all individual timeseries (and NOT average/summary metrics) are monitored, this is mostly useless. It's possible there may be a press benchmark or two where we're monitoring something called "Total", but generally users are able to find that for themselves pretty easily.

I think we can drop this feature without much user frustration. @benshayden probably has much better ideas on how to allow users to get the timeseries they want to see quickly in v2 spa.

[simonhatch]

Ah ok, I'll try to strip this out and see how it behaves. Otherwise, looks like it'd be possible to preserve it until v2spa with a bit of work.

I'll post a demo when it's working.

[simonhatch]

https://dev-simonhatch-d6d0bcb3-dot-chromeperf.appspot.com/report

Functionality difference isn't super obvious to me

[anniesullie]

I'm not sure what was stripped out in this demo, but I tried a bunch of benchmarks and can't find a difference in the display of the test suite menu or the "add" button or the chart selection/display in the legend.

[simonhatch]

I stripped out the "important" field from tests, which is the only placed that testselection.isImportant() was plumbed to.

Basically this: https://github.com/catapult-project/catapult/blob/master/dashboard/dashboard/elements/chart-container.html#L1146

And wherever that was referenced.

if you're not seeing much of a difference (I'm certainly not), feels like we can go ahead and remove the feature. That would only leave the suite level one, which as mentioned earlier in this thread probably isn't a super valuable tip in the UI.

[simonhatch]

Going to close this and can follow removing monitored in #3930

[benshayden]

V2SPA does not use 'monitored' in any way. I imagine that, if something like it becomes necessary again, we can re-implement it more cleanly.