You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/09/23 18:07:39 UTC
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5031: Fix CacheStats api to allow upgrading
srijeet0406 commented on a change in pull request #5031:
URL: https://github.com/apache/trafficcontrol/pull/5031#discussion_r493744704
##########
File path: lib/go-tc/traffic_monitor_test.go
##########
@@ -23,8 +23,139 @@ import (
"encoding/json"
"fmt"
"testing"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-util"
)
+func makeFakeStats(text string) map[string][]ResultStatVal {
+ return map[string][]ResultStatVal{
+ text + "stat1": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ text + "stat2": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ text + "stat3": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ }
+}
+
+func makeFakeInterfaces() map[string]map[string][]ResultStatVal {
+ return map[string]map[string][]ResultStatVal{
+ "interface1": makeFakeStats("interf"),
+ "interface2": makeFakeStats("interf"),
+ "interface3": makeFakeStats("interf"),
+ }
+}
+func TestLegacyStatsConversion(t *testing.T) {
+ stats := Stats{
+ CommonAPIData: CommonAPIData{},
+ Caches: make(map[string]ServerStats),
+ }
+ config := TrafficMonitorConfigMap{
+ TrafficServer: make(map[string]TrafficServer),
+ }
+ for _, cacheName := range []string{"cache1", "cache2", "cache3"} {
+ stats.Caches[cacheName] = ServerStats{
+ Interfaces: makeFakeInterfaces(),
+ Stats: makeFakeStats(""),
+ }
+ interfaces := []ServerInterfaceInfo{}
+ for name, _ := range stats.Caches[cacheName].Interfaces {
+ interfaces = append(interfaces, ServerInterfaceInfo{
+ IPAddresses: []ServerIPAddress{
+ {
+ Address: "192.168.0.8",
+ Gateway: util.StrPtr("192.168.0.1"),
+ ServiceAddress: true,
+ },
+ },
+ MaxBandwidth: util.Uint64Ptr(1500),
+ Monitor: false,
+ MTU: util.UInt64Ptr(1500),
+ Name: name,
+ })
+ }
+ interfaces[0].Monitor = true
+ config.TrafficServer[cacheName] = TrafficServer{
+ Interfaces: interfaces,
+ }
+ }
+
+ issues, legacyStats := stats.ToLegacy(config)
+
+ if len(issues) != 0 {
+ t.Error("expect no issues")
+ }
+
+ if legacyStats.CommonAPIData != stats.CommonAPIData {
+ t.Error("expected CommonAPIData to be the same")
Review comment:
Could we print the difference here? Might be easier to debug that way.
##########
File path: lib/go-tc/traffic_monitor_test.go
##########
@@ -23,8 +23,139 @@ import (
"encoding/json"
"fmt"
"testing"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-util"
)
+func makeFakeStats(text string) map[string][]ResultStatVal {
+ return map[string][]ResultStatVal{
+ text + "stat1": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ text + "stat2": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ text + "stat3": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ }
+}
+
+func makeFakeInterfaces() map[string]map[string][]ResultStatVal {
+ return map[string]map[string][]ResultStatVal{
+ "interface1": makeFakeStats("interf"),
+ "interface2": makeFakeStats("interf"),
+ "interface3": makeFakeStats("interf"),
+ }
+}
+func TestLegacyStatsConversion(t *testing.T) {
+ stats := Stats{
+ CommonAPIData: CommonAPIData{},
+ Caches: make(map[string]ServerStats),
+ }
+ config := TrafficMonitorConfigMap{
+ TrafficServer: make(map[string]TrafficServer),
+ }
+ for _, cacheName := range []string{"cache1", "cache2", "cache3"} {
+ stats.Caches[cacheName] = ServerStats{
+ Interfaces: makeFakeInterfaces(),
+ Stats: makeFakeStats(""),
+ }
+ interfaces := []ServerInterfaceInfo{}
+ for name, _ := range stats.Caches[cacheName].Interfaces {
+ interfaces = append(interfaces, ServerInterfaceInfo{
+ IPAddresses: []ServerIPAddress{
+ {
+ Address: "192.168.0.8",
+ Gateway: util.StrPtr("192.168.0.1"),
+ ServiceAddress: true,
+ },
+ },
+ MaxBandwidth: util.Uint64Ptr(1500),
+ Monitor: false,
+ MTU: util.UInt64Ptr(1500),
+ Name: name,
+ })
+ }
+ interfaces[0].Monitor = true
+ config.TrafficServer[cacheName] = TrafficServer{
+ Interfaces: interfaces,
+ }
+ }
+
+ issues, legacyStats := stats.ToLegacy(config)
+
+ if len(issues) != 0 {
+ t.Error("expect no issues")
+ }
+
+ if legacyStats.CommonAPIData != stats.CommonAPIData {
+ t.Error("expected CommonAPIData to be the same")
+ }
+
+ if len(legacyStats.Caches) != len(stats.Caches) {
+ t.Errorf("expected %v caches, got %v", len(stats.Caches), len(legacyStats.Caches))
+ }
+
+ for cacheName, legacyCache := range legacyStats.Caches {
+ cache, ok := stats.Caches[string(cacheName)]
+ if !ok {
+ t.Errorf("new interface %v found", cacheName)
Review comment:
Is this the correct error msg?
##########
File path: lib/go-tc/traffic_monitor_test.go
##########
@@ -23,8 +23,139 @@ import (
"encoding/json"
"fmt"
"testing"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-util"
)
+func makeFakeStats(text string) map[string][]ResultStatVal {
+ return map[string][]ResultStatVal{
+ text + "stat1": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ text + "stat2": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ text + "stat3": {
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ {
+ Span: 50,
+ Time: time.Now(),
+ Val: 50,
+ },
+ },
+ }
+}
+
+func makeFakeInterfaces() map[string]map[string][]ResultStatVal {
+ return map[string]map[string][]ResultStatVal{
+ "interface1": makeFakeStats("interf"),
+ "interface2": makeFakeStats("interf"),
+ "interface3": makeFakeStats("interf"),
+ }
+}
+func TestLegacyStatsConversion(t *testing.T) {
+ stats := Stats{
+ CommonAPIData: CommonAPIData{},
+ Caches: make(map[string]ServerStats),
+ }
+ config := TrafficMonitorConfigMap{
+ TrafficServer: make(map[string]TrafficServer),
+ }
+ for _, cacheName := range []string{"cache1", "cache2", "cache3"} {
+ stats.Caches[cacheName] = ServerStats{
+ Interfaces: makeFakeInterfaces(),
+ Stats: makeFakeStats(""),
+ }
+ interfaces := []ServerInterfaceInfo{}
+ for name, _ := range stats.Caches[cacheName].Interfaces {
+ interfaces = append(interfaces, ServerInterfaceInfo{
+ IPAddresses: []ServerIPAddress{
+ {
+ Address: "192.168.0.8",
+ Gateway: util.StrPtr("192.168.0.1"),
+ ServiceAddress: true,
+ },
+ },
+ MaxBandwidth: util.Uint64Ptr(1500),
+ Monitor: false,
+ MTU: util.UInt64Ptr(1500),
+ Name: name,
+ })
+ }
+ interfaces[0].Monitor = true
+ config.TrafficServer[cacheName] = TrafficServer{
+ Interfaces: interfaces,
+ }
+ }
+
+ issues, legacyStats := stats.ToLegacy(config)
+
+ if len(issues) != 0 {
+ t.Error("expect no issues")
+ }
+
+ if legacyStats.CommonAPIData != stats.CommonAPIData {
+ t.Error("expected CommonAPIData to be the same")
+ }
+
+ if len(legacyStats.Caches) != len(stats.Caches) {
+ t.Errorf("expected %v caches, got %v", len(stats.Caches), len(legacyStats.Caches))
+ }
+
+ for cacheName, legacyCache := range legacyStats.Caches {
+ cache, ok := stats.Caches[string(cacheName)]
+ if !ok {
+ t.Errorf("new interface %v found", cacheName)
+ }
+ interf := cache.Interfaces["interface1"]
+ if len(interf)+len(cache.Stats) != len(legacyCache) {
+ t.Errorf("expected %v stats, got %v", len(interf)+len(cache.Stats), len(legacyCache))
+ }
+ }
+}
+
+func TestLegacyStatsNilConversion(t *testing.T) {
+ stats := Stats{
+ CommonAPIData: CommonAPIData{},
+ Caches: nil,
+ }
+ config := TrafficMonitorConfigMap{
+ TrafficServer: nil,
+ }
+ issues, legacyStats := stats.ToLegacy(config)
+
+ if legacyStats.CommonAPIData != stats.CommonAPIData {
+ t.Error("expected CommonAPIData to be the samee")
Review comment:
Same comment about printing the differences here.
##########
File path: traffic_monitor/datareq/datareq.go
##########
@@ -78,9 +78,12 @@ func MakeDispatchMap(
bytes, statusCode, err := srvTRState(params, localStates, combinedStates, peerStates)
return WrapErrStatusCode(errorCount, path, bytes, statusCode, err)
}, rfc.ApplicationJSON)),
- "/publish/CacheStats": wrap(WrapParams(func(params url.Values, path string) ([]byte, int) {
+ "/publish/CacheStats2": wrap(WrapParams(func(params url.Values, path string) ([]byte, int) {
Review comment:
Could we rename this to `CacheStatsNew`?
##########
File path: traffic_ops/traffic_ops_golang/cachesstats/cachesstats.go
##########
@@ -128,49 +133,22 @@ func addTotals(data []CacheData) []CacheData {
return data
}
-// CRStates contains the Monitor CacheStats needed by Cachedata. It is NOT the full object served by the Monitor, but only the data required by the caches stats endpoint.
-type CacheStats struct {
- Caches map[tc.CacheName]CacheStat `json:"caches"`
-}
-
-type CacheStat struct {
- BandwidthKBPS []CacheStatData `json:"bandwidth"`
- Connections []CacheStatData `json:"ats.proxy.process.http.current_client_connections"`
-}
-
-type CacheStatData struct {
- Value int64 `json:"value,string"`
-}
-
-func getCacheStats(monitorFQDN string, client *http.Client) (CacheStats, error) {
- path := `/publish/CacheStats?stats=ats.proxy.process.http.current_client_connections,bandwidth`
- resp, err := client.Get("http://" + monitorFQDN + path)
- if err != nil {
- return CacheStats{}, errors.New("getting CacheStats from Monitor '" + monitorFQDN + "': " + err.Error())
- }
- defer resp.Body.Close()
-
- cs := CacheStats{}
- if err := json.NewDecoder(resp.Body).Decode(&cs); err != nil {
- return CacheStats{}, errors.New("decoding CacheStats from monitor '" + monitorFQDN + "': " + err.Error())
- }
- return cs, nil
-}
-
-func addStats(cacheData []CacheData, stats CacheStats) []CacheData {
+func addStats(cacheData []CacheData, stats tc.Stats) []CacheData {
if stats.Caches == nil {
return cacheData // TODO warn?
}
for i, cache := range cacheData {
- stat, ok := stats.Caches[cache.HostName]
+ stat, ok := stats.Caches[string(cache.HostName)]
if !ok {
continue
}
- if len(stat.BandwidthKBPS) > 0 {
- cache.KBPS = uint64(stat.BandwidthKBPS[0].Value)
+ bandwidth, ok := stat.Stats["bandwidth"]
Review comment:
And then just use the constants here
##########
File path: traffic_ops/traffic_ops_golang/cachesstats/cachesstats.go
##########
@@ -128,49 +133,22 @@ func addTotals(data []CacheData) []CacheData {
return data
}
-// CRStates contains the Monitor CacheStats needed by Cachedata. It is NOT the full object served by the Monitor, but only the data required by the caches stats endpoint.
-type CacheStats struct {
- Caches map[tc.CacheName]CacheStat `json:"caches"`
-}
-
-type CacheStat struct {
- BandwidthKBPS []CacheStatData `json:"bandwidth"`
- Connections []CacheStatData `json:"ats.proxy.process.http.current_client_connections"`
-}
-
-type CacheStatData struct {
- Value int64 `json:"value,string"`
-}
-
-func getCacheStats(monitorFQDN string, client *http.Client) (CacheStats, error) {
- path := `/publish/CacheStats?stats=ats.proxy.process.http.current_client_connections,bandwidth`
- resp, err := client.Get("http://" + monitorFQDN + path)
- if err != nil {
- return CacheStats{}, errors.New("getting CacheStats from Monitor '" + monitorFQDN + "': " + err.Error())
- }
- defer resp.Body.Close()
-
- cs := CacheStats{}
- if err := json.NewDecoder(resp.Body).Decode(&cs); err != nil {
- return CacheStats{}, errors.New("decoding CacheStats from monitor '" + monitorFQDN + "': " + err.Error())
- }
- return cs, nil
-}
-
-func addStats(cacheData []CacheData, stats CacheStats) []CacheData {
+func addStats(cacheData []CacheData, stats tc.Stats) []CacheData {
if stats.Caches == nil {
return cacheData // TODO warn?
}
for i, cache := range cacheData {
- stat, ok := stats.Caches[cache.HostName]
+ stat, ok := stats.Caches[string(cache.HostName)]
if !ok {
continue
}
- if len(stat.BandwidthKBPS) > 0 {
- cache.KBPS = uint64(stat.BandwidthKBPS[0].Value)
+ bandwidth, ok := stat.Stats["bandwidth"]
+ if ok && len(bandwidth) > 0 {
+ cache.KBPS = bandwidth[0].Val.(uint64)
}
- if len(stat.Connections) > 0 {
- cache.Connections = uint64(stat.Connections[0].Value)
+ connections, ok := stat.Stats["ats.proxy.process.http.current_client_connections"]
Review comment:
Same as above
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -140,10 +139,16 @@ func getCapacityData(monitors map[tc.CDNName][]string, thresholds map[string]flo
log.Warnln("getCapacity failed to get CRConfig from cdn '" + string(cdn) + " monitor '" + monitorFQDN + "', trying next monitor: " + err.Error())
continue
}
- if err := getCacheStats(monitorFQDN, client, []string{"kbps", "maxKbps"}, &cacheStats); err != nil {
+ if cacheStats, err = monitorhlp.GetCacheStats(monitorFQDN, client, []string{"kbps", "maxKbps"}); err != nil {
Review comment:
Same comment about constants here
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -199,42 +195,21 @@ func addCapacity(cap CapData, cacheStats CacheStats, crStates tc.CRStates, crCon
return cap
}
-func getStatsFromServiceInterface(stats CacheStat) (float64, float64, error) {
- var kbps, maxKbps float64
- if len(stats.KBPS) < 1 ||
- len(stats.MaxKBPS) < 1 {
- return kbps, maxKbps, errors.New("no kbps/ maxKbps stats to return")
+func getStatsFromServiceInterface(stats tc.ServerStats) (float64, float64, error) {
Review comment:
Since this no longer has any real relation with the `service interface`, you could probably rename this to just `getStats`.
##########
File path: traffic_monitor/datareq/cachestat.go
##########
@@ -37,3 +37,13 @@ func srvCacheStats(params url.Values, errorCount threadsafe.Uint, path string, t
bytes, err := threadsafe.StatsMarshall(statResultHistory, statInfoHistory.Get(), combinedStates.Get(), monitorConfig.Get(), statMaxKbpses.Get(), filter, params)
return WrapErrCode(errorCount, path, bytes, err)
}
+
+func srvLegacyCacheStats(params url.Values, errorCount threadsafe.Uint, path string, toData todata.TODataThreadsafe, statResultHistory threadsafe.ResultStatHistory, statInfoHistory threadsafe.ResultInfoHistory, monitorConfig threadsafe.TrafficMonitorConfigMap, combinedStates peer.CRStatesThreadsafe, statMaxKbpses threadsafe.CacheKbpses) ([]byte, int) {
Review comment:
IDK if running `go fmt` on this file will split this long line into two or not, but its a really long line.
##########
File path: traffic_monitor/static/index.html
##########
@@ -38,7 +38,7 @@
<div id="links">
<div>
<a href="/publish/EventLog">EventLog</a>
- <a href="/publish/CacheStats">CacheStats</a>
+ <a href="/publish/CacheStats2">CacheStats</a>
Review comment:
Same comment for renaming here as well
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/capacity.go
##########
@@ -116,7 +115,11 @@ func getCapacity(tx *sql.Tx, ds tc.DeliveryServiceName, cdn tc.CDNName) (Capacit
}
cacheStats, err := monitorhlp.GetCacheStats(monitorFQDN, client, []string{"maxKbps", "kbps"})
if err != nil {
- return CapacityResp{}, errors.New("getting CacheStats for delivery service '" + string(ds) + "' monitor '" + monitorFQDN + "': " + err.Error())
+ legacyCacheStats, err := monitorhlp.GetLegacyCacheStats(monitorFQDN, client, []string{"maxKbps", "kbps"})
Review comment:
Maybe create constants for kbps and maxKbps as well?
##########
File path: traffic_ops/traffic_ops_golang/cachesstats/cachesstats.go
##########
@@ -77,10 +76,16 @@ func getCachesStats(tx *sql.Tx) ([]CacheData, error) {
continue
}
- cacheStats, err := getCacheStats(monitorFQDN, client)
+ var cacheStats tc.Stats
+ stats := []string{"ats.proxy.process.http.current_client_connections", "bandwidth"}
Review comment:
Could we create constants for these strings?
##########
File path: traffic_ops/traffic_ops_golang/cdn/capacity.go
##########
@@ -199,42 +195,21 @@ func addCapacity(cap CapData, cacheStats CacheStats, crStates tc.CRStates, crCon
return cap
}
-func getStatsFromServiceInterface(stats CacheStat) (float64, float64, error) {
- var kbps, maxKbps float64
- if len(stats.KBPS) < 1 ||
- len(stats.MaxKBPS) < 1 {
- return kbps, maxKbps, errors.New("no kbps/ maxKbps stats to return")
+func getStatsFromServiceInterface(stats tc.ServerStats) (float64, float64, error) {
+ kbps, ok := stats.Stats["kbps"]
+ if !ok {
+ return 0, 0, errors.New("no kbps stats")
}
- kbps = stats.KBPS[0].Value
- maxKbps = stats.MaxKBPS[0].Value
- return kbps, maxKbps, nil
-}
+ maxKbps, ok := stats.Stats["maxKbps"]
+ if !ok {
+ return 0, 0, errors.New("no maxKbps stats")
-func getServiceInterfaces(tx *sql.Tx) (map[string]string, error) {
- query := `
-SELECT s.host_name, i.interface FROM ip_address i
-JOIN server s ON s.id = i.server
-WHERE i.service_address=true
-`
- rows, err := tx.Query(query)
- if err != nil {
- log.Errorf("couldn't get service interfaces %v", err.Error())
- return nil, err
}
- defer rows.Close()
-
- resultMap := make(map[string]string)
- for rows.Next() {
- hostname := ""
- serviceinterface := ""
- err = rows.Scan(&hostname, &serviceinterface)
- if err != nil {
- log.Errorf("error unmarshalling response %v", err.Error())
- continue
- }
- resultMap[hostname] = serviceinterface
+ if len(kbps) < 1 ||
+ len(maxKbps) < 1 {
+ return 0, 0, errors.New("no kbps/ maxKbps stats to return")
}
- return resultMap, nil
+ return kbps[0].Val.(float64), maxKbps[0].Val.(float64), nil
Review comment:
Could you add some checks for converting the interface into a float? I think there is a method called `util.ToNumeric()` that does this for you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org