Skip to content
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

lazy-load evaluation errors #1363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

lazy-load evaluation errors #1363

wants to merge 1 commit into from

Conversation

ajs124
Copy link
Member

@ajs124 ajs124 commented Feb 22, 2024

Closes #1362

Comment on lines -111 to -117
[% IF eval.evaluationerror.errormsg %]
<div id="tabs-errors" class="tab-pane">
<p>Errors occurred at [% INCLUDE renderDateTime timestamp=(eval.evaluationerror.errortime || eval.timestamp) %].</p>
<div class="card bg-light"><div class="card-body"><pre>[% HTML.escape(eval.evaluationerror.errormsg) %]</pre></div></div>
</div>
[% END %]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evals did in fact ship the eval error log twice, so we're killing that as well. 🤷

@delroth
Copy link
Contributor

delroth commented Feb 22, 2024

Future improvement: IF eval.evaluationerror.errormsg still loads the full errormsg from the DB, when in practice we could get away with just checking for whether it's empty or not (and loading from the DB only on the separate eval error page).

It's likely a slightly more involved fix though, just mentioning it in case you're interested :p

@delroth
Copy link
Contributor

delroth commented Feb 22, 2024

To whoever will merge this: please cherry-pick it on the 2.19 branch so we can easily apply it to hydra.nixos.org.

@delroth
Copy link
Contributor

delroth commented Feb 26, 2024

FWIW we've tested this on hydra.nixos.org for a few days and everything worked fine.

@Ericson2314 can you merge this please?

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw evaluationerrors is also part of prefetch in Nix::getEvals, if we want to get rid of it in the first query, we'll probably need to touch that part as well.

That said, I only know because I failed to read the entire conversation first and wondered how lazy loading works on ORM-side (which made me enable query logging in my postgresql 😅).

Anyways, the change looks good to me and works fine on my personal instance 👍

@mweinelt
Copy link
Member

@Ma27 If you know what to do, please give it a shot.

@mweinelt
Copy link
Member

The JSON API for jobsets/evals also ships the whole eval errors string, which is utterly pointless. Can we rip that out as well?

curl --header "Accept: application/json" https://hydra.nixos.org/jobset/nixpkgs/staging-next 👻

@Ma27
Copy link
Member

Ma27 commented Mar 23, 2024

@Ma27 If you know what to do, please give it a shot.

I'll happily do that, but it may take me a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eval errors make jobset and eval pages very expensive to serve and browse
4 participants