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

Improve YAML compliance of mgr:jobs report #1794

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

Improve YAML compliance of mgr:jobs report #1794

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d648cfd
turn output into yaml and simplify
kinkie Mar 20, 2024
9cba29b
Use proper yaml booleans for started field
kinkie Apr 25, 2024
d0ff4ab
Revert changes to BodyPipe
kinkie Apr 29, 2024
5e57705
Move AsyncJob::status spacing to callers
kinkie Apr 29, 2024
1d20c03
Ensure callers of AsyncJob::status() add space
kinkie Apr 29, 2024
ae5f2bf
Reformat HappyConnOpener
kinkie Apr 30, 2024
82508b1
Revert Adaptation::Ecap::ServiceRep::status
kinkie Apr 30, 2024
b790a1e
Do not use std::string
kinkie Apr 30, 2024
31865ca
quote job type
kinkie Apr 30, 2024
f44573b
quote status
kinkie Apr 30, 2024
a45698e
fix spacing
kinkie Apr 30, 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
4 changes: 0 additions & 4 deletions src/BodyPipe.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,6 @@ const char *BodyPipe::status() const
static MemBuf outputBuffer;
outputBuffer.reset();

outputBuffer.append(" [", 2);

outputBuffer.appendf("%" PRIu64 "<=%" PRIu64, theGetSize, thePutSize);
if (theBodySize >= 0)
outputBuffer.appendf("<=%" PRId64, theBodySize);
Expand All @@ -471,8 +469,6 @@ const char *BodyPipe::status() const
if (isCheckedOut)
outputBuffer.append(" L", 2); // Locked

outputBuffer.append("]", 1);

outputBuffer.terminate();

return outputBuffer.content();
Expand Down
6 changes: 2 additions & 4 deletions src/HappyConnOpener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,17 +437,15 @@ HappyConnOpener::status() const

SBufStream os(buf);

os.write(" [", 2);
if (stopReason)
os << "Stopped:" << stopReason;
if (prime)
os << "prime:" << prime;
os << " prime:" << prime;
if (spare)
os << "spare:" << spare;
os << " spare:" << spare;
if (n_tries)
os << " tries:" << n_tries;
os << " dst:" << *destinations;
os << ' ' << id << ']';

buf = os.buf();
return buf.c_str();
Expand Down
2 changes: 0 additions & 2 deletions src/adaptation/ecap/ServiceRep.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ const char *Adaptation::Ecap::ServiceRep::status() const
static MemBuf buf;

buf.reset();
buf.append("[", 1);

if (up())
buf.append("up", 2);
Expand All @@ -294,7 +293,6 @@ const char *Adaptation::Ecap::ServiceRep::status() const
if (detached())
buf.append(",detached", 9);

buf.append("]", 1);
buf.terminate();

return buf.content();
Expand Down
4 changes: 0 additions & 4 deletions src/adaptation/ecap/XactionRep.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,8 +702,6 @@ Adaptation::Ecap::XactionRep::status() const
static MemBuf buf;
buf.reset();

buf.append(" [", 2);

if (makingVb)
buf.appendf("M%d", static_cast<int>(makingVb));

Expand Down Expand Up @@ -732,8 +730,6 @@ Adaptation::Ecap::XactionRep::status() const
buf.append(" A?", 3);
}

buf.appendf(" %s%u]", id.prefix(), id.value);

buf.terminate();

return buf.content();
Expand Down
5 changes: 2 additions & 3 deletions src/adaptation/icap/ServiceRep.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,12 +674,12 @@ Adaptation::Icap::ServiceRep::makeXactLauncher(Http::Message *virgin,
}

// returns a temporary string depicting service status, for debugging
const char *Adaptation::Icap::ServiceRep::status() const
const char *
Adaptation::Icap::ServiceRep::status() const
{
static MemBuf buf;

buf.reset();
buf.append("[", 1);

if (up())
buf.append("up", 2);
Expand Down Expand Up @@ -708,7 +708,6 @@ const char *Adaptation::Icap::ServiceRep::status() const
if (const int failures = theSessionFailures.remembered())
buf.appendf(",fail%d", failures);

buf.append("]", 1);
buf.terminate();

return buf.content();
Expand Down
2 changes: 0 additions & 2 deletions src/adaptation/icap/Xaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,11 +637,9 @@ const char *Adaptation::Icap::Xaction::status() const
{
static MemBuf buf;
buf.reset();
buf.append(" [", 2);
fillPendingStatus(buf);
buf.append("/", 1);
fillDoneStatus(buf);
buf.appendf(" %s%u]", id.prefix(), id.value);
buf.terminate();

return buf.content();
Expand Down
23 changes: 10 additions & 13 deletions src/base/AsyncJob.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/TextException.h"
#include "cbdata.h"
#include "mem/PoolingAllocator.h"
#include "MemBuf.h"
#include "mgr/Registration.h"
#include "Store.h"

Expand Down Expand Up @@ -179,16 +178,15 @@ void AsyncJob::callEnd()
}

// returns a temporary string depicting transaction status, for debugging
const char *AsyncJob::status() const
const char *
AsyncJob::status() const
{
static MemBuf buf;
buf.reset();

buf.append(" [", 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

A PR changing many (all?) AsyncJob::status() implementations should fix that API to allow for compounded base+kid+gradkid status reports. We probably want to split that API into two because the current status() is used in many debugs() messages where output requirements are rather different (this PR probably breaks some of those use cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible that it breaks some use cases by missing out on spacing, I've fixed what I could find.
I'm not convinced that it's necessary to have two different functions to report status, in the end the only difference is formatting, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

the only difference is formatting, isn't it?

No, it is not, not for any reasonable definition of "formatting" in this context.

debugs() status():

  • target audience: developers (that can easily decipher cryptic output by reading Squid code)
  • should be very compact -- often added to many (already long) debugs() lines
  • should be fairly fast -- used often when debugging at level 3+

job state reported inside a mgr:jobs report:

  • target audience: admins (humans that do not read Squid code) and their tools
  • output size is not a concern; new lines and such are very useful for readability
  • output speed is not a significant concern

Copy link
Contributor

Choose a reason for hiding this comment

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

@yadij requested your review on: Improve YAML compliance of mgr:jobs report.

I am not sure why my re-review was requested before any attempt to address my change request. FWIW, until this change request is addressed, polishing minor PR changes is likely to result in quite a few wasted cycles because at least some of that polishing will need to be redone after this change request is addressed.

If there is interest, I can suggest a way to reshape this PR to avoid modifying existing status() implementations (which triggers status() API change requests and other complications) while still providing YAML-compliant mgr:jobs reports.

if (stopReason != nullptr) {
buf.appendf("Stopped, reason:%s", stopReason);
buf.appendf("Stopped, reason: %s", stopReason);
}
buf.appendf(" %s%u]", id.prefix(), id.value);
buf.terminate();

return buf.content();
Expand All @@ -199,15 +197,14 @@ AsyncJob::ReportAllJobs(StoreEntry *e)
{
PackableStream os(*e);
// this loop uses YAML syntax, but AsyncJob::status() still needs to be adjusted to use YAML
const char *indent = " ";
const std::string indent(" ");
for (const auto job: AllJobs()) {
os << indent << job->id << ":\n";
os << indent << indent << "type: '" << job->typeName << "'\n";
os << indent << indent << "status:" << job->status() << '\n';
if (!job->started_)
os << indent << indent << "started: false\n";
if (job->stopReason)
os << indent << indent << "stopped: '" << job->stopReason << "'\n";
os << job->id << ":\n";
os << indent << "type: " << job->typeName << '\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to review discussions that led to this recently added code. Did we make a mistake when deciding that typeName should be quoted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In yaml, quoting is optional unless there's a risk of
misinterpreting the element's type. I removed the quotes
as I don't think there is such a risk, but I can just add it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back

const char *status = job->status();
if (status && strlen(status))
os << indent << "status: " << job->status() << "\n";
os << indent << "started: " << (job->started_ ? "Yes": "No" ) << '\n';
}
}

Expand Down
3 changes: 1 addition & 2 deletions src/clients/HttpTunneler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ Http::Tunneler::status() const

// TODO: redesign AsyncJob::status() API to avoid
// id and stop reason reporting duplication.
buf.append(" [state:", 8);
buf.append("state:", 8);
if (requestWritten) buf.append("w", 1); // request sent
if (tunnelEstablished) buf.append("t", 1); // tunnel established
if (!callback) buf.append("x", 1); // caller informed
Expand All @@ -449,7 +449,6 @@ Http::Tunneler::status() const
}
if (connection != nullptr)
buf.appendf(" FD %d", connection->fd);
buf.appendf(" %s%u]", id.prefix(), id.value);
buf.terminate();

return buf.content();
Expand Down
2 changes: 1 addition & 1 deletion src/comm/TcpAcceptor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ const char *
Comm::TcpAcceptor::status() const
{
if (conn == nullptr)
return "[nil connection]";
return "nil connection";

static char ipbuf[MAX_IPSTRLEN] = {'\0'};
if (ipbuf[0] == '\0')
Expand Down
2 changes: 1 addition & 1 deletion src/ipc/Inquirer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ Ipc::Inquirer::status() const
{
static MemBuf buf;
buf.reset();
buf.appendf(" [requestId %u]", request->requestId.index());
buf.appendf("requestId %u", request->requestId.index());
buf.terminate();
return buf.content();
}
Expand Down
2 changes: 0 additions & 2 deletions src/security/PeerConnector.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,14 +582,12 @@ Security::PeerConnector::status() const

// TODO: redesign AsyncJob::status() API to avoid this
// id and stop reason reporting duplication.
buf.append(" [", 2);
if (stopReason != nullptr) {
buf.append("Stopped, reason:", 16);
buf.appendf("%s",stopReason);
}
if (Comm::IsConnOpen(serverConn))
buf.appendf(" FD %d", serverConn->fd);
buf.appendf(" %s%u]", id.prefix(), id.value);
buf.terminate();

return buf.content();
Expand Down
Loading