-
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
fix: csv output for non indexable metrics #2829
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2829 +/- ##
==========================================
- Coverage 76.04% 75.94% -0.11%
==========================================
Files 215 213 -2
Lines 16564 16564
==========================================
- Hits 12596 12579 -17
- Misses 3199 3210 +11
- Partials 769 775 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it 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.
Hi @leonyork,
thanks for your contribution 🙇 The implemented code looks good to me.
I think we could unblock this PR if we extend the following unit test
Line 212 in 613fe8b
func TestRun(t *testing.T) { |
providing the
vu
tag in one of the sample and enabling the VU system tag here:Line 353 in 613fe8b
SystemTags: metrics.NewSystemTagSet(metrics.TagError | metrics.TagCheck), |
An integration test is still nice to have but we could address it in another PR.
@codebien, thanks for the pointer to the unit test! I've updated that and removed the new line you suggested 🙂 |
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 for your work debugging and fixing this @leonyork! 👏 The excellent steps to reproduce it, and that separate repo were of great help 👍
This looks great to me. It's an elegant and simple fix in an inelegant part of the codebase 😅
Like @codebien says, we can add the integration test if needed later, once we merge all related PRs, or decide to just leave it with the unit test, since it does cover the change.
Don't worry about the CI failure, it's a known flaky test 😞
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, thanks @leonyork 🙇
Description
Fix for #2827 without refactoring the CSV output or adding integration tests (which are waiting on #2821).
vu
anditer
columns from the CSV output if they are included as system tags.vu
anditer
tags in themetadata
column of the output if they are included as system tags.Manual Testing
I've not added any integration tests as the integration tests are being refactored in #2821
Instead I get the follow results:
from the script: