-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add optional 'count' column to CLI summary output #1143
Conversation
802da9e
to
ac255fc
Compare
Codecov Report
@@ Coverage Diff @@
## master #1143 +/- ##
=========================================
+ Coverage 73.77% 75.1% +1.33%
=========================================
Files 147 147
Lines 10695 10709 +14
=========================================
+ Hits 7890 8043 +153
+ Misses 2345 2200 -145
- Partials 460 466 +6
Continue to review full report at Codecov.
|
ac255fc
to
e76e2a9
Compare
ui/summary.go
Outdated
if err == nil { | ||
newTrendColumns = append(newTrendColumns, TrendColumn{stat, percentileTrendColumn}) | ||
// Validate checks if defined trend columns are valid | ||
func (s *Summary) Validate() error { |
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 can technically be private, since it's called implicitly in NewSummary()
and not used anywhere else, but validation functions should probably be public.
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.
I'm wondering if this shouldn't actually be a separate function, not a method of Summary
? That way you can use it in the config handling without having to go through the NewSummary
invocation and result discarding...
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.
Thanks @na--, I like that. Will change.
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.
Resolved by b4a776d, but it's a bit sloppy since getDefaultResolvers()
is now a thing. But it's certainly nicer than before. Thanks for the suggestion.
Though this file and ui
in general need a bigger cleanup out of scope for this PR.
Feel free to resolve this if there's nothing pending.
ui/summary.go
Outdated
"min": func(s *stats.TrendSink) interface{} { return s.Min }, | ||
"med": func(s *stats.TrendSink) interface{} { return s.Med }, | ||
"max": func(s *stats.TrendSink) interface{} { return s.Max }, | ||
"p(90)": func(s *stats.TrendSink) interface{} { return s.P(0.90) }, |
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.
Are these actually necessary, given the fact that we have a way to generate arbitrary percentiles in generateCustomTrendValueResolvers()
?
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.
I considered that, but decided against it since p(90)
and p(95)
will pretty much always be the default percentiles, so it doesn't make sense to conditionally create them. This way it clearly shows what the defaults are.
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.
Yeah I am wondering the same ... given the code below, won't removing those two line .... continue to work as before ?
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.
@mstoykov Yes, it will, but like I said, I think it's clearer defining them upfront. Generating them muddles up the default vs. custom concept (though we could just call it generatePercentileResolvers
), and it would have to go through validatePercentile
for no reason.
But let me know if you feel strongly about it and I'll remove them.
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.
To be honest I think that https://github.com/loadimpact/k6/pull/1143/files#diff-cc7288c8b0cb64e706ac188a9e524b7fR82 should be set in https://github.com/loadimpact/k6/pull/1143/files#diff-b5b20214939c9d689266a9dccbd0c47cR66 as the default and it should this should not be named getDefaultResolvers
, but something like: getCustomResolvers/ getNonPercentileResolvers ?
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.
I would think that with #961 we are probably going to add some way for all the non percentile ones to be parsed directly from the name like avg
or even avg+count
(totally useless). (and yes I know that the issue is about thresholds parsing not summary columns but I would argue that code will be reused).
At which point this map will be ... removed :). So from that perspective I don't know if it's actually worth the effort, to do it now.
At least for me (@na-- might still think that it's worth to do most of the mentioned things above, now). Adding the defaults to the message is probably the only thing that we should do right now. Everything else seems (given that issue I linked and it's apparent relevance to this code) not too important ... especially given that it creates even more code to be refactored when we start to fix the configuration ...
On the other hand I do agree with the first comment of @na-- as this lines are definitely not needed and again ... the defaults are defined few lines after this ... we just aren't calling them that, there ;)
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.
Yeah, to me getDefaultResolvers()
doesn't make a lot of sense. We basically have 2 things:
- The possible values that can be configured as trend columns in the summary. This includes a mix of static (
min
,max
, etc.) and dynamic (p(??)
) columns, as well as default and non-default (count
) values so each value should be validated by a function. And each value corresponds to a "resolver", i.e. something that returns its actual value. - The default values that will be displayed - this is an ordered subset of 1, so an unordered map can't be it (and why
getDefaultResolvers()
) is a bad name. This should be just a simple list (i.e. a slice), and yeah, the best UX would be to display its default value it in the CLI help, so the Fix handling cli options default value #1092 workaround may be 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.
OK, that sounds good.
So to summarize what needs to be done in this PR:
-
Remove the pre-generated percentile resolvers and rename
getDefaultResolvers
to ...? @na-- you need to be the tie-breaker here with a good name suggestion, since @mstoykov doesn't like mine and I don't like his, so... 😄 -
Move the definition of columns to show as the default value for
--summary-trend-stats
, and apply the Fix handling cli options default value #1092 fix (whatever that is, still need to grok how it works).
Would that cover it?
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.
- Remove the pre-generated percentile resolvers and rename
getDefaultResolvers
togetStaticResolvers()
, or to just heave it a simplemap
, since this map will always be the same... - The Fix handling cli options default value #1092 fix is that:
- we set the CLI flag default value to
nil
, but we manually set the help text there to include the actual default value (seesystemTagsCliHelpText
in that PR) - we have an
applyDefault
function that is called at the end of the config consolidation - it applies the default value for any complex types (SystemTags
in that PR,SummaryTrendStats
in this) that are still nil (i.e. that the user hasn't specified)
- we set the CLI flag default value to
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.
Let me know if b91229a resolves this.
There are some new globals (I kept in line with the SystemTags
implementation) and ad-hoc repeated validation and just general suckiness, but the configuration system is a pain to work with...
And I need to rebase and fix the merge conflicts, but I'm waiting on the final approvals before doing so.
ui/summary.go
Outdated
if err == nil { | ||
newTrendColumns = append(newTrendColumns, TrendColumn{stat, percentileTrendColumn}) | ||
// Validate checks if defined trend columns are valid | ||
func (s *Summary) Validate() error { |
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.
I'm wondering if this shouldn't actually be a separate function, not a method of Summary
? That way you can use it in the config handling without having to go through the NewSummary
invocation and result discarding...
As per [1], it would be preferable to keep this struct. Though I renamed some fields and removed the dependency on the entire lib.Options, since it only requires one value. This also renames Summary.Write to Summary.SummarizeMetrics, as it's a more accurate description of what happens, despite of the "Summary.Summarize" stutter. [1]: #1143 (comment)
…rror handling Resolves: [1], [2] [1]: #1143 (comment) [2]: #1143 (comment)
e76e2a9
to
855843e
Compare
Apologies for force-pushing. I wanted to rebase on |
ui/summary_test.go
Outdated
|
||
s.SummarizeMetrics(&w, " ", SummaryData{ | ||
Metrics: metrics, | ||
RootGroup: nil, |
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.
Can you add a RootGroup to cover even more of the code
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.
Looks okay to me, apart from the p(90)... comment from @na-- .
Can you run the test before and after the changes (with obviously some changes to the test code) in order to confirm that nothing has changed, apart from adding that count
?
ui/summary.go
Outdated
"min": func(s *stats.TrendSink) interface{} { return s.Min }, | ||
"med": func(s *stats.TrendSink) interface{} { return s.Med }, | ||
"max": func(s *stats.TrendSink) interface{} { return s.Max }, | ||
"p(90)": func(s *stats.TrendSink) interface{} { return s.P(0.90) }, |
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.
Yeah I am wondering the same ... given the code below, won't removing those two line .... continue to work as before ?
Good idea @mstoykov. It wasn't that difficult, take a look. That passes on current If this wasn't going to be merged soon, I'd create a PR for that test anyway. :) |
a40db55
to
91e144e
Compare
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.
LGTM, but wait for @na-- as this is quite a big change
Besides the two minor nits I noted above, this LGTM |
This adds an optional count column for Trend metrics in the CLI summary output, while refactoring parts of ui/summary.go for code style (avoiding globals, explicit validation, etc.) and negligible performance improvements (using a map for column lookups). To enable it run e.g. `k6 run --summary-trend-stats 'avg,p(99.99),count' test.js`. Closes #1087
Resolves: b91229a#r35621359 This changes current behavior on `master`, but as discussed above, it makes sense to respect whatever the user specifies, so this can be considered a fix.
Also add a few missing summary-trend-stats tests. Resolves: #1143 (comment)
8bbc027
to
51e88e0
Compare
This adds an optional
count
column for Trend metrics in the CLI summary output, while refactoring parts ofui/summary.go
for code style (avoiding globals, explicit validation, etc.) and negligible performance improvements (using a map for column lookups).To enable it run e.g.
k6 run --summary-trend-stats 'avg,p(99.99),count' test.js
.This essentially makes the existing default Count metrics like "http_reqs" and "iterations" obsolete, so we might want to consider making
count
a default column after all and removing these two metrics from the output.I added a test for
SummarizeMetrics
, so our coverage should improve a bit. :)Closes #1087