-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Summary: Display thresholds values even when not in summaryTrendStats #4698
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
Conversation
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 on my end 🚀
(I believe removing the logs 👇🏻 would fix the tests 🙇🏻 )
internal/js/summary.js
Outdated
console.log(info); | ||
console.log(metric.values); | ||
|
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.
console.log(info); | |
console.log(metric.values); |
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, sorry, I forgot one extra force-push before opening the PR haha 😅
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.
Hehe no worries 😄 Been there, did that, many times 🫶
616bd35
to
17d877e
Compare
@@ -839,7 +839,7 @@ function renderThresholdResults( | |||
: formatter.decorate(failMark, 'red'); | |||
|
|||
const sourceText = formatter.decorate( | |||
`'${threshold.source}'`, |
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 isn't directly related, but it's so small that I prefer to add it as part of this PR. It's not a huge deal, but I think it's nice if we trim the threshold's source before displaying it in the summary.
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.
What is the reason for trimming here? If the source includes spaces why we don't interpret it as intentional?
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.
If the source includes spaces why we don't interpret it as intentional?
I'm a bit confused now; is there any reason for doing so?
To me, it feels like when you mistakenly add a leading or trailing whitespace in an input designed to write your name or similar.
Looking at the threshold syntax expression (ref), I can see for instance users adding (or not) whitespaces before/after the <operator>
, for readability, but not why doing so at the beginning/end of the threshold definition.
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.
Just an observation: the user experience might be tricky here. Because now we might have the potential scenarios where disabled stats appear in the summary.
I know that isn't a new limitation introduced here, instead it should highlight the need to track/untrack specific metrics as described in #1321, where we should extend the same concept to stats as well.
@@ -839,7 +839,7 @@ function renderThresholdResults( | |||
: formatter.decorate(failMark, 'red'); | |||
|
|||
const sourceText = formatter.decorate( | |||
`'${threshold.source}'`, |
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.
What is the reason for trimming here? If the source includes spaces why we don't interpret it as intentional?
@@ -1061,6 +1065,10 @@ function computeSummaryInfo(metrics, renderContext, options) { | |||
const nonTrendExtras = {}; | |||
const trendCols = {}; | |||
|
|||
// While "trendCols" contain the values for each "trendStats" aggregation (e.g. p(90) as a sorted array, | |||
// "trendKeys" is used to store specific aggregation values that aren't part of "trendStats"; mainly for thresholds. | |||
const trendKeys = {}; |
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.
// While "trendCols" contain the values for each "trendStats"
It doesn't sound right without applying in-depth thinking. Because Trend is itself a stat. Wouldn't it be easier to just use the concept of Threshold?
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 not sure 🤷🏻 The code/data structure is quite generic, and while it's true that this is now used for threshold, it could technically be used for anything else, so I tried to keep it consistent with the existing "conventions" (if any) - one data structure holds stats identified by "columns", and this one by "keys" 😅
Overall, that code has many aspects that could be improved (there's already some // TODOs
), and @oleiade already did a great job fixing some as part of #4089, but I'd prefer if we can keep focus on fixing the issue that's been reported, and re-evaluate namings and possible improvements over this code in another PR/ as part of another issue/work.
I'm sorry @codebien, but I don't fully get your point here. As far as I can see, #1321 is all about metrics and sub-metrics, but here we only care about different "calculations" (percentiles, basically") of already existing metrics/sub-metrics, so I cannot see how it related 🤔 Conceptually I see those two as different, unrelated things. |
@joanlopez, my interpretation of the issue is that the user wants to add a threshold for a specific metric at a particular percentile without adding that percentile to all the other metrics. If that's not the case, then I guess we could simply suggest using However, neither of these solutions addresses the user's specific requirement. Instead, I think 1321 might address it, if we integrate the proposed API with an option for defining the stats. Something similar to the example below: export const options = {
summaryTrendStats: ['max', 'p(95)'],
};
track("http_req_duration", {percentile: 99.9}) Where without the |
Let me slightly correct you, because I think it's key here: "...at a particular percentile without displaying that percentile in summary to all the What I mean here is basically two things (in my humble opinion):
I disagree with both, because:
That's definitely out of the scope of this PR, so I'd postpone that discussion for whenever we design that feature, but I'm not super convinced that having to specify what calculations do we want to track is really a good idea, because calculations aren't that associated with noise/high memory consumption, which I think are the underneath goal of that feature, and doing so would make both the design and implementation of such feature way more complex. cc/ @oleiade any take here? |
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 want to unblock the merge for the pull request, so it mostly looks good to me. I want to check what is the context to provide an answer for #4698 (comment), but I don't have time today, I will do it at the end of the week. 🙇
@joanlopez However, the CI seems to be red, can you check that is not related, please? |
It isn't, it's just one of the flaky Browser E2E tests that define a thresholds that isn't always satisfied. |
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.
🚀 🍾
What?
Makes it possible to display threshold values (since #4089) even when the aggregation on the thresholds expression (e.g.
p(99.5)
) isn't included as part ofoptions.summaryTrendStats
.Why?
Now those values aren't displayed (instead, it prints
undefined
, which I would consider a bug), because we're not calculating values for aggregations other thanoptions.summaryTrendStats
.Alternatively, we could just omit these lines from the summary, but I think these are valuable information for the user, and it's aligned with the goal of the revamped summary.
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 #4695