-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[usage] Add BillingService with UpdateInvoices RPC #11650
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
Conversation
google.protobuf.Timestamp start_time = 1; | ||
google.protobuf.Timestamp end_time = 2; | ||
|
||
repeated BilledSession sessions = 3; |
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.
As a client of this RPC I would assume I can:
- Give a start time and an end time and have invoices updated for all instances that ran in that time (maybe I'd like to give attributionIds too, so that I only update invoices for that team/user).
but here I can pass a list of BilledSessions
as inputs (presumably intended to be those returned by the ListBilledUsage
RPC on the usage service. What happens if I pass billed sessions that lie outside the time range?
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.
So this is not a general purpose RPC, in a way, it's an internal RPC. I can clarify in comments.
You're correct, this only makes sense in the context of another thing which produces the sessions. I'm not sure how to rename or better convey this but the intended usage is not outside callers without context, but the "usage reconcile run"
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 think some comments above the service to that effect would be good 👍
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.
When we do have authN in depth, I would've also encoded that the only caller of this RPC can be a service account which has the perms to generate usage reports, which I believe would've helped.
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.
Added a comment on the proto
578c317
to
8d604b7
Compare
Description
To move the logic into an RPC, and decouple it from the Usage logic
Related Issue(s)
How to test
CI runs
Release Notes
Documentation
Werft options: