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

WIP: stop NaNs from propogating through opencost and kubecost #2069

Merged
merged 1 commit into from Aug 9, 2023

Conversation

saweber
Copy link
Contributor

@saweber saweber commented Aug 7, 2023

What does this PR change?

  • Add SanitizeNaN functions to remove and prevent NaNs from breaking calculations and API responses due to NaN/JSON marshaling

Does this PR relate to any other PRs?

  • no

How will this PR impact users?

  • stop NaNs

Does this PR address any GitHub or Zendesk issues?

  • See GTM-12

How was this PR tested?

  • unit tests

Does this PR require changes to documentation?

  • worth noting that a rebuild will sanitize out existing NaNs

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?

  • I can't

@saweber saweber marked this pull request as draft August 7, 2023 20:45
@saweber saweber force-pushed the GTM-12/stop-NaNs branch 4 times, most recently from d0fad1d to af5308c Compare August 8, 2023 13:53
SharedCostBreakdown: SharedCostBreakdowns{"NaN": getMockSharedCostBreakdown(f)},
LoadBalancers: LbAllocations{"NaN": getMockLbAllocation(f)},
}
return alloc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to convert these mock functions to use reflection (like the check function) if I have any extra time.

}

func checkAllocation(t *testing.T, tcName string, alloc Allocation) {
v := reflect.ValueOf(alloc)
Copy link
Contributor Author

@saweber saweber Aug 8, 2023

Choose a reason for hiding this comment

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

There is a lot of use of reflect in here to avoid repetitive code - but only for tests.

@saweber saweber force-pushed the GTM-12/stop-NaNs branch 3 times, most recently from c4391f9 to 6b9edb3 Compare August 8, 2023 18:48
Signed-off-by: saweber <saweber@gmail.com>
@saweber saweber marked this pull request as ready for review August 8, 2023 21:21
Copy link
Contributor

@nikovacevic nikovacevic left a comment

Choose a reason for hiding this comment

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

LGTM. Phew, this one makes your eyes bleed! Haha.

@teevans teevans merged commit 9a5eeda into opencost:develop Aug 9, 2023
3 checks passed
nikovacevic added a commit that referenced this pull request Aug 10, 2023
Cherry pick of #2069: stop NaNs from propogating through opencost and kubecost
@saweber saweber deleted the GTM-12/stop-NaNs branch September 18, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants