-
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
output/cloud: Set v2 as the default #3400
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3400 +/- ##
==========================================
+ Coverage 73.13% 73.34% +0.20%
==========================================
Files 258 258
Lines 19641 19645 +4
==========================================
+ Hits 14364 14408 +44
+ Misses 4395 4352 -43
- Partials 882 885 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@@ -126,7 +126,7 @@ func TestOutputCreateTestWithConfigOverwrite(t *testing.T) { | |||
require.NoError(t, out.Start()) | |||
|
|||
assert.Equal(t, types.NullDurationFrom(10*time.Millisecond), out.config.MetricPushInterval) | |||
assert.Equal(t, types.NullDurationFrom(30*time.Millisecond), out.config.AggregationPeriod) | |||
assert.Equal(t, types.NullDurationFrom(1*time.Second), out.config.AggregationPeriod) |
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.
Is there a reason this is changed?
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.
There is, the aggregation in v2 can't be sub-second precision
k6/output/cloud/expv2/collect.go
Lines 69 to 71 in 44e1e63
if aggrPeriod != aggrPeriod.Truncate(time.Second) { | |
return nil, errors.New("aggregation period is not allowed to have sub-second precision") | |
} |
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, with a minor non-blocking comment.
What?
It sets the default version for the Cloud output as
v2
.Related PR(s)/Issue(s)
Closes #3258