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

Creates Cloud Cost Trends API doc, updates Allocation Trends content #835

Merged
merged 12 commits into from
Dec 14, 2023

Conversation

brstuder
Copy link
Contributor

@brstuder brstuder commented Dec 5, 2023

Related issue

Proposed Changes

Dev PR at https://github.com/kubecost/kubecost-cost-model/pull/1937

@brstuder brstuder requested a review from nickcurie December 5, 2023 21:07
@brstuder brstuder requested a review from a team as a code owner December 5, 2023 21:07
@brstuder brstuder added the 1.108 label Dec 5, 2023
@brstuder
Copy link
Contributor Author

brstuder commented Dec 5, 2023

Doc won't render as intended in GH's MD preview, but if you select Show all checks, GitBook has a preview that will show you what the doc will display as

Copy link
Contributor

@nickcurie nickcurie left a comment

Choose a reason for hiding this comment

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

This looks good, just one or two quick things


## Using the `names` parameter

`names` is a mandatory parameter which determines the sequence of items returned, based on whatever the query is aggregating by. For example, when `aggregateBy=provider`, the user should provide a comma-separated list of all items they which to see trend values for in this category. In this case, they should provide `names=AWS,GCP,Azure` to receive a list of trend values for all three providers. `service` is the default value for `AggregateBy`, so if the user does not provide a value for `aggregateBy`, they must still use the names parameter to list all services requested. For example, `names=AmazonEC2,AmazonS3,Microsoft.Compute...`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The actual parameter for aggregation is aggregate not aggregateBy


The equation for calculating `value` is: `value=window/comparisonWindow - 1`

Receiving a positive `value` means your `window` has increased compared to `comparisonWindow`. A negative `value` means spending has decreased.
Copy link
Contributor

@nickcurie nickcurie Dec 5, 2023

Choose a reason for hiding this comment

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

Should this be something like "Receiving a positive value means your spending has increased in window compared to comparisonWindow ..."? As it is reads kind of strange to me

@brstuder brstuder requested a review from nickcurie December 6, 2023 20:17
{% endswagger-parameter %}

{% swagger-parameter in="path" name="aggregate" type="string" required="false" %}
Field by which to aggregate the results. Accepts: `invoiceEntityID`, `accountID`, `provider`, `service`, and `label:<name>`. Supports multi-aggregation using comma-separated lists, such as `aggregate=accountID,service`. Defaults to `service`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm realizing now that we actually support more than this and that the default is not actually service but an empty string. Using an empty string for aggregate would cause us to aggregate by item.

As for a list of acceptable aggregate parameters, we can take invoiceEntityID, accountID, provider, providerID, category, service, and label:<name>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if they don't provide a value for aggregate, they will have to list every single line item in Names for a complete breakdown of their trends?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes they would

@brstuder brstuder merged commit d4e65c3 into main Dec 14, 2023
2 checks passed
@brstuder brstuder deleted the trends branch December 14, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants