-
Notifications
You must be signed in to change notification settings - Fork 160
Refactor new status page #2234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor new status page #2234
Conversation
…_request_as_completed`
The function returns errors associated with the requests, including both the error benchmark/context name and the error content. The query is also prepared, because it should be as fast as possible to not block the status page. Also adds some basic test builders for easier preparation of test state in tests.
Yes I think adding back the constants would be good. I haven't tested running this code locally however the constants are in lower case and the strings you have written are in title case. Which I think bolsters the justification for having them to avoid these discrepancies! Other than that looks good and as you say the query is much more readable 👍 |
The constants are in title case on the Rust side 😅 But more generally, using constants won't save us from differences between the frontend and the backend, and the TypeScript type-checker checks that we didn't make a mistake on the frontend side (since CI is green, the string values are fine). But as I said, if you really find it better, I'll add them back :) |
I'm a bit out of touch with TypeScript, if it were any other language I'd certainly lean on the use of symbolic constants; which is what we are doing in Rust. It makes things simpler to maintain. However if this is the idiomatic preferred way of doing things in TypeScript then lets leave it as it is. |
Yeah, TS is relatively unique in this, as it allows modelling string values (not just the string type) in the typesystem. So in that case I find it better to not use the constants, as it's both less code and it's easier to read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice tidy up 🚀
I examined the DB/backend/frontend logic of the new status page after the recent improvements, and it seemed to me like enough cruft from various experiments has been accumulated that we should first simplify the logic a bit before we move forward. In particular I found it very hard to understand what the SQL queries were doing anymore, so I tried to make them simpler, and only send data to the frontend that it really currently needs.
In the last N completed requests I removed the array aggregation. We need both
benchmark
anderror
fields from theerror
table, so we would need to somehow post-process the arrays anyway, and it was also causing some issues with nested NULLs when I switched toRIGHT JOIN
(because with theLEFT JOIN
, the query was loading essentially every artifact and error from DB and then filtering it).To test the changes, I implemented some helper test builders. I didn't yet port the existing tests to them, but I started writing new tests with them. The PR should be minus ~300 lines, if I subtract the new test code.
I simplified the frontend data model to match the old status page and avoid the enums, and I didn't yet bother adding back the status/type constant, but I'm fine with putting them back if you want.
Best reviewed commit-by-commit.