Skip to content
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.

Sign up for GitHub

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

Jump to bottom

output/cloud: Set v2 as the default #3400

Merged
merged 2 commits into from Oct 17, 2023
Merged

output/cloud: Set v2 as the default #3400

merged 2 commits into from Oct 17, 2023
+30 −14

Conversation

codebien
Copy link
Collaborator

@codebien codebien commented Oct 16, 2023 •
edited

What?

It sets the default version for the Cloud output as v2.

Related PR(s)/Issue(s)

Closes #3258

@codebien codebien added this to the v0.48.0 milestone Oct 16, 2023
@codebien codebien self-assigned this Oct 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2023 •
edited

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (479099f) 73.13% compared to head (6c0dadf) 73.34%.
Report is 2 commits behind head on master.

❗ Current head 6c0dadf differs from pull request most recent head 2339ffb. Consider uploading reports for the commit 2339ffb to get more accurate results

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
Flag Coverage Δ
ubuntu 73.28% <30.76%> (+0.21%) ⬆️
windows 73.19% <30.76%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cloudapi/config.go 83.33% <30.76%> (-7.58%) ⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codebien codebien requested review from a team, mstoykov and olegbespalov and removed request for a team October 17, 2023 07:53
@codebien codebien marked this pull request as ready for review October 17, 2023 07:53
mstoykov
mstoykov reviewed Oct 17, 2023
View reviewed changes
output/cloud/output_test.go
@@ -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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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")
}

mstoykov
mstoykov previously approved these changes Oct 17, 2023
View reviewed changes
olegbespalov
olegbespalov previously approved these changes Oct 17, 2023
View reviewed changes
Copy link
Collaborator

@olegbespalov olegbespalov left a 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.

cloudapi/config.go Outdated Show resolved Hide resolved
@codebien codebien dismissed stale reviews from olegbespalov and mstoykov via 2339ffb October 17, 2023 10:55
@codebien codebien merged commit 036f8a0 into master Oct 17, 2023
21 checks passed
@codebien codebien deleted the default-to-cloudv2 branch October 17, 2023 11:46
@Porkepix Porkepix mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstoykov mstoykov mstoykov approved these changes

@olegbespalov olegbespalov olegbespalov approved these changes

Assignees

@codebien codebien

Labels
None yet
Projects
None yet
Milestone
v0.48.0
Development

Successfully merging this pull request may close these issues.

cloud: Set v2 as the default output
4 participants
@codebien @codecov-commenter @mstoykov @olegbespalov