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

Switch metrics back to Diagnostics #129

Merged
merged 2 commits into from
Nov 10, 2022
Merged

Conversation

lalexgap
Copy link
Contributor

@lalexgap lalexgap commented Oct 7, 2022

Fixes #128

When using results (runenv.R()) all the metrics are sent to the database at the end of the run. If the run is long this can be quite a large set of data, which fails to get sent to the database with the error Request Entity Too Large.

This switches most of our metrics back to diagnostics (runenv.D()) which immediately writes out to the database. The only metrics that are still posted to results are the ci/nightly summary metrics. Since these are summary metrics about the whole test run it seemed more appropriate for them to post to results.

This does mean that most of the metrics are now excluded from the dashboard link on the tasks page. However the dashboard link is to a bunch of autogenerated graphs that aren't really useful, so I don't think that's a big loss.

TODO:

  • Compare a run with this change against main. This PR vs main

@lalexgap lalexgap marked this pull request as ready for review November 8, 2022 03:16
@lalexgap lalexgap requested a review from geoknee November 8, 2022 17:10
@lalexgap lalexgap self-assigned this Nov 8, 2022
@geoknee
Copy link
Contributor

geoknee commented Nov 9, 2022

TODO:

  • Compare a run with this change against main. This PR vs main

Both of these links result in "dashboard not found" for me?

@geoknee
Copy link
Contributor

geoknee commented Nov 9, 2022

The rationale for this change makes sense to me. The testground docs don't really give a clear steer on which API we ought to use, but the tradeoffs seem clear and worth making the change to diagnostics.

Before approving: there are lots of seemingly unrelated changes on this PR. Are they intentional?

@lalexgap
Copy link
Contributor Author

lalexgap commented Nov 9, 2022

The rationale for this change makes sense to me. The testground docs don't really give a clear steer on which API we ought to use, but the tradeoffs seem clear and worth making the change to diagnostics.

Before approving: there are lots of seemingly unrelated changes on this PR. Are they intentional?

Good catch! I think some changes were added via a rebase. I'll clean that up.

@lalexgap lalexgap changed the base branch from main to latest-dashboards November 9, 2022 16:57
@lalexgap lalexgap merged commit 71b2bfc into latest-dashboards Nov 10, 2022
@lalexgap lalexgap deleted the switch-to-diag branch November 10, 2022 18:47
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.

Long running tests can fail to record metrics
2 participants