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:
[email protected]
