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

feat(mgmt): add credit consumption chart endpoint #359

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

jvallesm
Copy link
Collaborator

@jvallesm jvallesm commented Jun 11, 2024

Because

  • Instill Cloud needs to display the credit consumption of an owner as a chart.

This commit

  • Adds the contract for the chart information. In pipeline metrics the total
    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.

CleanShot 2024-06-11 at 10 09 45

Info

Rough example of the returned data (without the total sum):

Result: _result
Table: keys: [_time]
                    _time:time                  amount:float
------------------------------  ----------------------------
2024-06-10T18:15:00.000000000Z                         20.85
Table: keys: [_time]
                    _time:time                  amount:float
------------------------------  ----------------------------
2024-06-10T18:20:00.000000000Z                         20.45
Table: keys: [_time]
                    _time:time                  amount:float
------------------------------  ----------------------------
2024-06-10T18:25:00.000000000Z                          58.5
Table: keys: [_time]
                    _time:time                  amount:float
------------------------------  ----------------------------
2024-06-10T18:30:00.000000000Z                          45.6
Table: keys: [_time]
                    _time:time                  amount:float
------------------------------  ----------------------------
2024-06-10T18:40:00.000000000Z                         15.75

@jvallesm jvallesm self-assigned this Jun 11, 2024
Copy link

linear bot commented Jun 11, 2024

@jvallesm jvallesm marked this pull request as ready for review June 11, 2024 11:01
@jvallesm jvallesm requested a review from EiffelFly June 11, 2024 11:01
// by owner and time frame.
message CreditConsumptionChartRecord {
// Credit owner UUID.
string credit_owner_uid = 1;
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

@EiffelFly EiffelFly left a 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 👍

// by owner and time frame.
message CreditConsumptionChartRecord {
// Credit owner UUID.
string credit_owner_uid = 1;
Copy link
Member

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

core/mgmt/v1beta/metric.proto Outdated Show resolved Hide resolved
donch1989
donch1989 previously approved these changes Jun 12, 2024
Copy link
Member

@donch1989 donch1989 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jvallesm jvallesm force-pushed the jvalles/ins-4207-collect-credit-usage-metrics branch from f6c92d4 to 30b218d Compare June 12, 2024 17:35
@jvallesm jvallesm requested a review from EiffelFly June 13, 2024 07:55
// 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.
Copy link
Collaborator Author

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

@jvallesm
Copy link
Collaborator Author

jvallesm commented Jun 13, 2024

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
}

@jvallesm jvallesm merged commit 6b98dfb into main Jun 13, 2024
3 checks passed
@jvallesm jvallesm deleted the jvalles/ins-4207-collect-credit-usage-metrics branch June 13, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👋 Done
4 participants