Skip to content

Revamp the end-of-test summary #4089

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

Merged
merged 60 commits into from
Mar 11, 2025
Merged

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented Dec 4, 2024

Contributes to #4052

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 --summary-mode=compact, or with no argument, as it is the default choice.
    • the full summary with --summary-mode=full.
    • the legacy summary with --summary-mode=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 --summary-mode=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 and others added 14 commits October 21, 2024 13:05
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
@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
mstoykov
mstoykov previously approved these changes Mar 10, 2025
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I have done one more pass, and given the time restraint (and the size of a bunch of the code) and that it is mostly internal - I am okay with merging this as is.

Please do move lib/summary.go under internal package - maybe internal/lib just to get it out of the way - although having a summary package is probably a good idea.

For the record this was not used last I checked during the move packages that are not used in internal - just it is part of lib package which is imported a lot.

@joanlopez joanlopez force-pushed the new-end-of-test-summary-output branch from bfdbc98 to 3404474 Compare March 10, 2025 17:01
@joanlopez joanlopez force-pushed the new-end-of-test-summary-output branch 3 times, most recently from 7e4f74a to 0dfdf7e Compare March 11, 2025 12:39
@joanlopez joanlopez force-pushed the new-end-of-test-summary-output branch from 0dfdf7e to 4fc0440 Compare March 11, 2025 14:52
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Do not forget to squash this 😅

@joanlopez
Copy link
Contributor Author

Do not forget to squash this 😅

Since we don't have a convention (or a strong one? 🤔), and personally I feel like what's atomic and what not is blurry, ambiguous and subjective, I generally squash all my PRs, so it would be very weird that specially for this one I misclick on "Merge", but thanks for the reminder.

@joanlopez joanlopez requested a review from olegbespalov March 11, 2025 16:29
Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

LGTM! Great work! 🚀

Thinking loudly: it seems like the summary mode could also take responsibility of displaying no summary, which makes the UX more straightforward, but it's probably not a big deal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants