Configurable Trend stats #64
Conversation
codebien
force-pushed
the
stats-option
branch
from
da31615
to
3023011
Compare
| // TODO: remove the assumption that Name label is the first. | ||
| func sortByNameLabel(s []*prompb.TimeSeries) { | ||
| sort.Slice(s, func(i, j int) bool { | ||
| return s[i].Labels[0].Value <= s[j].Labels[0].Value |
There was a problem hiding this comment.
Beotian question, but won't that break if Labels is empty?
| @@ -76,6 +85,50 @@ func (o *Output) Stop() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // setTrendStatsResolver sets the resolver for the Trend stats. | |||
| // | |||
| // TODO: refactor, the code can be improved | |||
There was a problem hiding this comment.
I would find the TODO comment more valuable to our future selves if it mentioned how it could be improved 😄
| // | ||
| // TODO: maybe decoupling mapping from the stat resolver keys? | ||
| if strings.HasPrefix(statKey, "p(") { | ||
| statKey = stat[2 : len(statKey)-1] // trim the parenthesis |
There was a problem hiding this comment.
I believe you could use strings.TrimPrefix here instead 👍🏻
| assert.Len(t, o.trendStatsResolver, len(tt.expResolverKeys)) | ||
| assert.ElementsMatch(t, tt.expResolverKeys, func() []string { | ||
| var keys []string | ||
| for statKey := range o.trendStatsResolver { |
There was a problem hiding this comment.
I think you could use copy here instead. That would make the code even shorter?
|
@oleiade thanks, your request changes are internal changes, so I'm going to merge then address them in a dedicated PR. |
codebien
force-pushed
the
stats-option
branch
from
decf778
to
923d6b6
Compare
|
@codebien makes sense 🙇🏻 |
| var defaultTrendStats = []string{"p(99)"} | ||
|
|
There was a problem hiding this comment.
For me it makes some sense that the default value should match whatever the value of --summary-trend-stats in k6 is (default or user-supplied). But I can also see why that might be a bit too expensive to maintain by default... So, I don't have a strong opinion either way, sorry.
No description provided.