rawlinp commented on code in PR #7291:
URL: https://github.com/apache/trafficcontrol/pull/7291#discussion_r1089396851
##########
traffic_ops/traffic_ops_golang/monitoring/monitoring.go:
##########
@@ -188,7 +188,7 @@ SELECT
status.name as status,
cachegroup.name as cachegroup,
me.tcp_port as port,
- profile.name as profile,
+ (SELECT STRING_AGG(sp.profile_name, '+' ORDER by sp.priority ASC) FROM
server_profile AS sp where sp.server=me.id group by sp.server) as profile,
Review Comment:
From a troubleshooting perspective, seeing the pseudo-aggregate profiles in
the monitoring snapshot is more helpful, because you don't have to do the
aggregation in your head when looking at it to see what actual health
thresholds are being used by TM for a given server. I'm trying to think of
`monitoring.json` as more of a config file than an API endpoint. From an API
perspective, sure, it would be nice to have that conformity w/ the servers API,
but from a config file perspective, breaking the format doesn't really provide
any real benefit to TM here and makes the config more confusing to operators.
TM doesn't actually gain any kind of additional functionality through this
breaking change, so why break it?
If this same change happened to be necessary for TR and the CRConfig, my
opinion would be the same, but thankfully the CRConfig doesn't list individual
profiles/params like monitoring.json. In general, the CRConfig has remained
backwards-compatible even though the API as a whole has undergone many breaking
changes. I'd think monitoring.json could be the same -- jumping through API
version hoops when upgrading for no tangible benefit is a bit of a pain.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
