Maintenance: improved stat5minClientRequests() naming #275
Conversation
src/stat.cc
Outdated
|
|
||
| if (NCountHist > recentMinutes) | ||
| return statCounter.client_http.requests - CountHist[recentMinutes].client_http.requests; | ||
| return 0; |
There was a problem hiding this comment.
The number of recent requests within the first few minutes of Squid process live is not always zero, right?
| return 0; | |
| return statCounter.client_http.requests; |
Alternatively, we can use this pattern (that reduces statCounter duplication):
const auto oldRequests = (NCountHist > recentMinutes) ? CountHist[recentMinutes].client_http.requests : 0;
return statCounter.client_http.requests - oldRequests;Your call.
There was a problem hiding this comment.
The number of recent requests within the first few minutes of Squid process live is not always zero
Right, but I thought that since this function cannot produce a meaningful result within these initial few minutes, it should not confuse the caller (which will proceed refreshing DNS). Theoretically, this function could return an 'optional', and the caller would wait in 'zero' and 'undefined' outcomes.
There was a problem hiding this comment.
this function cannot produce a meaningful result within these initial few minutes
I disagree with that assertion. Both true and false statSawRecentRequests() outcomes during the first few minutes seem perfectly meaningful to me (regardless of how an existing or future caller may use those outcomes).
it should not confuse the caller (which will proceed refreshing DNS).
Refreshing code that triggers a refresh just because statSawRecentRequests() returned true is confused for other reasons. No reasonable caller will refresh just because statSawRecentRequests() returned true!
Refreshing code that avoids a refresh just because statSawRecentRequests() returned false is reasonable and will not malfunction (or, to be more precise, any malfunction is outside this discussion scope).
In both cases, this function will not cause or create confusion where none existed.
Theoretically, this function could return an 'optional', and the caller would wait in 'zero' and 'undefined' outcomes.
That would be a good strategy for a function designed to classify cases where there were no requests for the first X minutes as "cannot tell; have to wait for X minutes to pass". Such a function should be named something like statSawRequestsDuringLastXminutes() so that the caller can correctly handle its dependency on X.
I see no reason to complicate our function right now. The vagueness of "recent" allows it to produce correct boolean result at any time. The caller is not dependent on any specific X value -- the caller should work correctly after sleeping for any reasonable (for that caller!) time between calls as long as that caller is OK with vague "recent" definition (e.g., because the caller sleeps for periods that ought to exceed statistics collection periods by a lot). And changing statistics collection interval to any reasonable value will not affect any such caller.
src/stat.cc
Outdated
| static const int recentMinutes = 5; | ||
| assert(N_COUNT_HIST > recentMinutes); | ||
|
|
||
| if (NCountHist > recentMinutes) |
There was a problem hiding this comment.
Let's document why this math is not precise:
| if (NCountHist > recentMinutes) | |
| // Math below computes the number of requests during the last 0-6 minutes. | |
| // CountHist is based on "minutes passed since Squid start" periods. It cannot | |
| // deliver precise info for "last N minutes", but we do not need to be precise. | |
| if (NCountHist > recentMinutes) |
src/stat.cc
Outdated
| { | ||
| assert(N_COUNT_HIST > 5); | ||
| return statCounter.client_http.requests - CountHist[5].client_http.requests; | ||
| static const int recentMinutes = 5; |
There was a problem hiding this comment.
AAA if possible:
| static const int recentMinutes = 5; | |
| static const auto recentMinutes = 5; |
|
This work became official PR 1951. |
stat5minClientRequests() was to meant to return the number of recent
client requests. However, the function did not provide implied 5 minute
precision. It returned, roughly speaking, the number of requests during
the last 0-6 minutes. The new, less strict function name and boolean
type avoid this false precision implication.
TODO: This function tracks the number of already processed/logged
requests, instead of the number of received/incoming client requests.
The time gap between receiving a request and logging it may be
significant, especially when bad/stale cache_peer addresses lead to
various retries and timeouts. In such situations, incoming requests
would benefit from fresh cache_peer addresses, so the function should
return true sooner, based on incoming (rather than processed) request
counters.
Also removed unused stat5minCPUUsage().