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

Revamp the end-of-test summary #4089

Open
wants to merge 47 commits into
base: master
Choose a base branch
from

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented Dec 4, 2024

Overview

This pull request changes the current end-of-test summary by a new design, with two available formats:

  • a) compact (default)
  • b) full
    aiming to bring clearer a more valuable results to users. Find a screenshot below.

User-facing details

  • 💹 The appearance of the end-of-test summary is now different, with thresholds at the beginning (instead on being inlined with metrics), checks slightly modified and metrics grouped by category (http, ws, network, etc).
  • 🔠 The user can choose between:
    • the compact summary with --with-summary=compact, or with no argument, as it is the default choice.
    • the full summary with --with-summary=full.
    • the legacy summary with --with-summary=legacy.
  • ⚠️ The data model passed into the custom handleSummary function is now different. So, those users relying on it must migrate their implementation or use --with-summary=legacy meanwhile.
    • 🙏🏻 (Dear reviewer) If you definitely think we should keep the old format, I can give it a try and see if I can accommodate the new to it before calling the function, without many extra allocations (I guess that since we don't propagate sinks here it should be fine in general). But please, comment it explicitly, exposing the reasons and what's your concrete proposal.

Technical details (for review purposes)

  • The core logic of the new end of test summary, and how it collects metrics, etc is based on a new output.Output named summary.
  • There's a new test script as example under internal/cmd/testdata/summary/... with different scenarios, groups, thresholds, custom metrics and what not... that can be used for both automated and manual testing. If you think anything is missing, just suggest it.
  • The JS code responsible for rendering the summary have been largely refactored and type-documented, 👏🏻 big shot-out to @oleiade, aiming to make that code way more maintainable that it has been until now. I guess that, once we merge this PR, we may need to copy-past it back to https://github.com/grafana/k6-jslib-summary.
  • I left two data structures for the summary representation (lib.Summary vs lib.LegacySummary), to keep support for the legacy summary for some time, in an easy way, so things aren't complexity mixed and the the clean up in the future is simpler, just by removing that type, all the references to it, and simplifying the few conditionals that behave depending on which summary type is provided.
    • Similarly, I left the old JS code for the summary as the summary-legacy.js, for simpler cleanup whenever we remove that support, which I guess might be for v2 (once we ship the formalized JSON output format within v1).

Internal Checklist

Before review readiness

  • Add support for a compact summary mode (likely enabled by default, so perhaps a "extended" flag, to avoid memory allocation issues), that only "relays" metrics but not stores metrics for groups and scenarios.
  • Revisit sorting: is there anything else we can sort for this first iteration (note that some ideas have been moved to a second iteration, see below)
  • Take a decision on what do we want to do with the existing summary, whether we want to use the new lib.Report as the new API for custom handleSummary (note this would be a breaking change), or if we want to ship this progressively.
    • Review and modify (if needed) the code in js/summary.go according to that decision.
  • Review and refactor the code in js/summary.js:
    • All the pieces from the old summary can probably be removed, if not removed yet.
    • De-duplicate the functions summarizeMetrics and summarizeMetricsWithThresholds, or just replace the first with the second one (as we may no longer need the first one if we remove the old summary code).
  • Review the structure and code of the output/summary package:
  • Verify all scenarios (different test scripts) work as expected (test with no groups nor scenarios, test with only groups, test with only scenarios, test with the combination of both, all of them with compact mode enabled or not).
  • Make the CI look 🟢 as an 🍏 again!
  • (Optional) Define the JSDoc for the newly introduced functions in JS code.
  • Remove the playground/full-summary files, or define a proper location for them.
    --- Ideas left for a second iteration---
  • Re-write the JS code in TS.
  • Sorting the thresholds by metric name, sorting the tags within the metric name and perhaps sorting the sources.
  • Implementing a more optimized data structure (as explored in https://github.com/joanlopez/xk6-custosummary/tree/main/timeseries) to use less memory allocations while keeping scenarios & groups samples.

General

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

@joanlopez joanlopez requested a review from a team as a code owner December 4, 2024 17:58
@joanlopez joanlopez requested review from mstoykov and olegbespalov and removed request for a team December 4, 2024 17:58
@joanlopez joanlopez marked this pull request as draft December 4, 2024 17:58
@oleiade oleiade force-pushed the new-end-of-test-summary-output branch from 58138ac to 126a188 Compare December 17, 2024 15:58
@joanlopez joanlopez force-pushed the new-end-of-test-summary-output branch from 1904999 to 4bb8f7a Compare February 7, 2025 21:11
@joanlopez joanlopez force-pushed the new-end-of-test-summary-output branch from 4bb8f7a to 9c1ed70 Compare February 7, 2025 21:14
@joanlopez joanlopez marked this pull request as ready for review February 7, 2025 21:59
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.

2 participants