-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat(mgmt): add credit consumption chart endpoint #359
feat(mgmt): add credit consumption chart endpoint #359
Conversation
triggered by commit: 498c634
core/mgmt/v1beta/metric.proto
Outdated
// by owner and time frame. | ||
message CreditConsumptionChartRecord { | ||
// Credit owner UUID. | ||
string credit_owner_uid = 1; |
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.
Hi @EiffelFly
Do you think we should use id
or uid
here? Since most of our APIs use id
in the path, I'm not sure if using uid
will make it harder for the Console to integrate.
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 actually added this field in the record to have some sort of ownership as part of its data, but in fact the frontend shouldn't need to use it. I agree that in public endpoints the owner ID is more readable and straightforward, but in the request frontend can use owner_name
.
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.
@donch1989 @jvallesm I would suggest we use id
like users/summerbud
since that is the essence of resource oriented API design and console is quite happy with that since all the endpoint have unified behavior
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.
Got it.
I have a related question about this. If we don't include owner_name
in the filter, what will it return?
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 would suggest we use id like users/summerbud
🤝 I guess exposing UIDs unnecessarily isn't great, I'll adjust the contract.
I have a related question about this. If we don't include owner_name in the filter, what will it return?
It'd aggregate credit over all owners. We don't have a use case for that and it is a resource-consuming operation so I'd make that filter mandatory. But, then, I guess it makes more sense as an explicit endpoint param. In the pipeline chart this is part of the filter but I think the same strategy applies.
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 left some comment 👍
core/mgmt/v1beta/metric.proto
Outdated
// by owner and time frame. | ||
message CreditConsumptionChartRecord { | ||
// Credit owner UUID. | ||
string credit_owner_uid = 1; |
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.
@donch1989 @jvallesm I would suggest we use id
like users/summerbud
since that is the essence of resource oriented API design and console is quite happy with that since all the endpoint have unified behavior
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.
LGTM
f6c92d4
to
30b218d
Compare
triggered by commit: 0307786
// expression to determine the time range for the charts. | ||
// - Example: `start='2024-06-04T10:000:00Z' AND stop='2024-06-11T10:00:00Z` | ||
optional string filter = 3 [(google.api.field_behavior) = OPTIONAL]; | ||
// The minimum (and default) window is 1h. |
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.
@donch1989 @EiffelFly one last change. The transpiler in mgmt-backend
produced only an empty expression, even if I copied the filter from the existing dashboard endpoints. In order to make it and, given that all the designs we're handling right now only filter by start and end date, I removed the filter
param
Sample request (window is reduced to showcase different time slots): $ curl 'https://localhost:8084/v1beta/metrics/credit/charts?owner=users/juan&start=2024-06-13T06:00:00Z&stop=2024-06-13T08:00:00Z&aggregation_window=10m' \
-H 'Instill-User-Uid: xxx-055d163d2425' -H 'Instill-Auth-Type: user' \
-H 'Content-Type: application/json' | jq
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 174 100 174 0 0 4221 0 --:--:-- --:--:-- --:--:-- 4243
{
"credit_consumption_chart_records": [
{
"credit_owner": "users/juan",
"time_buckets": [
"2024-06-13T07:50:00Z",
"2024-06-13T08:00:00Z"
],
"amount": [
24.7,
40.75
]
}
],
"total_amount": 65.45
} |
Because
This commit
count is served by a different endpoint. For now, we only want to sum amounts
by time slot so a single endpoint fits our use case.
Info
Rough example of the returned data (without the total sum):