Skip to content

Conversation

marun
Copy link
Contributor

@marun marun commented Aug 27, 2024

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

  • Defaults to emitting a metrics link at the end of spec execution
    • A test can disable this behavior by setting e2e.EmitMetricsLink to false
  • Composes a metrics link in a global ginkgo AfterEach handler for the shared network UUID and the start/end times of the current spec
    • The end time is extended by the network shutdown delay to account for the network scrape interval

How this was tested

CI

@marun marun self-assigned this Aug 27, 2024
@marun marun force-pushed the e2e-per-test-metrics branch 2 times, most recently from 0ea1618 to ef2fb5e Compare August 27, 2024 19:34
@marun marun changed the title [e2e] Emit a grafana link for each test scoped to the duration of its execution [e2e] Emit a duration-scoped grafana link for each test Aug 27, 2024
@marun marun force-pushed the e2e-per-test-metrics branch 2 times, most recently from 5a02656 to a11b047 Compare August 27, 2024 19:38
@marun marun added the testing This primarily focuses on testing label Aug 27, 2024
@marun marun force-pushed the e2e-per-test-metrics branch from a11b047 to 62d6253 Compare August 27, 2024 20:02
@marun marun force-pushed the e2e-per-test-metrics branch from 62d6253 to c3a4d13 Compare August 27, 2024 20:04
@marun marun marked this pull request as ready for review August 27, 2024 20:06
@marun marun force-pushed the e2e-per-test-metrics branch from a2eb7d0 to 501b151 Compare August 27, 2024 20:31
Co-authored-by: Stephen Buttolph <stephen@avalabs.org>
Signed-off-by: marun <maru.newby@avalabs.org>
@StephenButtolph StephenButtolph modified the milestone: v1.11.11 Aug 29, 2024
@StephenButtolph StephenButtolph added this to the v1.11.12 milestone Sep 4, 2024
Comment on lines +15 to +16
// Ensure that this value takes into account the scrape_interval
// defined in scripts/run_prometheus.sh.
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Comment on lines +39 to +41
if env == nil || !EmitMetricsLink {
return
}
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@aaronbuchwald aaronbuchwald added this pull request to the merge queue Sep 4, 2024
Merged via the queue into master with commit ca0228a Sep 4, 2024
21 checks passed
@aaronbuchwald aaronbuchwald deleted the e2e-per-test-metrics branch September 4, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing This primarily focuses on testing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants