Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintenance: improved stat5minClientRequests() naming #275

Closed
wants to merge 5 commits into from
Closed

Maintenance: improved stat5minClientRequests() naming #275

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9145a8f
Renamed stat5minClientRequests() to reflect its meaning
eduard-bagdasaryan Nov 19, 2024
90aa746
Polished and added a comment
eduard-bagdasaryan Nov 20, 2024
b50d546
Let statSawRecentRequests() give requests number on startup
eduard-bagdasaryan Nov 21, 2024
f936f71
Merged from master
eduard-bagdasaryan Nov 21, 2024
e2478f7
No 'static' for an integer constant
eduard-bagdasaryan Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/neighbors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,7 @@ peerRefreshDNS(void *data)
if (eventFind(peerRefreshDNS, nullptr))
eventDelete(peerRefreshDNS, nullptr);

if (!data && 0 == stat5minClientRequests()) {
if (!data && !statSawRecentRequests()) {
/* no recent client traffic, wait a bit */
eventAddIsh("peerRefreshDNS", peerRefreshDNS, nullptr, 180.0, 1);
return;
Expand Down
12 changes: 8 additions & 4 deletions src/stat.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1684,11 +1684,15 @@ snmpStatGet(int minutes)
return &CountHist[minutes];
}

int
stat5minClientRequests(void)
bool
statSawRecentRequests()
{
assert(N_COUNT_HIST > 5);
return statCounter.client_http.requests - CountHist[5].client_http.requests;
static const int recentMinutes = 5;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AAA if possible:

Suggested change
static const int recentMinutes = 5;
static const auto recentMinutes = 5;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

assert(N_COUNT_HIST > recentMinutes);

if (NCountHist > recentMinutes)
rousskov marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's document why this math is not precise:

Suggested change
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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return statCounter.client_http.requests - CountHist[recentMinutes].client_http.requests;
return 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of recent requests within the first few minutes of Squid process live is not always zero, right?

Suggested change
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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

}

static double
Expand Down
5 changes: 3 additions & 2 deletions src/stat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
void statInit(void);
double median_svc_get(int, int);
void pconnHistCount(int, int);
int stat5minClientRequests(void);
double stat5minCPUUsage(void);
/// whether we processed any incoming requests in the last few minutes
/// \sa ClientHttpRequest::updateCounters()
bool statSawRecentRequests();
double statRequestHitRatio(int minutes);
double statRequestHitMemoryRatio(int minutes);
double statRequestHitDiskRatio(int minutes);
Expand Down