Emit valid YAML for cachemgr:server_list #1735

kinkie · 2024-03-13T15:56:03Z

Use PackableStream for CachePeer::reportStatistics and
update output format to well-formed YAML

kinkie · 2024-03-13T15:57:23Z

Runtime test:

master:

Sibling    : localhost
Host       : localhost/13128/0
Flags      : default connection-auth=auto tls-disable
Address[0] : 127.0.0.1
Status     : Down
FETCHES    : 0
OPEN CONNS : 0
AVG RTT    : 0 msec
LAST QUERY : 1710344842 seconds ago
LAST REPLY : none received
PINGS SENT :        0
PINGS ACKED:        0   0%
IGNORED    :        0   0%
Histogram of PINGS ACKED:
Last failed connect() at: 13/Mar/2024:15:47:16 +0000
keep-alive ratio: 0%

PR:

Sibling    : localhost
Host       : localhost/13128/0
Flags      : default connection-auth=auto tls-disable
Address[0] : 127.0.0.1
Status     : Down
FETCHES    :        0
OPEN CONNS :        0
AVG RTT    :        0 msec
LAST QUERY : none sent
LAST REPLY : none received
PINGS SENT :        0
PINGS ACKED:        0   0%
IGNORED    :        0   0%
Histogram of PINGS ACKED:
Last failed connect() at: 13/Mar/2024:15:54:31 +0000
keep-alive ratio: 0%

Delta:

--- /tmp/before	2024-03-13 22:54:54
+++ /tmp/after	2024-03-13 22:54:53
@@ -3,14 +3,14 @@
 Flags      : default connection-auth=auto tls-disable
 Address[0] : 127.0.0.1
 Status     : Down
-FETCHES    : 0
-OPEN CONNS : 0
-AVG RTT    : 0 msec
-LAST QUERY : 1710344842 seconds ago
+FETCHES    :        0
+OPEN CONNS :        0
+AVG RTT    :        0 msec
+LAST QUERY : none sent
 LAST REPLY : none received
 PINGS SENT :        0
 PINGS ACKED:        0   0%
 IGNORED    :        0   0%
 Histogram of PINGS ACKED:
-Last failed connect() at: 13/Mar/2024:15:47:16 +0000
+Last failed connect() at: 13/Mar/2024:15:54:31 +0000
 keep-alive ratio: 0%

rousskov

Thank you for posting sample output and the diff!

src/neighbors.cc

Co-authored-by: Alex Rousskov <[email protected]>

kinkie · 2024-03-15T05:31:53Z

Current sample output:

Name       : 127.0.0.1
Type       : Sibling
Addresses  : 1
Host       : 127.0.0.1/13129/0
Flags      : default connection-auth=auto tls-disable
Address[0] : 127.0.0.1
Status     : Down
Fetches    : 0
Open conns : 0
Avg rtt    : 0 msec
Pings sent : 0
Pings acked: 0 0%
Ignored    : 0 0%
Histogram of pings acked:
Last failed connect() at: 15/Mar/2024:05:27:58 +0000
keep-alive ratio: 0%

src/CachePeer.cc

kinkie · 2024-04-13T11:01:39Z

Current output format:

cache_peers number: 2
cache_peers:
  - name: localhost
    type: sibling
    http address: localhost:3129
    options: default tls-disable
    addresses: [ "[::1]" ]
    status: Down
    fetches: 0
    open connections: 0
    average RTT: 0 msec
    pings sent: 0
    pings acked: 0 0%
    replies ignored: 0 0%
    last failed connection at: 13/Apr/2024:18:01:11 +0700
    keep-alive ratio: 0%
  - name: localhost.localdomain
    type: sibling
    http address: localhost.localdomain:3130
    options: default tls-disable
    addresses: [  ]
    status: Down
    fetches: 0
    open connections: 0
    average RTT: 0 msec
    pings sent: 0
    pings acked: 0 0%
    replies ignored: 0 0%
    keep-alive ratio: 0%

kinkie · 2024-04-13T12:33:50Z

Possible thing to investigate: whether to have this action return application/yaml content type

yadij · 2024-04-13T13:08:40Z

FWIW, @rousskov vetoed the change to allow reports to have their own Content-Type.

rousskov · 2024-04-13T19:48:43Z

FWIW, @rousskov vetoed the change to allow reports to have their own Content-Type.

FWIW, I did not.

rousskov · 2024-04-13T20:49:04Z

Possible thing to investigate: whether to have this action return application/yaml content type

That would be nice, but we cannot do that easily right now because this action (as a whole) does not return YAML in SMP Squids. To get to YAML, we need to fix¹ how SMP Squids "bracket" from-kid output, replacing current by kidN { ... } by kidN "brackets" with some YAML-compatible variation. Here is one option that limits bracketing modifications and mostly preserves their current advantages, but there may be better options:

by kid1: {
...
} # by kid1

by kid2: {
...
} # by kid2

Bracketing changes do not belong to this PR.

More fixes may be necessary because each kid-specific blob may need to be adjusted as well to keep aggregated output valid; I have not investigated what exact changes, if any, are needed there. Ideally, we should check/decide before this PR goes in so that we do not have to modify non-SMP output twice. However, I cannot insist on that precondition. ↩

rousskov

Great progress, thank you! This is an incomplete review that should still allow you to move forward. I will come back to this PR later.

src/CachePeer.cc

rousskov · 2024-04-13T21:11:01Z

src/neighbors.cc

 if (!peers) {
- storeAppendPrintf(sentry, "There are no neighbors installed.\n");
+ os << "There are no neighbors installed.\n";
 return;
 }


the output format is now: ...

Looks good to me overall, thank you!

I would prefer "cache_peer directives:" to "cache_peers number:", but that is subjective, and I do not insist on that change.

If that does not break YAML, I would probably add an empty line between peers to improve readability, but that is rather subjective and I do not insist on that change.

src/CachePeer.cc

rousskov · 2024-04-13T21:22:08Z

src/sbuf/Spaces.h

+
+#include "sbuf/SBuf.h"
+
+const SBuf spaces(size_t count);


FWIW, I will try to find the time to check whether this can become a constexpr (which may even improve some code in callers). If I succeed, I will fix the return type (extra/unwanted const), the lack of documentation for this new global function, and NoNewGlobals implementation violation at that time.

Based on our conversation, I'd hold off on this experiment; spaces (or its successor) will likely need to hold some state

Sounds good. The current spaces() implementation is not something we can merge, but waiting for the decision regarding indentation/YAML formatting future is fine with me.

kinkie · 2024-04-17T08:48:31Z

Current output:

cache_peers number: 3
cache_peers:
  - name: "localhost"
    type: parent
    HTTP address: localhost:3129
    ICP address: localhost:3129
    options:  default tls-disable
    addresses: [ "[::1]" ]
    status: Down
    fetches: 0
    open connections: 0
    average RTT: 0 msec
    pings sent: 0
    pings acked: 0 0%
    replies ignored: 0 0%
    last failed connection at: 17/Apr/2024:09:46:22 +0100
    keep-alive ratio: 0%
  - name: "127.0.0.1"
    type: parent
    HTTP address: 127.0.0.1:3130
    ICP address: 127.0.0.1:3130
    options:  default tls-disable
    addresses: [ "127.0.0.1" ]
    status: Down
    fetches: 0
    open connections: 0
    average RTT: 0 msec
    pings sent: 0
    pings acked: 0 0%
    replies ignored: 0 0%
    last failed connection at: 17/Apr/2024:09:46:22 +0100
    keep-alive ratio: 0%
  - name: "::1"
    type: parent
    HTTP address: ::1:3131
    ICP address: ::1:3131
    options:  default tls-disable
    addresses: [ "[::1]" ]
    status: Down
    fetches: 0
    open connections: 0
    average RTT: 0 msec
    pings sent: 0
    pings acked: 0 0%
    replies ignored: 0 0%
    last failed connection at: 17/Apr/2024:09:46:22 +0100
    keep-alive ratio: 0%

kinkie · 2024-04-17T08:49:48Z

Possible thing to investigate: whether to have this action return application/yaml content type

That would be nice, but we cannot do that easily right now because this action (as a whole) does not return YAML in SMP Squids. To get to YAML, we need to fix1 how SMP Squids "bracket" from-kid output, replacing current by kidN { ... } by kidN "brackets" with some YAML-compatible variation. Here is one option that limits bracketing modifications and mostly preserves their current advantages, but there may be better options:
by kid1: {
...
} # by kid1

by kid2: {
...
} # by kid2
Bracketing changes do not belong to this PR.

Footnotes

More fixes may be necessary because each kid-specific blob may need to be adjusted as well to keep aggregated output valid; I have not investigated what exact changes, if any, are needed there. Ideally, we should check/decide before this PR goes in so that we do not have to modify non-SMP output twice. However, I cannot insist on that precondition. ↩

A simplistic (v1) solution is at In PR #1784

rousskov

Another incomplete review to help you make progress.

rousskov · 2024-04-18T15:29:33Z

src/CachePeer.cc

+ yaml <<
+ spaces(2) << "- name: \"" << name << "\"\n" <<
+ spaces(4) << "type: " << typeString() << '\n' <<
+ spaces(4) << "HTTP address: " << host << ':' << http_port << '\n';


host can be an IP address. I assume host is bracketed when it is an IPv6 address (if not, that is another problem to fix in this or prerequisite PR because we print :port after the host value).

We quote() IP addresses in the addresses field below. I am guessing that we do that because : and/or [ characters in an IPv6 address (if we are bracketing those addresses) must be quoted to remain YAML-compliant. Please adjust (this code and/or addresses printing code) to be consistent and YAML-compliant.

rousskov · 2024-04-18T15:33:10Z

src/CachePeer.cc

+
+#if USE_CACHE_DIGESTS
+ if (digest_url)
+ os << " digest-url=" << digest_url;


Do we need to quote this URL to be YAML-compliant? If yes, I hope we can be both YAML- and squid.conf-compliant because this output is used for mgr:config reporting as well!

If you have a reference to YAML quoting rules, please share it.

rousskov · 2024-04-18T15:41:36Z

src/enums.h

@@ -20,7 +20,7 @@ enum fd_type {
 };

 typedef enum {
- PEER_NONE,
+ PEER_NONE = 0,


C++ guarantees that, right? If yes, we do not want to start adjusting various enums to duplicate that guarantee, especially in a PR focusing on YAML conversion.

Suggested change

PEER_NONE = 0,

PEER_NONE,

rousskov · 2024-04-18T15:46:36Z

src/CachePeer.cc

+CachePeer::typeString() const
+{
+ static const char *typeNames[] {
+ "non-peer",


What is a "non-peer" CachePeer? I do not think those things exist in current code. If this PR has reasons to change these strings, then let's do this instead:

Suggested change

"non-peer",

"unknown",

P.S. I am also OK with replacing typeString() in various debugs() messages modified in this PR with cache_peer to avoid questions of this "non-peer" or "unknown" string escaping into contexts where it does not "fit". The rationale for reporting the peer type in those contexts is rather weak AFAICT. I do not insist on such debugs() changes though.

kinkie added 2 commits March 13, 2024 15:44

Use PackableStream for dump_peers

71ae5e0

Fix formatting

57d511e

This comment was marked as resolved.

Sign in to view

rousskov requested changes Mar 13, 2024

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 13, 2024

kinkie added 4 commits March 14, 2024 08:00

Source maintenance

8441c91

dump_peer_options -> CachePeer::dumpOptions

b411cee

neighborTypeStr -> CachePeer::typeString

5a46e40

dump_peers internals -> CachePeer::reportStatistics

cd395c9

This comment was marked as outdated.

Sign in to view

Source maintenance

0533486

This comment was marked as resolved.

Sign in to view

kinkie and others added 10 commits March 14, 2024 14:26

fix typo

d99bc9e

Move newline at end of CachePeer::dumpOptions to callers

d0e4b1b

move opening newline in reportStatistics()

321a664

Reposition newline

40d7295

Apply suggestions from code review

aec3b06

Co-authored-by: Alex Rousskov <[email protected]>

fix const

f1e5848

Do not consider current configuration to output stats

50959d5

Reorder CachePeer::typeString position

5486566

Steamline output

cc0587d

Move away from all caps

58759f6

kinkie requested review from rousskov and yadij March 15, 2024 05:34

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

Simplified htcp options output

0513961

rousskov requested changes Mar 15, 2024

View reviewed changes

Merge branch 'master' into packablestream-dump_peers

2886965

kinkie removed 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) labels Apr 13, 2024

kinkie requested review from yadij and rousskov April 13, 2024 11:07

rousskov requested changes Apr 13, 2024

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Apr 13, 2024

kinkie mentioned this pull request Apr 14, 2024

Format mgr:pconn as YAML #1780

Closed

kinkie added 6 commits April 14, 2024 12:23

Make CachePeer::reportStatistics const

648f7a9

remove "icp " prefix for ICP opcodes output

5552cca

capitalize protocol names in output

eff7ce7

simplify address output iteration

bf1721c

std::vector must have a non-const, non-volatile value_type

e5f2cc0

add missing spaces, quote name

3dbe088

kinkie requested a review from rousskov April 17, 2024 09:02

kinkie removed the S-waiting-for-author author action is expected (and usually required) label Apr 17, 2024

rousskov added the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Apr 18, 2024

kinkie mentioned this pull request Apr 25, 2024

Emit YAML for events cachemgr report #1792

Open

rousskov requested changes Apr 30, 2024

View reviewed changes

rousskov added the S-waiting-for-author author action is expected (and usually required) label Apr 30, 2024

use application/yaml content-type

6756fdd

kinkie mentioned this pull request May 1, 2024

Make SMP squid reports YAML-compliant #1784

Draft

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit valid YAML for cachemgr:server_list #1735

Emit valid YAML for cachemgr:server_list #1735

kinkie commented Mar 13, 2024 •

edited

kinkie commented Mar 13, 2024

This comment was marked as resolved.

rousskov left a comment

This comment was marked as outdated.

This comment was marked as resolved.

kinkie commented Mar 15, 2024

kinkie commented Apr 13, 2024

kinkie commented Apr 13, 2024

yadij commented Apr 13, 2024

rousskov commented Apr 13, 2024

rousskov commented Apr 13, 2024

rousskov left a comment

rousskov Apr 13, 2024

rousskov Apr 13, 2024

kinkie Apr 17, 2024

rousskov Apr 18, 2024

kinkie commented Apr 17, 2024

kinkie commented Apr 17, 2024

Footnotes

rousskov left a comment

rousskov Apr 18, 2024

rousskov Apr 18, 2024

rousskov Apr 18, 2024

rousskov Apr 18, 2024

Emit valid YAML for cachemgr:server_list #1735

Are you sure you want to change the base?

Emit valid YAML for cachemgr:server_list #1735

Conversation

kinkie commented Mar 13, 2024 • edited

kinkie commented Mar 13, 2024

This comment was marked as resolved.

rousskov left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

kinkie commented Mar 15, 2024

kinkie commented Apr 13, 2024

kinkie commented Apr 13, 2024

yadij commented Apr 13, 2024

rousskov commented Apr 13, 2024

rousskov commented Apr 13, 2024

Footnotes

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

kinkie commented Apr 17, 2024

kinkie commented Apr 17, 2024

Footnotes

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

kinkie commented Mar 13, 2024 •

edited