cleanup, improvements

Signed-off-by: Matt Bolt <mbolt35@gmail.com>

modules/prometheus-source/pkg/prom/datasource.go

@@ -16,7 +16,6 @@ import (
 	"github.com/opencost/opencost/core/pkg/source"
 	"github.com/opencost/opencost/core/pkg/util/httputil"
 	"github.com/opencost/opencost/core/pkg/util/json"
-	"github.com/opencost/opencost/core/pkg/util/timeutil"
 
 	prometheus "github.com/prometheus/client_golang/api"
 	prometheusAPI "github.com/prometheus/client_golang/api/prometheus/v1"
@@ -65,61 +64,6 @@ func toStartEndStep(qp httputil.QueryParams) (start, end time.Time, step time.Du
 	return
 }
 
-// FIXME: Before merge, implement a more robust design. This is brittle and bug-prone,
-// FIXME: but decouples the prom requirements from the Provider implementations.
-var providerStorageQueries = map[string]func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string{
-	"aws": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"gcp": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		// TODO Set to the price for the appropriate storage class. It's not trivial to determine the local storage disk type
-		// See https://cloud.google.com/compute/disks-image-pricing#persistentdisk
-		localStorageCost := 0.04
-
-		baseMetric := "container_fs_limit_bytes"
-		if used {
-			baseMetric = "container_fs_usage_bytes"
-		}
-
-		fmtCumulativeQuery := `sum(
-			sum_over_time(%s{device!="tmpfs", id="/", %s}[%s:1m])
-		) by (%s) / 60 / 730 / 1024 / 1024 / 1024 * %f`
-
-		fmtMonthlyQuery := `sum(
-			avg_over_time(%s{device!="tmpfs", id="/", %s}[%s:1m])
-		) by (%s) / 1024 / 1024 / 1024 * %f`
-
-		fmtQuery := fmtCumulativeQuery
-		if rate {
-			fmtQuery = fmtMonthlyQuery
-		}
-		fmtWindow := timeutil.DurationString(end.Sub(start))
-
-		return fmt.Sprintf(fmtQuery, baseMetric, config.ClusterFilter, fmtWindow, config.ClusterLabel, localStorageCost)
-	},
-	"azure": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"alibaba": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"scaleway": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"otc": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"oracle": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"csv": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-	"custom": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
-		return ""
-	},
-}
-
 // creates a new help error which indicates the caller can retry and is non-fatal.
 func newHelpRetryError(format string, args ...any) error {
 	formatWithHelp := format + "\nTroubleshooting help available at: %s"

modules/prometheus-source/pkg/prom/metricsquerier.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/opencost/opencost/core/pkg/log"
 	"github.com/opencost/opencost/core/pkg/source"
 	"github.com/opencost/opencost/core/pkg/util/timeutil"
 	prometheus "github.com/prometheus/client_golang/api"
@@ -133,20 +132,13 @@ func (pds *PrometheusMetricsQuerier) QueryLocalStorageCost(start, end time.Time)
 	const localStorageCostQuery = `sum_over_time(sum(container_fs_limit_bytes{device=~"/dev/(nvme|sda).*", id="/", %s}) by (instance, device, %s)[%s:%dm]) / 1024 / 1024 / 1024 * %f * %f`
 
 	cfg := pds.promConfig
-	resolution := cfg.DataResolution
+	minsPerResolution := cfg.DataResolutionMinutes
 
 	durStr := timeutil.DurationString(end.Sub(start))
 	if durStr == "" {
 		panic("failed to parse duration string passed to QueryLocalStorageCost")
 	}
 
-	//Ensuring if data resolution is less than 60s default it to 1m
-	var minsPerResolution int
-	if minsPerResolution = int(resolution.Minutes()); int(resolution.Minutes()) == 0 {
-		minsPerResolution = 1
-		log.DedupedWarningf(3, "QueryLocalStorageCost: Configured resolution (%d seconds) is below the 60 seconds threshold. Overriding with 1 minute.", int(resolution.Seconds()))
-	}
-
 	// hourlyToCumulative is a scaling factor that, when multiplied by an
 	// hourly value, converts it to a cumulative value; i.e. [$/hr] *
 	// [min/res]*[hr/min] = [$/res]

modules/prometheus-source/pkg/prom/providerstoragequeries.go

@@ -0,0 +1,66 @@
+package prom
+
+import (
+	"fmt"
+	"time"
+
+	"github.com/opencost/opencost/core/pkg/util/timeutil"
+)
+
+// NOTE (bolt): This is currently not being used directly in the prometheus data source, but may be useful in the future
+// NOTE (bolt): when it comes to pricing local storage options per provider. Recommendation is to abstract this into some
+// NOTE (bolt): type of storage queury registry.
+var providerStorageQueries = map[string]func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string{
+	"aws": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"gcp": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		// TODO Set to the price for the appropriate storage class. It's not trivial to determine the local storage disk type
+		// See https://cloud.google.com/compute/disks-image-pricing#persistentdisk
+		localStorageCost := 0.04
+
+		baseMetric := "container_fs_limit_bytes"
+		if used {
+			baseMetric = "container_fs_usage_bytes"
+		}
+
+		fmtCumulativeQuery := `sum(
+			sum_over_time(%s{device!="tmpfs", id="/", %s}[%s:1m])
+		) by (%s) / 60 / 730 / 1024 / 1024 / 1024 * %f`
+
+		fmtMonthlyQuery := `sum(
+			avg_over_time(%s{device!="tmpfs", id="/", %s}[%s:1m])
+		) by (%s) / 1024 / 1024 / 1024 * %f`
+
+		fmtQuery := fmtCumulativeQuery
+		if rate {
+			fmtQuery = fmtMonthlyQuery
+		}
+		fmtWindow := timeutil.DurationString(end.Sub(start))
+
+		return fmt.Sprintf(fmtQuery, baseMetric, config.ClusterFilter, fmtWindow, config.ClusterLabel, localStorageCost)
+	},
+	"azure": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"alibaba": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"scaleway": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"otc": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"oracle": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"csv": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+	"custom": func(config *OpenCostPrometheusConfig, start, end time.Time, rate bool, used bool) string {
+		return ""
+	},
+}
+
+var _ = providerStorageQueries

pkg/costmodel/cluster.go

@@ -811,7 +811,10 @@ func pvCosts(
 			continue
 		}
 
-		key := DiskIdentifier{cluster, name}
+		key := DiskIdentifier{
+			Cluster: cluster,
+			Name:    name,
+		}
 		if _, ok := diskMap[key]; !ok {
 			diskMap[key] = &Disk{
 				Cluster:   cluster,
@@ -956,7 +959,10 @@ func pvCosts(
 
 		usage := result.Data[0].Value
 
-		key := DiskIdentifier{cluster, volumeName}
+		key := DiskIdentifier{
+			Cluster: cluster,
+			Name:    volumeName,
+		}
 
 		if _, ok := diskMap[key]; !ok {
 			diskMap[key] = &Disk{

pkg/costmodel/cluster_helpers.go

@@ -680,7 +680,10 @@ func buildAssetsPVCMap(resPVCInfo []*source.PVCInfoResult) map[DiskIdentifier]*D
 			continue
 		}
 
-		key := DiskIdentifier{cluster, volumeName}
+		key := DiskIdentifier{
+			Cluster: cluster,
+			Name:    volumeName,
+		}
 		if _, ok := diskMap[key]; !ok {
 			diskMap[key] = &Disk{
 				Cluster:   cluster,