-
Notifications
You must be signed in to change notification settings - Fork 704
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[e2e] Emit a duration-scoped grafana link for each test #3340
Conversation
0ea1618
to
ef2fb5e
Compare
5a02656
to
a11b047
Compare
a11b047
to
62d6253
Compare
62d6253
to
c3a4d13
Compare
a2eb7d0
to
501b151
Compare
Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: marun <maru.newby@avalabs.org>
// Ensure that this value takes into account the scrape_interval | ||
// defined in scripts/run_prometheus.sh. |
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.
Was this meant as a TODO?
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.
Not a TODO - this is a reminder to coordinate changes to this value with the value in the script.
FWIW this should be temporary - when I have the cycles I plan to integrate starting of the log and metrics collectors into the e2e framework and that will remove the need for duplicating configuration.
if env == nil || !EmitMetricsLink { | ||
return | ||
} |
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.
Where is handling env == nil
necessary?
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.
This avoids a broken event handler in the case that there is no global env configured. This is the case for subnet-evm's load test suite for example, where the env is local-only:
https://github.com/ava-labs/subnet-evm/blob/master/tests/load/load_test.go#L60
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.
For future, please don't hesitate to wait until someone responds to merge. I would have been happy to add a comment here pre-merge.
Why this should be merged
When an e2e failure occurs in CI, it would be useful to start with a metrics link scoped to the duration of execution of the failing test.
How this works
e2e.EmitMetricsLink
tofalse
AfterEach
handler for the shared network UUID and the start/end times of the current specHow this was tested
CI