Machine-readable summary respects SUMMARY_TREND_STATS#5790
Machine-readable summary respects SUMMARY_TREND_STATS#5790Reranko05 wants to merge 2 commits intografana:masterfrom
Conversation
|
|
ankur22
left a comment
There was a problem hiding this comment.
Thanks for the contribution and for digging into this — the issue is real and worth fixing.
After tracing through the code, the root cause is that the TrendValues struct in internal/lib/summary/machinereadable/ is code-generated from the grafana/k6-summary schema (via cog), and the schema itself only defines the six standard trend stats (avg, min, max, med, p(90), p(95)). The fix in machine_readable.go is working around the schema rather than fixing it at the source.
The right path forward is likely:
- Update the
grafana/k6-summaryschema to support additional/dynamic percentiles - Regenerate the Go types via
make generate - Update
machine_readable.goto map the new schema fields
Tagging @joanlopez as the original author of the machine-readable schema integration (#5338) to weigh in on the best way to extend the schema for dynamic percentiles.
We'll hold off on reviewing the code changes themselves until the schema question is settled.
| }) | ||
| } | ||
|
|
||
| func TestMachineReadable_IncludesP99(t *testing.T) { |
There was a problem hiding this comment.
Please add more tests and fix the implementation. We want the exact thigns set - so if you set that you jstu want avg, you should get this. Please add a few tests tahta are just 1,2,3 things and some of them are custome p sucha as p(12) or p(42)
There was a problem hiding this comment.
Thanks for the suggestions! Adding more targeted tests makes sense.
Since there’s also an ongoing discussion about addressing this at the schema level, I’ll wait for clarity on that direction before refining the implementation and expanding the test coverage accordingly.
|
Hi @ankur22, thanks for the detailed explanation, that makes a lot of sense. I see now that the issue originates from the generated schema rather than the output layer. I’ll wait for input on the schema changes before proceeding further. |
What?
This PR updates the machine-readable summary output to respect the values specified in
SUMMARY_TREND_STATS.When additional trend statistics such as
p(99)are configured, they are currently computed but not included in the machine-readable output. This change preserves such values by falling back to a dynamic representation when unsupported fields are present.Why?
The current implementation only supports a fixed set of trend statistics (
avg,min,max,med,p(90),p(95)) via the generatedTrendValuesstruct.However,
SUMMARY_TREND_STATSallows specifying additional percentiles likep(99). These values are silently dropped in the machine-readable output, leading to inconsistency between computed metrics and exported results.This PR ensures that all computed trend statistics are preserved without modifying the generated schema.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #5736