You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by ma...@apache.org on 2021/12/20 22:58:04 UTC
[trafficcontrol] branch master updated: Fix TM stats_over_http parsing to sum DS data (#6423)
This is an automated email from the ASF dual-hosted git repository.
mattjackson pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new 1c956e8 Fix TM stats_over_http parsing to sum DS data (#6423)
1c956e8 is described below
commit 1c956e80f46ccbf4b7056639568240d99430a8cc
Author: Rawlin Peters <ra...@apache.org>
AuthorDate: Mon Dec 20 15:57:50 2021 -0700
Fix TM stats_over_http parsing to sum DS data (#6423)
The stats_over_http parsing was replacing stat values instead of summing
them for delivery services with multiple regexes. Since stats_over_http
response output can change order from request to request, this was
causing TM to report "new stat is lower than last stat" warnings when
the response order changed (which happens quite frequently). By properly
summing together the stats that match the same underlying delivery
service, these warnings disappear, and the reported data from TM is more
accurate.
Additionally, some things like unused parameters were cleaned up while
trying to find the underlying cause of these warnings.
---
CHANGELOG.md | 1 +
traffic_monitor/cache/astats.go | 12 ++++-----
traffic_monitor/cache/cache.go | 2 +-
traffic_monitor/cache/stats_over_http.go | 16 ++++++------
traffic_monitor/ds/stat.go | 43 ++++++++++++++------------------
traffic_monitor/ds/stat_test.go | 13 ++--------
traffic_monitor/manager/manager.go | 1 -
traffic_monitor/manager/stat.go | 15 +++--------
8 files changed, 41 insertions(+), 62 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d147dce..664b19f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -35,6 +35,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- [#6259](https://github.com/apache/trafficcontrol/issues/6259) - Traffic Portal No Longer Allows Spaces in Server Object "Router Port Name"
- [#6392](https://github.com/apache/trafficcontrol/issues/6392) - Traffic Ops prevents assigning ORG servers to topology-based delivery services (as well as a number of other valid operations being prohibited by "last server assigned to DS" validations which don't apply to topology-based delivery services)
- [#6175](https://github.com/apache/trafficcontrol/issues/6175) - POST request to /api/4.0/phys_locations accepts mismatch values for regionName.
+- Fixed Traffic Monitor parsing stats_over_http output so that multiple stats for the same underlying delivery service (when the delivery service has more than 1 regex) are properly summed together. This makes the resulting data more accurate in addition to fixing the "new stat is lower than last stat" warnings.
- [#6285](https://github.com/apache/trafficcontrol/issues/6285) - The Traffic Ops Postinstall script will work in CentOS 7, even if Python 3 is installed
- [#5373](https://github.com/apache/trafficcontrol/issues/5373) - Traffic Monitor logs not consistent
- Traffic Ops: Sanitize username before executing LDAP query
diff --git a/traffic_monitor/cache/astats.go b/traffic_monitor/cache/astats.go
index fe33dcf..00981b3 100644
--- a/traffic_monitor/cache/astats.go
+++ b/traffic_monitor/cache/astats.go
@@ -139,7 +139,7 @@ func astatsPrecompute(cacheName string, data todata.TOData, stats Statistics, mi
var err error
for stat, value := range miscStats {
- dsStats, err = astatsProcessStat(cacheName, dsStats, data, stat, value)
+ dsStats, err = astatsProcessStat(dsStats, data, stat, value)
if err != nil && err != dsdata.ErrNotProcessedStat {
log.Infof("precomputing cache %s stat %s value %v error %v", cacheName, stat, value, err)
precomputed.Errors = append(precomputed.Errors, err)
@@ -153,7 +153,7 @@ func astatsPrecompute(cacheName string, data todata.TOData, stats Statistics, mi
// astatsProcessStat and its subsidiary functions act as a State Machine,
// flowing the stat through states for each "." component of the stat name.
-func astatsProcessStat(server string, stats map[string]*DSStat, toData todata.TOData, stat string, value interface{}) (map[string]*DSStat, error) {
+func astatsProcessStat(stats map[string]*DSStat, toData todata.TOData, stat string, value interface{}) (map[string]*DSStat, error) {
parts := strings.Split(stat, ".")
if len(parts) < 1 {
return stats, fmt.Errorf("stat has no initial part")
@@ -161,7 +161,7 @@ func astatsProcessStat(server string, stats map[string]*DSStat, toData todata.TO
switch parts[0] {
case "plugin":
- return astatsProcessStatPlugin(server, stats, toData, stat, parts[1:], value)
+ return astatsProcessStatPlugin(stats, toData, parts[1:], value)
case "proxy":
fallthrough
case "server":
@@ -173,19 +173,19 @@ func astatsProcessStat(server string, stats map[string]*DSStat, toData todata.TO
}
}
-func astatsProcessStatPlugin(server string, stats map[string]*DSStat, toData todata.TOData, stat string, statParts []string, value interface{}) (map[string]*DSStat, error) {
+func astatsProcessStatPlugin(stats map[string]*DSStat, toData todata.TOData, statParts []string, value interface{}) (map[string]*DSStat, error) {
if len(statParts) < 1 {
return stats, fmt.Errorf("stat has no plugin part")
}
switch statParts[0] {
case "remap_stats":
- return astatsProcessStatPluginRemapStats(server, stats, toData, stat, statParts[1:], value)
+ return astatsProcessStatPluginRemapStats(stats, toData, statParts[1:], value)
default:
return stats, fmt.Errorf("stat has unknown plugin part '%s'", statParts[0])
}
}
-func astatsProcessStatPluginRemapStats(server string, stats map[string]*DSStat, toData todata.TOData, stat string, statParts []string, value interface{}) (map[string]*DSStat, error) {
+func astatsProcessStatPluginRemapStats(stats map[string]*DSStat, toData todata.TOData, statParts []string, value interface{}) (map[string]*DSStat, error) {
if len(statParts) < 3 {
return stats, fmt.Errorf("stat has no remap_stats deliveryservice and name parts")
}
diff --git a/traffic_monitor/cache/cache.go b/traffic_monitor/cache/cache.go
index 341b18b..211b390 100644
--- a/traffic_monitor/cache/cache.go
+++ b/traffic_monitor/cache/cache.go
@@ -280,7 +280,7 @@ func (handler Handler) Handle(id string, rdr io.Reader, format string, reqTime t
}
if reqErr != nil {
- log.Warnf("%v handler given error '%v'\n", id, reqErr) // error here, in case the thing that called Handle didn't error
+ log.Warnf("%s handler given error: %s", id, reqErr.Error()) // error here, in case the thing that called Handle didn't error
result.Error = reqErr
handler.resultChan <- result
return
diff --git a/traffic_monitor/cache/stats_over_http.go b/traffic_monitor/cache/stats_over_http.go
index 29e9a76..7926d42 100644
--- a/traffic_monitor/cache/stats_over_http.go
+++ b/traffic_monitor/cache/stats_over_http.go
@@ -343,7 +343,7 @@ func parseNumericStat(value interface{}) (uint64, error) {
if value.(int64) < 0 {
return 0, errors.New("value was negative")
}
- return uint64(value.(uint64)), nil
+ return value.(uint64), nil
case float64:
if value.(float64) > math.MaxUint64 || value.(float64) < 0 {
return 0, errors.New("value out of range for uint64")
@@ -356,7 +356,7 @@ func parseNumericStat(value interface{}) (uint64, error) {
return uint64(value.(float64)), nil
case string:
if statVal, err := strconv.ParseUint(value.(string), 10, 64); err != nil {
- return 0, fmt.Errorf("Could not parse '%v' to uint64: %v", value, err)
+ return 0, fmt.Errorf("could not parse '%v' to uint64: %v", value, err)
} else {
return statVal, nil
}
@@ -421,17 +421,17 @@ func statsOverHTTPPrecompute(cacheName string, data todata.TOData, stats Statist
switch statParts[len(statParts)-1] {
case "status_2xx":
- dsStat.Status2xx = parsedStat
+ dsStat.Status2xx += parsedStat
case "status_3xx":
- dsStat.Status3xx = parsedStat
+ dsStat.Status3xx += parsedStat
case "status_4xx":
- dsStat.Status4xx = parsedStat
+ dsStat.Status4xx += parsedStat
case "status_5xx":
- dsStat.Status5xx = parsedStat
+ dsStat.Status5xx += parsedStat
case "out_bytes":
- dsStat.OutBytes = parsedStat
+ dsStat.OutBytes += parsedStat
case "in_bytes":
- dsStat.InBytes = parsedStat
+ dsStat.InBytes += parsedStat
default:
err = fmt.Errorf("Unknown stat '%s'", statParts[len(statParts)-1])
log.Infof("precomputing cache %s stat %s value %v error %v", cacheName, stat, value, err)
diff --git a/traffic_monitor/ds/stat.go b/traffic_monitor/ds/stat.go
index 95f3b68..17bc613 100644
--- a/traffic_monitor/ds/stat.go
+++ b/traffic_monitor/ds/stat.go
@@ -42,7 +42,7 @@ func setStaticData(dsStats *dsdata.Stats, dsServers map[tc.DeliveryServiceName][
}
}
-func addAvailableData(dsStats *dsdata.Stats, crStates tc.CRStates, serverCachegroups map[tc.CacheName]tc.CacheGroupName, serverDs map[tc.CacheName][]tc.DeliveryServiceName, serverTypes map[tc.CacheName]tc.CacheType, precomputed map[tc.CacheName]cache.PrecomputedData, lastStats *dsdata.LastStats, events health.ThreadsafeEvents) {
+func addAvailableData(dsStats *dsdata.Stats, crStates tc.CRStates, serverCachegroups map[tc.CacheName]tc.CacheGroupName, serverDs map[tc.CacheName][]tc.DeliveryServiceName, serverTypes map[tc.CacheName]tc.CacheType, precomputed map[tc.CacheName]cache.PrecomputedData) {
for cache, available := range crStates.Caches {
cacheGroup, ok := serverCachegroups[cache]
if !ok {
@@ -116,14 +116,6 @@ func addAvailableData(dsStats *dsdata.Stats, crStates tc.CRStates, serverCachegr
}
}
-func newLastDSStat() *dsdata.LastDSStat {
- return &dsdata.LastDSStat{
- CacheGroups: map[tc.CacheGroupName]*dsdata.LastStatsData{},
- Type: map[tc.CacheType]*dsdata.LastStatsData{},
- Caches: map[tc.CacheName]*dsdata.LastStatsData{},
- }
-}
-
// BytesPerKilobit is the number of bytes in a kilobit.
const BytesPerKilobit = 125
@@ -141,14 +133,14 @@ func addLastStat(lastData *dsdata.LastStatData, newStat int64, newStatTime time.
if newStat < lastData.Stat {
// if a new stat comes in lower than current, assume rollover, set the 'last stat' to the new one, but leave PerSec what it was (not negative).
- err := fmt.Errorf("new stat '%d'@'%v' value less than last stat '%d'@'%v'", newStat, newStatTime, lastData.Stat, lastData.Time)
+ err := fmt.Errorf("new stat '%d'@'%s' value less than last stat '%d'@'%s'", newStat, newStatTime.Format(time.RFC3339Nano), lastData.Stat, lastData.Time.Format(time.RFC3339Nano))
lastData.Stat = newStat
lastData.Time = newStatTime
return err
}
if newStatTime.Before(lastData.Time) {
- return fmt.Errorf("new stat '%d'@'%v' time less than last stat '%d'@'%v'", newStat, newStatTime, lastData.Stat, lastData.Time)
+ return fmt.Errorf("new stat '%d'@'%s' time less than last stat '%d'@'%s'", newStat, newStatTime.Format(time.RFC3339Nano), lastData.Stat, lastData.Time.Format(time.RFC3339Nano))
}
if lastData.Stat != 0 {
@@ -233,10 +225,14 @@ func addLastDSStatTotals(lastStat *dsdata.LastDSStat, cachesReporting map[tc.Cac
// addDSPerSecStats calculates and adds the per-second delivery service stats to
// both the Stats and LastStats structures. Note this mutates both dsStats and
// lastStats, adding the per-second stats to them.
-func addDSPerSecStats(lastStats *dsdata.LastStats, dsStats *dsdata.Stats, dsName tc.DeliveryServiceName, stat *dsdata.Stat, serverCachegroups map[tc.CacheName]tc.CacheGroupName, serverTypes map[tc.CacheName]tc.CacheType, mc tc.TrafficMonitorConfigMap, events health.ThreadsafeEvents, precomputed map[tc.CacheName]cache.PrecomputedData, states peer.CRStatesThreadsafe) {
+func addDSPerSecStats(lastStats *dsdata.LastStats, dsName tc.DeliveryServiceName, stat *dsdata.Stat, serverCachegroups map[tc.CacheName]tc.CacheGroupName, serverTypes map[tc.CacheName]tc.CacheType, mc tc.TrafficMonitorConfigMap, events health.ThreadsafeEvents, precomputed map[tc.CacheName]cache.PrecomputedData, states peer.CRStatesThreadsafe) {
lastStat, lastStatExists := lastStats.DeliveryServices[dsName]
if !lastStatExists {
- lastStat = newLastDSStat() // TODO sync.Pool?
+ lastStat = &dsdata.LastDSStat{
+ CacheGroups: map[tc.CacheGroupName]*dsdata.LastStatsData{},
+ Type: map[tc.CacheType]*dsdata.LastStatsData{},
+ Caches: map[tc.CacheName]*dsdata.LastStatsData{},
+ }
lastStats.DeliveryServices[dsName] = lastStat
}
for cacheName, cacheStats := range stat.Caches {
@@ -245,14 +241,12 @@ func addDSPerSecStats(lastStats *dsdata.LastStats, dsStats *dsdata.Stats, dsName
stat.Caches[cacheName] = &dsdata.StatCacheStats{}
}
- lastStatCache := lastStat.Caches[cacheName]
- if lastStatCache == nil {
- lastStatCache = &dsdata.LastStatsData{}
- lastStat.Caches[cacheName] = lastStatCache
+ if lastStat.Caches[cacheName] == nil {
+ lastStat.Caches[cacheName] = &dsdata.LastStatsData{}
}
if _, ok := precomputed[cacheName]; ok {
- if err := addLastStats(lastStatCache, cacheStats, precomputed[cacheName].Time); err != nil {
- log.Warnf("%v adding per-second stats for cache %v: %v", dsName, cacheName, err)
+ if err := addLastStats(lastStat.Caches[cacheName], cacheStats, precomputed[cacheName].Time); err != nil {
+ log.Warnf("%s adding per-second stats for cache %s: %s", dsName.String(), cacheName.String(), err.Error())
continue
}
}
@@ -360,7 +354,7 @@ func addCachePerSecStats(lastStats *dsdata.LastStats, cacheName tc.CacheName, pr
// them.
func addPerSecStats(precomputed map[tc.CacheName]cache.PrecomputedData, dsStats *dsdata.Stats, lastStats *dsdata.LastStats, serverCachegroups map[tc.CacheName]tc.CacheGroupName, serverTypes map[tc.CacheName]tc.CacheType, mc tc.TrafficMonitorConfigMap, events health.ThreadsafeEvents, states peer.CRStatesThreadsafe) {
for dsName, stat := range dsStats.DeliveryService {
- addDSPerSecStats(lastStats, dsStats, dsName, stat, serverCachegroups, serverTypes, mc, events, precomputed, states)
+ addDSPerSecStats(lastStats, dsName, stat, serverCachegroups, serverTypes, mc, events, precomputed, states)
}
for cacheName, precomputedData := range precomputed {
addCachePerSecStats(lastStats, cacheName, precomputedData)
@@ -369,7 +363,7 @@ func addPerSecStats(precomputed map[tc.CacheName]cache.PrecomputedData, dsStats
// CreateStats aggregates and creates statistics from given precomputed stat history. It returns the created stats, information about these stats necessary for the next calculation, and any error.
// Note lastStats is mutated, being set with the new last stats.
-func CreateStats(precomputed map[tc.CacheName]cache.PrecomputedData, toData todata.TOData, crStates tc.CRStates, lastStats *dsdata.LastStats, now time.Time, mc tc.TrafficMonitorConfigMap, events health.ThreadsafeEvents, states peer.CRStatesThreadsafe) (*dsdata.Stats, error) {
+func CreateStats(precomputed map[tc.CacheName]cache.PrecomputedData, toData todata.TOData, crStates tc.CRStates, lastStats *dsdata.LastStats, mc tc.TrafficMonitorConfigMap, events health.ThreadsafeEvents, states peer.CRStatesThreadsafe) *dsdata.Stats {
start := time.Now()
dsStats := dsdata.NewStats(len(toData.DeliveryServiceServers)) // TODO sync.Pool?
for deliveryService := range toData.DeliveryServiceServers {
@@ -380,12 +374,13 @@ func CreateStats(precomputed map[tc.CacheName]cache.PrecomputedData, toData toda
dsStats.DeliveryService[deliveryService] = dsdata.NewStat() // TODO sync.Pool?
}
setStaticData(dsStats, toData.DeliveryServiceServers)
- addAvailableData(dsStats, crStates, toData.ServerCachegroups, toData.ServerDeliveryServices, toData.ServerTypes, precomputed, lastStats, events) // TODO move after stat summarisation
+ addAvailableData(dsStats, crStates, toData.ServerCachegroups, toData.ServerDeliveryServices, toData.ServerTypes, precomputed) // TODO move after stat summarisation
for server, precomputedData := range precomputed {
cachegroup, ok := toData.ServerCachegroups[server]
if !ok {
- log.Warnf("server %s has no cachegroup, skipping\n", server)
+ // this can happen if we have precomputed data for a cache but the cache has since been deleted from Traffic Ops
+ log.Infof("server %s has no cachegroup, skipping", server)
continue
}
serverType, ok := toData.ServerTypes[server]
@@ -436,7 +431,7 @@ func CreateStats(precomputed map[tc.CacheName]cache.PrecomputedData, toData toda
addPerSecStats(precomputed, dsStats, lastStats, toData.ServerCachegroups, toData.ServerTypes, mc, events, states)
log.Infof("CreateStats took %v\n", time.Since(start))
dsStats.Time = time.Now()
- return dsStats, nil
+ return dsStats
}
func getDSErr(dsName tc.DeliveryServiceName, dsStats dsdata.StatCacheStats, monitorConfig tc.TrafficMonitorConfigMap) error {
diff --git a/traffic_monitor/ds/stat_test.go b/traffic_monitor/ds/stat_test.go
index 62d442a..00e2814 100644
--- a/traffic_monitor/ds/stat_test.go
+++ b/traffic_monitor/ds/stat_test.go
@@ -28,7 +28,6 @@ import (
"math/rand"
"regexp"
"testing"
- "time"
tc_log "github.com/apache/trafficcontrol/lib/go-log"
"github.com/apache/trafficcontrol/lib/go-tc"
@@ -60,7 +59,6 @@ func TestCreateStats(t *testing.T) {
toData := getMockTOData()
combinedCRStates := peer.NewCRStatesThreadsafe()
lastStatsThs := threadsafe.NewLastStats()
- now := time.Now()
maxEvents := uint64(4)
events := health.NewThreadsafeEvents(maxEvents)
localCRStates := peer.NewCRStatesThreadsafe()
@@ -87,20 +85,13 @@ func TestCreateStats(t *testing.T) {
lastStatsVal := lastStatsThs.Get()
lastStatsCopy := lastStatsVal.Copy()
- dsStats, err := CreateStats(precomputeds, toData, combinedCRStates.Get(), lastStatsCopy, now, monitorConfig, events, localCRStates)
-
- if err != nil {
- t.Fatalf("CreateStats err expected: nil, actual: " + err.Error())
- }
+ dsStats := CreateStats(precomputeds, toData, combinedCRStates.Get(), lastStatsCopy, monitorConfig, events, localCRStates)
serverDeliveryServices := toData.ServerDeliveryServices
toData.ServerDeliveryServices = map[tc.CacheName][]tc.DeliveryServiceName{} // temporarily unassign servers to generate warnings about caches not assigned to delivery services
buffer := bytes.NewBuffer(make([]byte, 0, 10000))
tc_log.Info = log.New(buffer, "TestAddAvailabilityDataNotFoundInDeliveryService", log.Lshortfile)
- _, err = CreateStats(precomputeds, toData, combinedCRStates.Get(), lastStatsCopy, now, monitorConfig, events, localCRStates)
- if err != nil {
- t.Fatalf("CreateStats err expected: nil, actual: " + err.Error())
- }
+ _ = CreateStats(precomputeds, toData, combinedCRStates.Get(), lastStatsCopy, monitorConfig, events, localCRStates)
checkLogOutput(t, buffer, toData, caches)
toData.ServerDeliveryServices = serverDeliveryServices
diff --git a/traffic_monitor/manager/manager.go b/traffic_monitor/manager/manager.go
index c7fe838..3d38c76 100644
--- a/traffic_monitor/manager/manager.go
+++ b/traffic_monitor/manager/manager.go
@@ -112,7 +112,6 @@ func Start(opsConfigFile string, cfg config.Config, appData config.StaticAppData
combinedStates,
toData,
cachesChangedForStatMgr,
- errorCount,
cfg,
monitorConfig,
events,
diff --git a/traffic_monitor/manager/stat.go b/traffic_monitor/manager/stat.go
index 18fa698..d3878b5 100644
--- a/traffic_monitor/manager/stat.go
+++ b/traffic_monitor/manager/stat.go
@@ -64,7 +64,6 @@ func StartStatHistoryManager(
combinedStates peer.CRStatesThreadsafe,
toData todata.TODataThreadsafe,
cachesChanged <-chan struct{},
- errorCount threadsafe.Uint,
cfg config.Config,
monitorConfig threadsafe.TrafficMonitorConfigMap,
events health.ThreadsafeEvents,
@@ -97,7 +96,7 @@ func StartStatHistoryManager(
if haveCachesChanged() {
statUnpolledCaches.SetNewCaches(getNewCaches(localStates, monitorConfig))
}
- processStatResults(results, statInfoHistory, statResultHistory, statMaxKbpses, combinedStates, lastStats, toData.Get(), errorCount, dsStats, lastStatEndTimes, lastStatDurations, statUnpolledCaches, monitorConfig.Get(), precomputedData, lastResults, localStates, events, localCacheStatus, combineState, cfg.CachePollingProtocol)
+ processStatResults(results, statInfoHistory, statResultHistory, statMaxKbpses, combinedStates, lastStats, toData.Get(), dsStats, lastStatEndTimes, lastStatDurations, statUnpolledCaches, monitorConfig.Get(), precomputedData, lastResults, localStates, events, localCacheStatus, combineState, cfg.CachePollingProtocol)
}
go func() {
@@ -241,7 +240,6 @@ func processStatResults(
combinedStatesThreadsafe peer.CRStatesThreadsafe,
lastStats threadsafe.LastStats,
toData todata.TOData,
- errorCount threadsafe.Uint,
dsStats threadsafe.DSStats,
lastStatEndTimes map[tc.CacheName]time.Time,
lastStatDurationsThreadsafe threadsafe.DurationMap,
@@ -307,15 +305,10 @@ func processStatResults(
lastStatsVal := lastStats.Get()
lastStatsCopy := lastStatsVal.Copy()
- newDsStats, err := ds.CreateStats(precomputedData, toData, combinedStates, lastStatsCopy, time.Now(), mc, events, localStates)
+ newDsStats := ds.CreateStats(precomputedData, toData, combinedStates, lastStatsCopy, mc, events, localStates)
- if err != nil {
- errorCount.Inc()
- log.Errorf("getting deliveryservice: %v\n", err)
- } else {
- dsStats.Set(*newDsStats)
- lastStats.Set(*lastStatsCopy)
- }
+ dsStats.Set(*newDsStats)
+ lastStats.Set(*lastStatsCopy)
pollerName := "stat"
health.CalcAvailability(results, pollerName, &statResultHistoryThreadsafe, mc, toData, localCacheStatusThreadsafe, localStates, events, pollingProtocol)