kubecost / kubectl-cost

CLI for determining the cost of Kubernetes workloads
Apache License 2.0
872 stars 55 forks source link

Add support for cost difference in "predict" using new API #141

Closed michaelmdresser closed 1 year ago

michaelmdresser commented 1 year ago

What does this PR change?

See release note. Diff behavior matches the current diff used by our new Admission Controller.

How does this PR impact users? (This is the kind of thing that goes in release notes!)

How was this PR tested?

→ go run cmd/kubectl-cost/kubectl-cost.go predict -f ./test/multi.yaml --window '2h'
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+
| WORKLOAD                                   | CPU  | MEM   | GPU | CPU/MO     | MEM/MO     | GPU/MO      | Δ CPU/MO   | Δ MEM/MO   | TOTAL/MO    |
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+
| default/Deployment/nginx-deployment        | 9    | 6Gi   | 0   | 207.72 USD |  18.54 USD |    0.00 USD | 207.72 USD |  18.54 USD |  226.26 USD |
| kubecost/Deployment/kubecost-cost-analyzer | 15m  | 9Mi   | 0   |   0.35 USD |   0.03 USD |    0.00 USD |  -4.40 USD |  -0.37 USD |    0.37 USD |
| default/Deployment/nginx-deployment-2      | 10   | 35Gi  | 0   | 230.80 USD | 108.15 USD |    0.00 USD | 230.80 USD | 108.15 USD |  338.95 USD |
| default/Pod/nginx-pod                      | 350m | 200Mi | 0   |   8.08 USD |   0.60 USD |    0.00 USD |   8.08 USD |   0.60 USD |    8.68 USD |
| default/Pod/nginx-pod                      | 350m | 200Mi | 3   |   8.08 USD |   0.60 USD | 2080.50 USD |   8.08 USD |   0.60 USD | 2089.18 USD |
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+
|                                            |      |       |     | 455.02 USD | 127.92 USD | 2080.50 USD | 450.28 USD | 127.53 USD | 2663.45 USD |
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+

Have you made an update to documentation?

Yes, README update in this PR.

AjayTripathy commented 1 year ago

Super cool! cc @kwombach12 for UX feedback but lgtm!

kwombach12 commented 1 year ago

@michaelmdresser very cool. The data is readable and well-presented.

dwbrown2 commented 1 year ago

I suggest we explicitly state that it's the diff if we're showing this by default... i.e. we could have one sentence before the table showing something like:

"Expected monthly cost change is as follows:"

vs

"Expected monthly total cost is as follows:"

Thoughts?

michaelmdresser commented 1 year ago

The table shows both total cost and cost change on two resources: cpu and memory. If you think the delta symbol isn't universal enough to convey "difference" we can think up something to address it. I mainly want to avoid cluttering up the terminal output, as IMO its better for tools like this to be straight to the point with useful --help text if users are confused. Happy to update that, too if it isn't clear.

dwbrown2 commented 1 year ago

I see your point. I think some users may either miss or not understand the user of delta. One reason for the latter might be that it's not applied to "Total/Mo" Column. Should that be changed to Diff/Mo?

michaelmdresser commented 1 year ago

The current predict API only diffs CPU and memory but kubectl cost predict also shows GPU cost. This means we can't put a diff on total cost, only on CPU and memory individually right now. So, the total/mo column right now is not a diff; even now I can recognize how that might be confusing.

I'm hoping it will be straightforward to address this UX shortcoming once I finish the better predict API, incorporating what we've learned.