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/07/08 18:38:16 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4809: updated TM to aggregate health data and provide health data per inter…
ocket8888 commented on a change in pull request #4809:
URL: https://github.com/apache/trafficcontrol/pull/4809#discussion_r451720999
##########
File path: docs/source/api/v3/cdns_name_configs_monitoring.rst
##########
@@ -74,10 +74,11 @@ Response Structure
:name: A string that is the :ref:`Profile's Name <profile-name>`
:parameters: An array of the :term:`Parameters` in this :term:`Profile` that relate to monitoring configuration. This can be ``null`` if the servers using this :term:`Profile` cannot be monitored (e.g. Traffic Routers)
- :health.connection.timeout: A timeout value, in milliseconds, to wait before giving up on a health check request
- :health.polling.url: A URL to request for polling health. Substitutions can be made in a shell-like syntax using the properties of an object from the ``"trafficServers"`` array
- :health.threshold.availableBandwidthInKbps: The total amount of bandwidth that servers using this profile are allowed, in Kilobits per second. This is a string and using comparison operators to specify ranges, e.g. ">10" means "more than 10 kbps"
- :health.threshold.loadavg: The UNIX loadavg at which the server should be marked "unhealthy"
+ :health.connection.timeout: A timeout value, in milliseconds, to wait before giving up on a health check request
+ :health.polling.url: A URL to request for polling health. Substitutions can be made in a shell-like syntax using the properties of an object from the ``"trafficServers"`` array
+ :health.threshold.availableBandwidthInKbps: The total amount of bandwidth that servers using this profile are allowed per network interface, in Kilobits per second. This is a string and using comparison operators to specify ranges, e.g. ">10" means "more than 10 kbps"
+ :health.threshold.aggregate.availableBandwidthInKbps: The total amount of bandwidth that servers using this profile are allowed as an aggreagate across all network interfaces, in Kilobits per second. This is a string and using comparison operators to specify ranges, e.g. ">10" means "more than 10 kbps"
Review comment:
According to the blueprint:
> *"The cache server should be marked unavailable - or 'down' - if the total used bandwidth of all monitored interfaces exceeds the limits set by Parameters on the cache server's Profile,* **or** *if the bandwidth of any single interface exceeds its `maxBandwidth` property."*
So the intent was for `health.threshold.availableBandwidthInKbps` to be used to express the aggregate bandwidth allowance (which gives it the same functionality as before for servers with one interface) and each interface object has its own `maxBandwidth` property. Why add this Parameter?
##########
File path: lib/go-tc/traffic_monitor.go
##########
@@ -219,13 +221,18 @@ func (params *TMParameters) UnmarshalJSON(bytes []byte) (err error) {
}
params.Thresholds = map[string]HealthThreshold{}
+ params.AggregateThresholds = map[string]HealthThreshold{}
thresholdPrefix := "health.threshold."
+ thresholdAggregatePrefix := "health.threshold.aggregate."
Review comment:
This should probably be a `const`.
##########
File path: traffic_monitor/cache/cache.go
##########
@@ -177,54 +179,83 @@ type Filter interface {
const nsPerMs = 1000000
-type StatComputeFunc func(resultInfo ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable) interface{}
+type StatComputeFunc func(resultInfo ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable, interfaceName string) interface{}
+type AggregateComputeFunc func(resultInfo ResultInfo) interface{}
-// ComputedStats returns a map of cache stats which are computed by Traffic Monitor (rather than returned literally from ATS), mapped to the func to compute them.
-func ComputedStats() map[string]StatComputeFunc {
- return map[string]StatComputeFunc{
- "availableBandwidthInKbps": func(info ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable) interface{} {
+// ComputedAggregateStats returns a map of cache stats which are computed by Traffic Monitor (rather than returned literally from ATS), mapped to the func to compute them.
+// ComputedAggregateStats returns stats based on the aggregate data from all interfaces.
+func ComputedAggregateStats() map[string]AggregateComputeFunc {
+ return map[string]AggregateComputeFunc{
+ "availableBandwidthInKbps": func(info ResultInfo) interface{} {
return info.Vitals.MaxKbpsOut - info.Vitals.KbpsOut
},
- "availableBandwidthInMbps": func(info ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable) interface{} {
+ "availableBandwidthInMbps": func(info ResultInfo) interface{} {
return (info.Vitals.MaxKbpsOut - info.Vitals.KbpsOut) / 1000
},
- "bandwidth": func(info ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable) interface{} {
+ "bandwidth": func(info ResultInfo) interface{} {
return info.Vitals.KbpsOut
},
- "error-string": func(info ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable) interface{} {
+ "kbps": func(info ResultInfo) interface{} {
+ return info.Vitals.KbpsOut
+ },
+ "gbps": func(info ResultInfo) interface{} {
+ return float64(info.Vitals.KbpsOut) / 1000000.0
+ },
+ "loadavg": func(info ResultInfo) interface{} {
+ return info.Vitals.LoadAvg
+ },
+ "maxKbps": func(info ResultInfo) interface{} {
+ return info.Vitals.MaxKbpsOut
+ },
+ }
+}
+
+// ComputedStats returns a map of cache stats which are computed by Traffic Monitor (rather than returned literally from ATS), mapped to the func to compute them.
+func ComputedStats() map[string]StatComputeFunc {
Review comment:
The entire return map of this function is a little verbose, IMO. If you're not using an argument, you can omit a label for it because it'll never be dereferenced. So for example you have
```go
"availableBandwidthInMbps": func(info ResultInfo, serverInfo tc.LegacyTrafficServer, serverProfile tc.TMProfile, combinedState tc.IsAvailable, interfaceName string) interface{} {
return (info.InterfaceVitals[interfaceName].MaxKbpsOut - info.InterfaceVitals[interfaceName].KbpsOut) / 1000
}
```
but you're only using two of those argumentns, so this could just say:
```go
"availableBandwidthInMbps": func(info ResultInfo, tc.LegacyTrafficServer, tc.TMProfile, tc.IsAvailable, interfaceName string) interface{} {
return (info.InterfaceVitals[interfaceName].MaxKbpsOut - info.InterfaceVitals[interfaceName].KbpsOut) / 1000
}
```
Which is shorter, if only a little. I wouldn't demand that you make that change - unless you want to do it for the whole thing, removing names of unused parameters to make things easier to read.
----------------------------------------------------------------
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