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

Correct GCP's default discount percentage #2514

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

namm2
Copy link

@namm2 namm2 commented Feb 7, 2024

What does this PR change?

  • Correct the GCP's default discount value to 0%, this should be the correct value when this field is not set, also all other clouds' configs are 0%.

Does this PR relate to any other PRs?

How will this PR impact users?

Does this PR address any GitHub or Zendesk issues?

  • Closes ...

How was this PR tested?

Does this PR require changes to documentation?

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?

Copy link

vercel bot commented Feb 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencost ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 8, 2024 11:24am

@mattray mattray added opencost OpenCost issues vs. external/downstream P1 Estimated Priority (P0 is highest, P4 is lowest) kubecost Relevant to Kubecost's downstream project E1 Estimated level of Effort (1 is easiest, 4 is hardest) labels Feb 8, 2024
@mattray
Copy link
Collaborator

mattray commented Feb 8, 2024

@namm2 Good catch! I've confirmed the other providers have this set to zero. @opencost/opencost-maintainers any reason this may have been set differently?

@nikovacevic
Copy link
Contributor

Historically, we've set this to 30% intentionally to match GCP's sustained use discount. Maybe product wants to chime in with regard to the best decision here. Do we assume sustained use or not? Or, even if we do assume sustained use, should we put the onus on the user to set this discount themselves?

@mattray
Copy link
Collaborator

mattray commented Feb 9, 2024

If we don't set it to 0%, could we add the rationale as a comment?

@namm2
Copy link
Author

namm2 commented Feb 9, 2024

AFAIU, SUD discount is variable based on the %usage and machine type (hence up to 30%) then the static value 30% is still not correct out-of-the-box. Also there's another Commited Use Discount which is variable too, I'd leave it to users to set their discount value than hard code it.

@mbolt35
Copy link
Contributor

mbolt35 commented Feb 9, 2024

@AjayTripathy this has been part of the product as long as I've been around. Would love your take here.

Copy link

This pull request has been marked as stale because it has been open for 90 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label May 10, 2024
@mattray
Copy link
Collaborator

mattray commented May 10, 2024

@AjayTripathy you want to comment?

@mattray mattray removed the Stale label May 10, 2024
@AjayTripathy
Copy link
Contributor

We just take an average 30% discount because this is meant to be an estimate. This is user configurable so I'm not sure there's a compelling reason to change this right now.

@namm2
Copy link
Author

namm2 commented May 16, 2024

We just take an average 30% discount because this is meant to be an estimate. This is user configurable so I'm not sure there's a compelling reason to change this right now.

Yes it's configurable but I mean the default value should be 0% like other cloud providers, and it's up to end users to set this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E1 Estimated level of Effort (1 is easiest, 4 is hardest) kubecost Relevant to Kubecost's downstream project needs-follow-up opencost OpenCost issues vs. external/downstream P1 Estimated Priority (P0 is highest, P4 is lowest)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants