-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve cloud aggregation for different URLs #1220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1220 +/- ##
=========================================
Coverage ? 73.77%
=========================================
Files ? 147
Lines ? 10695
Branches ? 0
=========================================
Hits ? 7890
Misses ? 2345
Partials ? 460
Continue to review full report at Codecov.
|
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.
Besides the inline comment, you should also actually call sc.ReplaceURLWithName()
(or however the method is named) somewhere in here: https://github.com/loadimpact/k6/blob/c2d73ba070b5223d60e784401e9bda8d634440fe/stats/cloud/collector.go#L271-L272
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, only squash the commits before merging
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 prefer if you either:
- copy the whole sample but only if there is
1.1. url
1.2. name
1.3. url != name - don't use the sample's tags but work around them somehow - probably the more perfomant variant but will take more code
- shelf this until we have more optimized tags/ metric samples and we can fix it without introducing neither a race condition nor a big perf penalty
@mstoykov, see my inline comment why this isn't problematic from a data race perspective. Rather, the problem is that if you have multiple outputs, the cloud output's
So I'm for merging this as it is now, and properly fixing it when we implement the better cloud aggregation. |
Hmm although the performance overhead of copying the maps around wouldn't be too terrible, given the fact that we'd only do it if |
For cloud output, we currently don't aggregate HTTP metrics with different url tag, but with the same name tag. This PR improves aggregation by setting url tag to be the same as name tag. It's not ideal, but seems like the least bad option in term of UX. Fixes #1166
For cloud output, we currently don't aggregate HTTP metrics with
different url tag, but with the same name tag.
This PR improves aggregation by setting url tag to be the same as name
tag. It's not ideal, but seems like the least bad option in term of UX.
Fixes #1166