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

Emit YAML for events cachemgr report #1792

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

Emit YAML for events cachemgr report #1792

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8558e73
Emit YAML for events cahcemgr report
kinkie Apr 25, 2024
68b1eb1
Source maintenance
kinkie Apr 25, 2024
b369c03
Fix "scheduled events" header
kinkie Apr 25, 2024
553a97b
use SBuf for indent; do not report missing callbacks
kinkie Apr 29, 2024
3375bbe
Do not pollute stream flags
kinkie Apr 29, 2024
cacc9df
Apply suggestions from code review
kinkie Apr 29, 2024
4fa6429
use inline comment for time precision
kinkie Apr 29, 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/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2749,7 +2749,6 @@ tests_testEvent_SOURCES = \
tests/testEvent.cc
nodist_tests_testEvent_SOURCES = \
MemBuf.cc \
tests/stub_SBuf.cc \
tests/stub_cache_manager.cc \
tests/stub_cbdata.cc \
tests/stub_debug.cc \
Expand All @@ -2758,6 +2757,7 @@ nodist_tests_testEvent_SOURCES = \
tests/stub_libtime.cc \
tests/stub_tools.cc
tests_testEvent_LDADD = \
sbuf/libsbuf.la \
base/libbase.la \
$(LIBCPPUNIT_LIBS) \
$(COMPAT_LIB) \
Expand Down
32 changes: 21 additions & 11 deletions src/event.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
/* DEBUG: section 41 Event Processing */

#include "squid.h"
#include "base/IoManip.h"
#include "base/PackableStream.h"
#include "base/Random.h"
#include "event.h"
#include "mgr/Registration.h"
Expand Down Expand Up @@ -138,7 +140,8 @@ eventInit(void)
static void
eventDump(StoreEntry * sentry)
{
EventScheduler::GetInstance()->dump(sentry);
PackableStream yaml(*sentry);
EventScheduler::GetInstance()->dump(yaml);
}

int
Expand Down Expand Up @@ -258,21 +261,28 @@ EventScheduler::clean()
}

void
EventScheduler::dump(Packable *out)
EventScheduler::dump(std::ostream &yaml)
{
static const SBuf indent(" ",2);
kinkie marked this conversation as resolved.
Show resolved Hide resolved

if (last_event_ran)
out->appendf("Last event to run: %s\n\n", last_event_ran);
yaml << "last event to run: " << last_event_ran << "\n";
kinkie marked this conversation as resolved.
Show resolved Hide resolved

out->appendf("%-25s\t%-15s\t%s\t%s\n",
"Operation",
"Next Execution",
"Weight",
"Callback Valid?");
const auto savedFlags = yaml.flags();

AtMostOnce header("scheduled events:\n");
for (auto *e = tasks; e; e = e->next) {
out->appendf("%-25s\t%0.3f sec\t%5d\t %s\n",
e->name, (e->when ? e->when - current_dtime : 0), e->weight,
(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.


yaml.flags(savedFlags);
yaml << indent << indent << "weight: " << e->weight << '\n';

if (e->arg && e->cbdata) {
yaml << indent << indent << "callback valid: " <<
(cbdataReferenceValid(e->arg) ? "Yes" : "No") << '\n';
}
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
#define SQUID_SRC_EVENT_H

#include "AsyncEngine.h"
#include "base/Packable.h"
#include "mem/forward.h"

#include <iosfwd>

/* event scheduling facilities - run a callback after a given time period. */

typedef void EVH(void *);
Expand Down Expand Up @@ -55,7 +56,7 @@ class EventScheduler : public AsyncEngine
/* either EVENT_IDLE or milliseconds remaining until the next event */
int timeRemaining() const;
/* cache manager output for the event queue */
void dump(Packable *);
void dump(std::ostream &);
/* find a scheduled event */
bool find(EVH * func, void * arg);
/* schedule a callback function to run in when seconds */
Expand Down
2 changes: 1 addition & 1 deletion src/tests/stub_event.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ EventScheduler::~EventScheduler() STUB
void EventScheduler::cancel(EVH *, void *) STUB
int EventScheduler::timeRemaining() const STUB_RETVAL(1)
void EventScheduler::clean() STUB
void EventScheduler::dump(Packable *) STUB
void EventScheduler::dump(std::ostream &) STUB
bool EventScheduler::find(EVH *, void *) STUB_RETVAL(false)
void EventScheduler::schedule(const char *, EVH *, void *, double, int, bool) STUB
int EventScheduler::checkEvents(int) STUB_RETVAL(-1)
Expand Down
48 changes: 15 additions & 33 deletions src/tests/testEvent.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include "MemBuf.h"
#include "unitTestMain.h"

#include <sstream>

/*
* test the event module.
*/
Expand Down Expand Up @@ -83,14 +85,6 @@ TestEvent::testDump()
EventScheduler scheduler;
CalledEvent event;
CalledEvent event2;
const char *expected = "Last event to run: last event\n"
"\n"
"Operation \tNext Execution \tWeight\tCallback Valid?\n"
"test event \t0.000 sec\t 0\t N/A\n"
"test event2 \t0.000 sec\t 0\t N/A\n";
MemBuf expect;
expect.init();
expect.append(expected, strlen(expected));

scheduler.schedule("last event", CalledEvent::Handler, &event, 0, 0, false);

Expand All @@ -100,31 +94,19 @@ TestEvent::testDump()
scheduler.schedule("test event", CalledEvent::Handler, &event, 0, 0, false);
scheduler.schedule("test event2", CalledEvent::Handler, &event2, 0, 0, false);

MemBuf result;
result.init();
scheduler.dump(&result);

/* loop over the strings, showing exactly where they differ (if at all) */
printf("Actual Text:\n");
/* TODO: these should really be just [] lookups, but String doesn't have those here yet. */
for (size_t i = 0; i < size_t(result.contentSize()); ++i) {
CPPUNIT_ASSERT(expect.content()[i]);
CPPUNIT_ASSERT(result.content()[i]);

/* slight hack to make special chars visible */
switch (result.content()[i]) {
case '\t':
printf("\\t");
break;
default:
printf("%c", result.content()[i]);
}
/* make this an int comparison, so that we can see the ASCII code at failure */
CPPUNIT_ASSERT_EQUAL(int(expect.content()[i]), int(result.content()[i]));
}
printf("\n");
CPPUNIT_ASSERT_EQUAL(expect.contentSize(), result.contentSize());
CPPUNIT_ASSERT(strcmp(expect.content(), result.content()) == 0);
std::ostringstream os;
scheduler.dump(os);

const std::string expected("last event to run: last event\n"
"scheduled events:\n"
" - operation: test event\n"
" secs to next execution: 0.000\n"
" weight: 0\n"
" - operation: test event2\n"
" secs to next execution: 0.000\n"
" weight: 0\n");

CPPUNIT_ASSERT_EQUAL(expected, os.str());
}

/* submit two callbacks, and find the right one.
Expand Down
Loading