[usage] show sum of usage not balance#13222
Conversation
|
/werft run 👍 started the job as gitpod-build-sefftinge-usage-show-the-sum-of-13067.1 |
easyCZ
left a comment
easyCZ
left a comment
There was a problem hiding this comment.
LGTM - non-backwards compatible proto change
| if creditCents.ToCredits() <= 0 { | ||
| // account has no debt, do nothing | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
While we're here, we could also change the logic to do creditCents <= 0 because the Credits conversion only multiplies by a 100, which won't change the polarity
| // the amount of credits the given account (attributionId) had at the end of the requested period | ||
| double credit_balance_at_end = 4; | ||
| // the amount of credits the given account (attributionId) has used during the requested period | ||
| double credits_used = 3; |
There was a problem hiding this comment.
This is not backwards compatible. The rollout order between server and usage is undefined, and therefore you're not guaranteed it won't try to pass this value.
Please mark credit_balance_at_start as either reserved or [deprecated=true] and add a new field with ID 5.
There was a problem hiding this comment.
I'm aware this is not compatible.
I expect the friction this would cause minimal (very short period between the two component deploys, where errors would be thrown for a feature that is not used by users, yet) compared to the extra turnaround.
There was a problem hiding this comment.
Do you expect any serious issues here or is this more a matter of principles?
There was a problem hiding this comment.
Requests could actually fail to serialize on the usage component and return Unknown error. This is because each field is looked up by their index field. Here, it's kind of lucky that the field is the same type, but the removal of a field (4) would cause issues for the serializer as it expects there to be a field with index 4 (even if it's empty).
Even if we're happy with a temporary issue, we as a team (and organization) need to do better here in order to improve our reliability. It's a matter of getting into the habit of thinking (and dealing) with these else we're gonna struggle to uphold our standards as the systems grow.
There was a problem hiding this comment.
Ok, I agree with the principle behind it. Not sure going through unnecessary ceremony is helpful. I think acknowledging this explicitly, which is what we are doing here, is great. IMHO it is actually even better than blindly following a certain protocol, because we are talking about the real impact. FWIW just making it technically compatible wouldn't help users, because on the client side there would be a different result unless I roll out a full compatibility mode, which I would be happy to do if it was worth it.
easyCZ
left a comment
easyCZ
left a comment
There was a problem hiding this comment.
Approving, as you are comfortable with the incompatibility introduced.
roboquat
added
deployed: webapp
3 participants
Description
On the usage view we now only show usage and the sum is the sum of all usage in the selected period.
Related Issue(s)
fixes #13067
How to test
Release Notes
Documentation
Werft options:
If enabled this will build
install/previewValid options are
all,workspace,webapp,ide