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

Bug/rds calculation is missing additional costs #143

Merged
merged 8 commits into from
Sep 6, 2020

Conversation

cregev
Copy link
Contributor

@cregev cregev commented Aug 21, 2020

What type of PR is this?
/Bug

What this PR does / why we need it:
Fix a bug in RDS calculation

Fixes: #68

@cregev cregev added the kind/bug Something isn't working label Aug 21, 2020
@cregev cregev changed the title WIP: Bug/rds calculation is missing additional costs Bug/rds calculation is missing additional costs Aug 22, 2020
@cregev cregev requested a review from kaplanelad August 22, 2020 11:58
@cregev cregev added the area/collector Improvements or additions to collector label Aug 22, 2020
@cregev cregev self-assigned this Aug 22, 2020
"region": r.awsManager.GetRegion(),
}).Info("RDS instance detected as unutilized resource")

instancePricingFilters := r.getPricingInstanceFilterInput(instance)
Copy link
Contributor

Choose a reason for hiding this comment

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

The price calculation really needs to be in the metrics loop?
the price can be changed when we have a different metric?

}

var hourlyStoragePrice float64
if rdsStorageType, found := rdsStorageType[*instance.StorageType]; found {
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you say to move this logic into a function?
it will help us for better tests coverage + make Detect function to be more readable

},
},
}
}
func (r *RDSManager) getPricingAuroraStorageFilterInput(rdsStorageType string, pricingRegionPrefix string) pricing.GetProductsInput {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a description on this function?

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 not seeing testing on this function.
can you please add one?

return databaseEngine
}

func (r *RDSManager) getPricingDeploymentOption(instance *rds.DBInstance) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a description on this function?

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 not seeing testing on this function.
can you please add one?

log.WithField("storage_filters", storagePricingFilters).Debug("pricing storage filters")
storagePrice, err := r.awsManager.GetPricingClient().GetPrice(storagePricingFilters, "", r.awsManager.GetRegion())
if err != nil {
log.WithError(err).Error("Could not get rds storage price")
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you say to add the filter object into the log to improve the troubleshoot

collector/aws/resources/rds.go Show resolved Hide resolved
@kaplanelad kaplanelad added the lgtm label Sep 6, 2020
@cregev cregev merged commit e8b2f75 into master Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collector Improvements or additions to collector kind/bug Something isn't working lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RDS Calculation is missing additional costs.
2 participants