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.

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

cloud/v1: Aggregation in a dedicated pkg #3063

Merged
merged 6 commits into from May 16, 2023

Conversation

codebien
Copy link
Collaborator

@codebien codebien commented May 10, 2023

It splits the current output in a subpackage v1, it moves there the specific parts for version 1 and that are not expected to be used from version 2.
It uses part of the work done in #3041, and it is a foundation for adding, on top of it, the versioning config option (#3041) and the expv2 package (#2963).

The proposed architecture should resolve the requested change in #2963 (comment).

I think you should review the code going through the commits one by one to check the evolution, otherwise, the final code could be too chaotic.

I think the test coverage is not high enough for the new output/cloud/output.go to have good confidence. If the architecture is confirmed I will add a bunch of them in a dedicated PR (we can keep this open till it is approved).

@codebien codebien added this to the v0.45.0 milestone May 10, 2023
@codebien codebien self-assigned this May 10, 2023
@codebien codebien requested review from na-- and removed request for oleiade May 10, 2023 20:35
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #3063 (f90d741) into master (c00bfbb) will decrease coverage by 0.04%.
The diff coverage is 71.34%.

❗ Current head f90d741 differs from pull request most recent head 10ecd76. Consider uploading reports for the commit 10ecd76 to get more accurate results

@@            Coverage Diff             @@
##           master    #3063      +/-   ##
==========================================
- Coverage   76.72%   76.69%   -0.04%     
==========================================
  Files         229      230       +1     
  Lines       17110    17173      +63     
==========================================
+ Hits        13127    13170      +43     
- Misses       3140     3153      +13     
- Partials      843      850       +7     
Flag Coverage Δ
ubuntu 76.69% <71.34%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
output/cloud/v1/cloud_easyjson.go 35.55% <ø> (ø)
output/cloud/v1/data.go 91.24% <ø> (ø)
output/cloud/v1/metrics_client.go 80.00% <ø> (ø)
cmd/cloud.go 70.40% <57.14%> (-1.18%) ⬇️
output/cloud/v1/output.go 67.81% <67.81%> (ø)
cloudapi/api.go 54.45% <71.42%> (+2.37%) ⬆️
output/cloud/output.go 66.90% <72.50%> (-2.71%) ⬇️
js/bundle.go 85.23% <100.00%> (+0.66%) ⬆️
lib/testutils/logrus_hook.go 100.00% <100.00%> (ø)
metrics/registry.go 95.34% <100.00%> (+3.85%) ⬆️
... and 1 more

... and 5 files with indirect coverage changes

@codebien codebien marked this pull request as ready for review May 10, 2023 20:49
Base automatically changed from cloud/refactor/tests to master May 11, 2023 07:02
@codebien codebien force-pushed the cloud/refactor/split-aggregation branch from 7752baa to e127a23 Compare May 11, 2023 07:56
@codebien
Copy link
Collaborator Author

Rebased to resolve the conflicts, green now

na--
na-- previously approved these changes May 12, 2023
olegbespalov
olegbespalov previously approved these changes May 12, 2023
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 few minor non-blocking comments 👍

output/cloud/output.go Outdated Show resolved Hide resolved
output/cloud/output.go Outdated Show resolved Hide resolved
Co-authored-by: Oleg Bespalov <oleg.bespalov@grafana.com>
@codebien codebien dismissed stale reviews from olegbespalov and na-- via 10ecd76 May 15, 2023 18:39
@codebien codebien merged commit 311d94f into master May 16, 2023
21 checks passed
@codebien codebien deleted the cloud/refactor/split-aggregation branch May 16, 2023 08:42
@codebien codebien mentioned this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants