Conversation
|
Just wanted to drop a quick note about network costing. When we calculate network costs, we do not use the NetworkTransferBytes or NetworkReceiveBytes -- It's not a bad test to run for comparison, but as it relates to cost, we do not use these statistics for costs because they include all sub-categories of traffic (ie: local traffic, which is free). I'm not sure if the network-costs pods are installed on this cluster, so we should definitely ensure that gets done as soon as possible. |
|
The actual costs and usage data we use is here: https://github.com/opencost/opencost/blob/ef740759c88732487bffa644509480b070ca77c5/modules/prometheus-source/pkg/prom/metricsquerier.go#L907-L1021 But note that this requires a |
| networkCostsPod.AllocNetworkInternetGiB = allocationResponseItem.NetworkInternetCost | ||
| } | ||
|
|
||
| noNegligibleCosts := false |
|
@ameijer , if network zone and region tests look good, I'll go ahead anc comment those out in test.bats |
|
@Manas23601 looks like the tests for zone and region are failing? for those, it is ok if we don't see any valid values |
| // AllocNetworkInternetGiB: 0.0, | ||
| // } | ||
| // continue | ||
| // } |
There was a problem hiding this comment.
I wanted the option to drill down to a pod level if needed. do you think we need to go down further down?
Signed-off-by: Manas Sivakumar <manas23601@gmail.com>
|
These are meant to fail because there is no valid zone and region costs. We should either not fail the tests if there are no valid costs or not run this test at all |
Signed-off-by: Manas Sivakumar <manas23601@gmail.com>
…ion-tests into networkcost
|
bugbot run |
|
Skipping Bugbot: Bugbot is disabled for this repository |
Signed-off-by: Manas Sivakumar <manas23601@gmail.com>
|
bugbot run |
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| networkCostsNamespace, ok := networkCostsNamespaceMap[namespace] | ||
| if !ok { | ||
| networkCostsNamespaceMap[namespace] = &NetworkCostsAggregate{ | ||
| PromNetworkReceiveBytes: networkTransferBytesPod, |
There was a problem hiding this comment.
Bug: Wrong field assigned in network transfer initialization
When a namespace doesn't exist in the map during Network Transfer Bytes processing, PromNetworkReceiveBytes is incorrectly set to networkTransferBytesPod instead of 0.0. This causes the receive bytes value to be initialized with transfer bytes data, leading to incorrect aggregation and comparison results.
| t.Errorf(" - NetworkReceiveBytes[Fail]: DifferencePercent: %0.2f, Prometheus: %0.2f, /allocation: %0.2f", diff_percent, networkCostValues.PromNetworkReceiveBytes, networkCostValues.AllocNetworkReceiveBytes) | ||
| } else { | ||
| t.Logf(" - NetworkReceiveBytes[Pass]: ~ %0.2f", networkCostValues.PromNetworkReceiveBytes) | ||
| } |
There was a problem hiding this comment.
Bug: NetworkReceiveBytes validation uses wrong comparison result
The withinRange variable from the NetworkTransferBytes comparison on line 181 is reused for NetworkReceiveBytes validation without recalculating it. This means NetworkReceiveBytes always reports the same pass/fail status as NetworkTransferBytes, regardless of the actual NetworkReceiveBytes values. The code needs to call utils.AreWithinPercentage again for NetworkReceiveBytes before line 187.
| // Network Internet Cost for all Pods | ||
| // -------------------------------- | ||
|
|
||
| networkInternetCost := promNetworkInternetCostResponse.Data.Result[0].Value.Value |
There was a problem hiding this comment.
Bug: Potential panic from accessing empty result array
The code accesses Data.Result[0] without checking if the Result array is empty. If the Prometheus query returns no results, this causes a panic. This can happen when the network-costs daemonset isn't running or when there's no matching data for the query window.
Additional Locations (2)
| promNetworkRegionInput.Filters = map[string]string{ | ||
| "internet": "false", | ||
| "same_Region": "false", | ||
| "same_region": "false", |
There was a problem hiding this comment.
Bug: Duplicate filter key with different casing
The filters map contains both "same_Region": "false" and "same_region": "false". Since Go map keys are case-sensitive, this creates two separate filter entries. The capital-R version appears to be a typo and likely won't match any Prometheus label, potentially causing the query to return incorrect results or no results at all.
| "internet": "false", | ||
| "same_Region": "false", | ||
| "same_region": "false", | ||
| } |
There was a problem hiding this comment.
Bug: Missing same_zone filter uses wrong same_Region key
The filter map contains same_Region (capital R) instead of same_zone. According to the comment on line 56, the query should filter by same_zone="false" along with internet="false" and same_region="false". The same_Region key is a typo that won't match any Prometheus label, and the required same_zone filter is missing entirely, causing the query to return incorrect network cost data.
Description
Network Costs
Prometheus Query
Testing
Network Costs
Network Internet Costs
Note
Adds Prometheus vs Allocation integration tests for network bytes and egress cost (internet, zone, region), and wires them into test.bats.
container_network_receive_bytes_totalandcontainer_network_transmit_bytes_totalagainst Allocation/allocationby namespace intest/integration/prometheus/network_costs_test.go.kubecost_pod_network_egress_bytes_total{internet="true"}cost vs AllocationNetworkInternetCostinnetwork_internet_costs_test.go.kubecost_pod_network_egress_bytes_total{internet="false", same_zone="false", same_region="true"}cost vs AllocationNetworkCrossZoneCostinnetwork_zone_costs_test.go.kubecost_pod_network_egress_bytes_total{internet="false", same_region="false"}cost vs AllocationNetworkCrossRegionCostinnetwork_region_costs_test.go.test/integration/prometheus/test.batsupdated to execute the new network tests.Written by Cursor Bugbot for commit 99d1524. This will update automatically on new commits. Configure here.