Add new ListRepositoriesUsage rpc and messages #2951
Conversation
| } | ||
|
|
||
| message GetAllOrganizationsUsagesResponse { | ||
| repeated RepositoryUsage repository_usages = 1; |
There was a problem hiding this comment.
What is repeated here? What is unique and what is not? All needs to be documented
There was a problem hiding this comment.
// The list of types usage for all repositories in all orgs on a single BSR instance.
There was a problem hiding this comment.
This doesn't answer the original question - is this monthly? What does the list contain? What should I expect a valid list to include based on the timestamp?
There was a problem hiding this comment.
// The list contains types usage info and counts for all repositories in all orgs on a single BSR instance, based on a snapshot single point in time within the requested time range where the aggregate types usage is at a maximum.
does this make more sense?
is this monthly?
no, it's for any point within the requested date range
What does the list contain?
repository information and types usage for all repositories on the instance
What should I expect a valid list to include based on the timestamp?
the types count for the point when types usage was at a maximum in the time range
There was a problem hiding this comment.
Between the documentation of this method, and the documentation/name for RepositoryUsage, some things are very unclear
- How many items will be in this list? Do we need to have paging?
- What is the timeframe associated with each item? Is this configurable? Should it be?
- How do I map items in this list to a particular timeframe?
Other thought:
- If this method is really specialized toward returning max usage within some time horizon, maybe the names should more explicitly represent that.
| } | ||
|
|
||
| message GetAllOrganizationsUsagesResponse { | ||
| repeated RepositoryUsage repository_usages = 1; |
There was a problem hiding this comment.
// The list of types usage for all repositories in all orgs on a single BSR instance.
gilwong00
changed the title
There was a problem hiding this comment.
Generally we want our internal APIs to be as clearly named, designed, and documented as our external APIs. Please read through https://github.com/bufbuild/registry-proto to get a sense of what a good standard looks like moving forward, and to understand the patterns we have established (e.g. like how we do pagination). It would also be good to add validation rules to all fields as appropriate (lmk if there is a reason we haven't been doing that already).
| @@ -142,6 +161,9 @@ service AdminService { | |||
| // GetClusterUsage returns the summation of total message, enum and service types usage | |||
| // for every repository in each organization within a single tenant BSR instance. | |||
| rpc GetClusterUsage(GetClusterUsageRequest) returns (GetClusterUsageResponse); | |||
| // ListRepositoriesUsage returns the usage metrics for all repositories within | |||
| // a single tenant BSR instance highest point in time usage for a given time range. | |||
There was a problem hiding this comment.
this sentence doesn't make sense to me, like there are some missing words or something
| // Usage filter is an optional owner or repository id used to filter usage. | ||
| oneof usage_filter { | ||
| // The id of the owner. | ||
| string owner_id = 3; | ||
| // The id of the repository. | ||
| string repository_id = 4; | ||
| } |
There was a problem hiding this comment.
Can you explain why this is a oneof?
| } | ||
|
|
||
| message GetAllOrganizationsUsagesResponse { | ||
| repeated RepositoryUsage repository_usages = 1; |
There was a problem hiding this comment.
Between the documentation of this method, and the documentation/name for RepositoryUsage, some things are very unclear
- How many items will be in this list? Do we need to have paging?
- What is the timeframe associated with each item? Is this configurable? Should it be?
- How do I map items in this list to a particular timeframe?
Other thought:
- If this method is really specialized toward returning max usage within some time horizon, maybe the names should more explicitly represent that.
Part 1 to: GRW-1804