Improve YAML compliance of mgr:jobs report #1794

kinkie · 2024-04-25T14:01:52Z

Improve the report's schema and formatting

kinkie · 2024-04-25T14:11:09Z

Current master output:

    job5:
        type: 'Http1::Server'
        status: [ job5]
    job6:
        type: 'ClientHttpRequest'
        status: [ job6]
        started: false
    job4:
        type: 'Comm::TcpAcceptor'
        status: FD 15, [::] [ job4]

notice the unnecessary indent, and confusing array for most "status" fields

Proposed output:

job6:
  type: ClientHttpRequest
  started: No
job5:
  type: Http1::Server
  started: Yes
job4:
  type: Comm::TcpAcceptor
  status:  FD 15, [::]
  started: Yes

rousskov

This PR requires a lot more work.

src/base/AsyncJob.cc

src/BodyPipe.cc

src/HappyConnOpener.cc

rousskov · 2024-04-26T20:54:31Z

src/base/AsyncJob.cc

-        if (job->stopReason)
-            os << indent << indent << "stopped: '" << job->stopReason << "'\n";
+        os << job->id << ":\n";
+        os << indent << "type: " << job->typeName << '\n';


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?

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

rousskov · 2024-04-26T20:59:14Z

src/base/AsyncJob.cc

 {
    static MemBuf buf;
    buf.reset();

-    buf.append(" [", 2);


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).

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?

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

@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.

kinkie · 2024-04-30T15:22:09Z

I think all outstanding requests for change are addressed

yadij

just some polish now.

yadij · 2024-06-05T18:59:41Z

src/BodyPipe.cc

@@ -133,12 +133,12 @@ BodyPipe::BodyPipe(Producer *aProducer): theBodySize(-1),
    // TODO: teach MemBuf to start with zero minSize
    // TODO: limit maxSize by theBodySize, when known?
    theBuf.init(2*1024, MaxCapacity);
-    debugs(91,7, "created BodyPipe" << status());
+    debugs(91,7, "created BodyPipe " << status());


Please take advantage of the touch to make this produce the log pattern detected by the scripts/find-alive.pl:

Suggested change

debugs(91,7, "created BodyPipe " << status());

debugs(91,7, "construct, this=" << static_cast<void*>(this) << ", " << status());

yadij · 2024-06-05T19:02:34Z

src/BodyPipe.cc

 }

 BodyPipe::~BodyPipe()
 {
-    debugs(91,7, "destroying BodyPipe" << status());
+    debugs(91,7, "destroying BodyPipe " << status());


Please take advantage of the touch to make this produce the log pattern detected by the scripts/find-alive.pl:

Suggested change

debugs(91,7, "destroying BodyPipe " << status());

debugs(91,7, "destruct, this=" << static_cast<void*>(this) << ", " << status());

yadij · 2024-06-05T19:24:13Z

src/adaptation/icap/ModXact.cc

@@ -708,7 +708,7 @@ void Adaptation::Icap::ModXact::bypassFailure()
        reuseConnection = false; // be conservative
        cancelRead(); // may not work; and we cannot stop connecting either
        if (!doneWithIo())
-            debugs(93, 7, "Warning: bypass failed to stop I/O" << status());
+            debugs(93, 7, "Warning: bypass failed to stop I/O " << status());


Suggested change

debugs(93, 7, "Warning: bypass failed to stop I/O " << status());

debugs(93, 7, "WARNING: bypass failed to stop I/O " << status());

yadij · 2024-06-05T19:25:26Z

src/adaptation/icap/ModXact.cc

@@ -1292,7 +1292,7 @@ Adaptation::Icap::ModXact::~ModXact()
 // internal cleanup
 void Adaptation::Icap::ModXact::swanSong()
 {
-    debugs(93, 5, "swan sings" << status());
+    debugs(93, 5, "swan sings " << status());


method name "swanSong" makes the content text redundant.

Suggested change

debugs(93, 5, "swan sings " << status());

debugs(93, 5, status());

yadij · 2024-06-05T19:29:29Z

src/base/AsyncJob.cc

        if (job->stopReason)
-            os << indent << indent << "stopped: '" << job->stopReason << "'\n";
+            os << indent << "stopped: \'" << job->stopReason << "'\n";


single quote should not need to be escaped within double-quoted string:

Suggested change

os << indent << "stopped: \'" << job->stopReason << "'\n";

os << indent << "stopped: '" << job->stopReason << "'\n";

rousskov · 2024-06-06T22:08:01Z

src/base/AsyncJob.cc

 {
    static MemBuf buf;
    buf.reset();

-    buf.append(" [", 2);


@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.

kinkie added 2 commits April 25, 2024 14:58

turn output into yaml and simplify

d648cfd

Use proper yaml booleans for started field

9cba29b

rousskov requested changes Apr 26, 2024

View reviewed changes

kinkie added 9 commits April 29, 2024 17:37

Revert changes to BodyPipe

d0ff4ab

Move AsyncJob::status spacing to callers

5e57705

Ensure callers of AsyncJob::status() add space

1d20c03

Reformat HappyConnOpener

ae5f2bf

Revert Adaptation::Ecap::ServiceRep::status

82508b1

Do not use std::string

b790a1e

quote job type

31865ca

quote status

f44573b

fix spacing

a45698e

kinkie requested review from rousskov and yadij April 30, 2024 15:22

kinkie added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 30, 2024

rousskov removed their request for review April 30, 2024 20:09

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

yadij requested changes Jun 5, 2024

View reviewed changes

yadij added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jun 5, 2024

yadij requested a review from rousskov June 5, 2024 19:34

rousskov requested changes Jun 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve YAML compliance of mgr:jobs report #1794

Improve YAML compliance of mgr:jobs report #1794

kinkie commented Apr 25, 2024

kinkie commented Apr 25, 2024

rousskov left a comment

rousskov Apr 26, 2024

kinkie Apr 30, 2024

kinkie Apr 30, 2024

rousskov Apr 26, 2024

kinkie Apr 30, 2024

rousskov Apr 30, 2024

rousskov Jun 6, 2024

kinkie commented Apr 30, 2024

yadij left a comment

yadij Jun 5, 2024

yadij Jun 5, 2024

yadij Jun 5, 2024

yadij Jun 5, 2024

yadij Jun 5, 2024

rousskov Jun 6, 2024

	debugs(91,7, "created BodyPipe " << status());
	debugs(91,7, "construct, this=" << static_cast<void*>(this) << ", " << status());

	debugs(91,7, "destroying BodyPipe " << status());
	debugs(91,7, "destruct, this=" << static_cast<void*>(this) << ", " << status());

	debugs(93, 7, "Warning: bypass failed to stop I/O " << status());
	debugs(93, 7, "WARNING: bypass failed to stop I/O " << status());

	debugs(93, 5, "swan sings " << status());
	debugs(93, 5, status());

	os << indent << "stopped: \'" << job->stopReason << "'\n";
	os << indent << "stopped: '" << job->stopReason << "'\n";

Improve YAML compliance of mgr:jobs report #1794

Are you sure you want to change the base?

Improve YAML compliance of mgr:jobs report #1794

Conversation

kinkie commented Apr 25, 2024

kinkie commented Apr 25, 2024

rousskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinkie commented Apr 30, 2024

yadij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment