-
Notifications
You must be signed in to change notification settings - Fork 21
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
agent,billing: Move clients and queue into new pkg/reporting
#1078
Conversation
9bb7768
to
c0ad93e
Compare
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.
Overall looks good, couple of places I think should be simplified, and couple names should be changed.
@@ -1,21 +1,10 @@ | |||
package billing |
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.
Does it make sense to get rid of this package and move all the stuff to either agent/billing or reporting?
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'd like to do that, yeah. Wanted to leave it outside the scope of this change for now though — will keep this comment open as a reminder.
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.
⇒ #1139
This is part 2 of 2; see #1078 for the ground work. In short, this commit: * Adds a new package: 'pkg/agent/scalingevents' * Adds new callbacks to core.State to allow it to report on scaling events changes in desired CU.
This is part 2 of 2; see #1078 for the ground work. In short, this commit: * Adds a new package: 'pkg/agent/scalingevents' * Adds new callbacks to core.State to allow it to report on scaling events changes in desired CU.
f608569
to
1c71a57
Compare
This is part 2 of 2; see #1078 for the ground work. In short, this commit: * Adds a new package: 'pkg/agent/scalingevents' * Adds new callbacks to core.State to allow it to report on scaling events changes in desired CU.
@Omrigan I've responded to review comments. LMK what you think. |
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.
Mostly nits.
This commit adds a new package: pkg/reporting. The new package contains both the HTTP/S3/Azure Blob client implementations from pkg/billing, as well as the event queue and sender implementation from pkg/agent/billing. The intent is to provide a 'plug and play' infrastructure for asynchronously sending billing events (among other event types) to a variety of sources, with the code responsible for generating those events only being responsible for setting up the clients and queueing the events.
a46466d
to
df54b37
Compare
This is part 2 of 2; see #1078 for the ground work. In short, this commit: * Adds a new package: 'pkg/agent/scalingevents' * Adds new callbacks to core.State to allow it to report on scaling events changes in desired CU.
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
HTML Report |
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.
Looks mostly done!
This is part 2 of 2; see #1078 for the ground work. In short, this commit: * Adds a new package: 'pkg/agent/scalingevents' * Adds new callbacks to core.State to allow it to report on scaling events changes in desired CU.
Initial refactoring for neondatabase/cloud#15939 — opening this PR early because it's rather substantial and might be good discuss earlier on.
Details of this change
This commit adds a new package:
pkg/reporting
.The new package contains both the HTTP/S3/Azure Blob client implementations from
pkg/billing
, as well as the event queue and sender implementation frompkg/agent/billing
.The intent is to provide a 'plug and play' infrastructure for asynchronously sending billing events (among other event types) to a variety of sources, with the code responsible for generating those events only being responsible for setting up the clients and queueing the events.
Current state: All should be working as expected.
Notes for review: It's a big diff, sorry. I'm not totally sure if there's good ways to make it come out cleaner. Open for feedback on the new design — looking at
pkg/reporting
directly (particularlyclient.go
andsink.go
) is probably also the easiest way to get your bearings on the change as a whole.