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.

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

Emit YAML for events cachemgr report #1792

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Apr 25, 2024

Also EventScheduler::dump to be stream-based,
and simplify TestEvent::testDump

@kinkie
Copy link
Contributor Author

kinkie commented Apr 25, 2024

Single-worker output in master:

Last event to run: MaintainSwapSpace

Operation                   Next Execution  Weight  Callback Valid?
logfileFlush                0.039 sec       1    yes
MaintainSwapSpace           0.039 sec       1    N/A
fqdncache_purgelru          2.429 sec       1    N/A
ipcache_purgelru            7.531 sec       1    N/A
memPoolCleanIdlePools       13.968 sec      1    N/A
statAvgTick                 38.488 sec      1    N/A
peerRefreshDNS              100.777 sec     1    N/A
peerClearRR                 216.965 sec     0    N/A
netdbSaveState              1711.270 sec        1    N/A

Output with PR applied:

last event to run: MaintainSwapSpace
scheduled events:
  - operation: logfileFlush
    secs to next execution: 0.424
    weight: 1
    callback valid: yes
  - operation: MaintainSwapSpace
    secs to next execution: 0.438
    weight: 1
    callback valid: N/A
  - operation: fqdncache_purgelru
    secs to next execution: 4.427
    weight: 1
    callback valid: N/A
  - operation: memPoolCleanIdlePools
    secs to next execution: 4.427
    weight: 1
    callback valid: N/A
  - operation: ipcache_purgelru
    secs to next execution: 9.428
    weight: 1
    callback valid: N/A
  - operation: statAvgTick
    secs to next execution: 49.413
    weight: 1
    callback valid: N/A
  - operation: peerClearRR
    secs to next execution: 289.391
    weight: 0
    callback valid: N/A
  - operation: peerRefreshDNS
    secs to next execution: 2973.609
    weight: 1
    callback valid: N/A

@kinkie
Copy link
Contributor Author

kinkie commented Apr 25, 2024

This PR has not adopted spaces() from PR #1735 yet

@kinkie kinkie added the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Apr 25, 2024
@kinkie kinkie marked this pull request as draft April 25, 2024 09:41
@kinkie kinkie marked this pull request as ready for review April 25, 2024 09:41
@yadij yadij changed the title Emit YAML for events cahcemgr report Emit YAML for events cachemgr report Apr 25, 2024
@rousskov rousskov self-requested a review April 26, 2024 13:15
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Please settle spaces() issue before posting any new YAML PRs or requesting additional reviews on the already posted ones. Squid currently lacks the necessary foundation for new YAML PRs, and we have enough information to provide that foundation.

src/event.cc Outdated
else
yaml << "no\n";
} else {
yaml << "N/A\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just like in other reports you have already converted, please do not report fields that are not applicable to an event. It was difficult or even wrong to skip a field when this was a table. This PR is no longer using a table, removing that obstacle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am obviously biased, for me spaces() can go in as-is. What do I need to do to convince you that it's good enough? Right now I have a backlog of completed changes; using a local variable is to me a way to minimize the changes to redo over once spaces() lands, please advise if you have any idea on how I can keep myself unblocked otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like in other reports you have already converted, please do not report fields that are not applicable to an event. It was difficult or even wrong to skip a field when this was a table. This PR is no longer using a table, removing that obstacle.

Removed in 553a97b

Copy link
Contributor

Choose a reason for hiding this comment

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

for me spaces() can go in as-is. What do I need to do to convince you that it's good enough?

This PR is not the right place to discuss problems with spaces().

Right now I have a backlog of completed changes

Please do not interpret my commends below as an implicit agreement with the above "completed" characterization of any pending changes.

Creating more and more non-urgent PRs (or code branches, etc.) that heavily depend on some pending issue resolution will naturally grow that backlog, especially when the exact resolution of that issue is unknown and may impact all those changes quite a bit!

BTW, creating lots of PRs that fix a duplicated issue in a new/untested way is likely to face very similar problems.

I cannot stop new PRs, but I do not understand why keep creating all that extra work/overhead.

please advise if you have any idea on how I can keep myself unblocked otherwise

My recommendation is to work towards spaces() resolution. If you have free time after that, pick a non-YAML problem that does not depend on spaces() resolution.

@kinkie
Copy link
Contributor Author

kinkie commented Apr 29, 2024

This is ready to review, using a local indent SBuf instead of spaces() while waiting for spaces to land. But apart from that bit of rework, I don't have any outstanding requests for change

@kinkie kinkie added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 29, 2024
@kinkie kinkie requested a review from rousskov April 29, 2024 16:28
@rousskov rousskov removed their request for review April 29, 2024 20:06
@rousskov rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 29, 2024
src/event.cc Outdated
(e->arg && e->cbdata) ? cbdataReferenceValid(e->arg) ? "yes" : "no" : "N/A");
yaml << header <<
indent << "- operation: " << e->name << '\n' <<
indent << indent << "secs to next execution: " << std::setprecision(3) << std::fixed << (e->when? e->when - current_dtime : 0) << '\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid unnecessary abbreviations, especially non-standard ones.

Suggested change
indent << indent << "secs to next execution: " << std::setprecision(3) << std::fixed << (e->when? e->when - current_dtime : 0) << '\n';
indent << indent << "seconds to next execution: " << std::setprecision(3) << std::fixed << (e->when ? e->when - current_dtime : 0) << '\n';

Many of these events wait for hours. I recommend removing "seconds" from the item key so that we can easily change it later, when we improve support for time units:

Suggested change
indent << indent << "secs to next execution: " << std::setprecision(3) << std::fixed << (e->when? e->when - current_dtime : 0) << '\n';
indent << indent << "next execution in: " << std::setprecision(3) << std::fixed << (e->when ? e->when - current_dtime : 0) << " seconds\n";

... or something like that.

Finally, many of these events do not repeat so "next" is quite misleading here. I would rephrase further to avoid the (often wrong) implication (e.g., "runs in ... seconds" or "scheduled to run in ... seconds").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... seconds, from the YAML schema perspective, turns the field from a number into a string, requiring additional work for automations.
In fact, in an ideal world I would specify the unit in a comment for the benefit of humans only: specifying it in the key means that if we change time unit the key changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can use an inline comment: next execution in: ... # seconds

Copy link
Contributor

Choose a reason for hiding this comment

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

... seconds, from the YAML schema perspective, turns the field from a number into a string, requiring additional work for automations.

I strongly disagree with the implication that consuming unitless numbers (or dealing with field key changes) is overall better "for automations" than consuming numbers with units (because the latter requires "additional work"). And since lots of Squid report fields do use units of various kinds, and using units often makes reports more readable for humans, that "additional work" will most likely be required anyway and will be amortized across various consumed reports.

In fact, in an ideal world I would specify the unit in a comment for the benefit of humans only:

Our ideals differ: Ideally, as a consumer application developer, I would want units to be correctly processed by my code rather than hidden from it in a YAML comment, especially if units change based on the reported value (to assist human readers).

specifying it in the key means that if we change time unit the key changes

Agreed. The unit should not go into the field key, especially when the unit should change based on the reported value (for human benefit). That is why I explicitly recommended removing PR-added seconds from the field key.

src/event.cc Outdated Show resolved Hide resolved
src/event.cc Outdated Show resolved Hide resolved
kinkie and others added 2 commits April 29, 2024 22:35
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Apr 30, 2024
@kinkie kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels S-waiting-for-author author action is expected (and usually required) S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants