This is an archive of the discontinued LLVM Phabricator instance.

[LNT][RFC] Introduce Latest Runs Report.
ClosedPublic

Authored by marxin on Aug 29 2018, 12:56 AM.

Details

Summary

We've started using LNT with about 80 machines and we noticed it's crucial to check regressions and progress on a single place.
Daily Report looked very promising tool, but I found some limitations:

  • we have machines that very rare frequency (once a month), so that only way to see a progress is to use num_days=big_value
  • you need to have a Run that ends precisely in Day 0, it's not much handy if you want to see recent activity
  • the Daily Report does quite some juggling with Orders and selects the recent one in a day frame
  • that makes the code very complex (240 LOC)

Thus I made a simple report, called it for now Latest Runs. It jsut takes last N runs of each machine and does comparison. From implementation
point of view it consists of only 80 LOC and is very simple.

I also factored out common code shared in between Daily Report and the new report. Hope it can help
acceptance of the patch.

Static example of the report: https://drive.google.com/file/d/1PPFYugVg4Q3lhYxG6q2J4BUifj91JQkI/view?usp=sharing

Diff Detail

Repository
rL LLVM

Event Timeline

marxin created this revision.Aug 29 2018, 12:56 AM

The new report and its implementation look good to me! However before committing this needs at least a basic unit test (tests/server/ui/V4Pages.py) to check that the page renders without exception and produces valid html.

Testcases?

lnt/server/reporting/dailyreport.py
3 ↗(On Diff #163022)

Please use explicit imports.

lnt/server/reporting/latestrunsreport.py
3 ↗(On Diff #163022)

no * imports

lnt/server/ui/templates/reporting/latest_runs_report.html
9 ↗(On Diff #163022)

Does something else close this body tag, or do you need to?

lnt/server/ui/views.py
12 ↗(On Diff #163022)

Are you using these in your patch? I don't see them.

1532 ↗(On Diff #163022)

+ comment on why this can happen.

1533 ↗(On Diff #163022)

400 is "the server could not understand the request due to invalid syntax." This endpoint has no arguments, so that does not make sense. What is the error condition this is responding to?

cmatthews added inline comments.Aug 29 2018, 10:52 AM
lnt/server/reporting/latestrunsreport.py
101 ↗(On Diff #163022)

no space around keywords.

lnt/server/ui/views.py
1521 ↗(On Diff #163022)

Not a redirect (http 302).

1526 ↗(On Diff #163022)

Sort of obvious comment.

1527 ↗(On Diff #163022)

It might be handy to have the report length as a url parameter and default to 10. Sometimes the reports can be very slow, so it is nice to tweak how much data they look at.

cmatthews requested changes to this revision.Aug 29 2018, 10:53 AM

This is a great idea. Thanks for setting it up!

This revision now requires changes to proceed.Aug 29 2018, 10:53 AM
MatzeB added inline comments.Aug 29 2018, 10:53 AM
lnt/server/reporting/dailyreport.py
3 ↗(On Diff #163022)

+1

434–452 ↗(On Diff #163022)

where did this block of code go to and where is report_css_styles coming from, is there a file missing?

marxin updated this revision to Diff 163301.Aug 30 2018, 4:30 AM
marxin marked 2 inline comments as done.
marxin marked 10 inline comments as done.Aug 30 2018, 4:33 AM

Changes from previous version:

  • I removed try, except in v4_latest_runs_report
  • I added at least check_html test, once installed I add some real checking content
  • accordion toggle added to HTML report, it's easier to navigate now
  • all nits you mentioned should be fixed now
lnt/server/reporting/dailyreport.py
434–452 ↗(On Diff #163022)

I isolated styles into report_css_styles which is now shared by both reports.

marxin updated this revision to Diff 164022.Sep 5 2018, 5:51 AM
marxin marked an inline comment as done.

One more update where I add accordion for easier orientation on the report page.

May I please ping this.

May I please remind this review..

cmatthews accepted this revision.Sep 27 2018, 1:32 PM

I bet some of these report queries are going to be really slow on big databases. But we can fix them up over time if that is the case. LGTM.

This revision is now accepted and ready to land.Sep 27 2018, 1:32 PM
This revision was automatically updated to reflect the committed changes.