rpc: Make HTTP RPC debug logging more informative #14618

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:limit-rpc-request-logging changing 3 files +9 −3

practicalswift commented at 10:45 AM on October 31, 2018: contributor
- Make HTTP RPC debug logging more informative
- Avoid excessively large log messages (which could theoretically fill up the disk) when running with debug option -debug=http
fanquake added the label RPC/REST/ZMQ on Oct 31, 2018
laanwj commented at 4:47 PM on October 31, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/14618/commits/a6cf746b43686c1c2307b31308d2d6cd18ddb696 thanks!
kristapsk commented at 5:01 PM on October 31, 2018: contributor

utACK a6cf746b43686c1c2307b31308d2d6cd18ddb696
luke-jr commented at 6:00 PM on October 31, 2018: member

utACK
hebasto commented at 9:28 PM on October 31, 2018: member

utACK a6cf746b43686c1c2307b31308d2d6cd18ddb696

in src/httpserver.cpp:244 in a6cf746b43 outdated

 242 |          hreq->WriteReply(HTTP_BADMETHOD);
 243 |          return;
 244 |      }
 245 |  
 246 | +    LogPrint(BCLog::HTTP, "Received a %s request for %s from %s\n",
 247 | +             RequestMethodString(hreq->GetRequestMethod()), SanitizeString(hreq->GetURI()).substr(0, 100), hreq->GetPeer().ToString());

ken2812221 commented at 5:14 AM on November 1, 2018:

Why call SanitizeString here? It would discard urlencoded characters '%' if the user use them in wallet name.

ken2812221 commented at 5:19 AM on November 1, 2018: contributor

Concept ACK
jgarzik commented at 2:47 PM on November 1, 2018: contributor

utACK

However, it must be noted that the HTTP server has always lacked proper request logging.

Having a separate log stream with logs parse-able by HTTP log analysis tools would be nice to have.
practicalswift commented at 4:04 PM on November 1, 2018: contributor

@ken2812221 Thanks for reviewing. Now allowing all characters allowed in URIs.

Please re-review :-)
practicalswift force-pushed on Nov 1, 2018
luke-jr commented at 5:29 AM on November 2, 2018: member

Related (to adding new chars for SanitizeString): #10731
DrahtBot commented at 2:43 PM on November 2, 2018: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
promag commented at 2:51 PM on November 2, 2018: member

@jgarzik see discussion in #13992.

in src/httpserver.cpp:244 in b301fecb83 outdated

 242 |          hreq->WriteReply(HTTP_BADMETHOD);
 243 |          return;
 244 |      }
 245 |  
 246 | +    LogPrint(BCLog::HTTP, "Received a %s request for %s from %s\n",
 247 | +             RequestMethodString(hreq->GetRequestMethod()), SanitizeString(hreq->GetURI(), SAFE_CHARS_URI).substr(0, 100), hreq->GetPeer().ToString());

promag commented at 3:27 PM on November 3, 2018:

What's the deal with 100 chars?

laanwj commented at 12:41 PM on November 5, 2018:

it's nothing special simply a sanity limit that avoids logging huge entire requests

promag commented at 3:29 PM on November 3, 2018: member

utACK b301fecb.
sipa commented at 7:17 PM on November 3, 2018: member

utACK b301fecb830ad8ca9ec2419978ae8e0b6fbc6754
jonasschnelli commented at 1:33 PM on November 4, 2018: contributor

utACK b301fecb830ad8ca9ec2419978ae8e0b6fbc6754
DrahtBot commented at 11:16 AM on November 5, 2018: member

Needs rebase
DrahtBot added the label Needs rebase on Nov 5, 2018
rpc: Make HTTP RPC debug logging more informative 991248649b
Add SAFE_CHARS[SAFE_CHARS_URI]: Chars allowed in URIs (RFC 3986) ab8c6f24d2
practicalswift force-pushed on Nov 5, 2018
practicalswift commented at 12:27 PM on November 5, 2018: contributor

Rebased! :-)
laanwj merged this on Nov 5, 2018
laanwj closed this on Nov 5, 2018
laanwj referenced this in commit 45f50063a9 on Nov 5, 2018
fanquake added the label Needs backport on Nov 29, 2018
fanquake removed the label Needs rebase on Nov 29, 2018
fanquake referenced this in commit 9666dbaf09 on Nov 29, 2018
fanquake referenced this in commit 79358817e5 on Nov 29, 2018
fanquake removed the label Needs backport on Nov 29, 2018
fanquake commented at 10:44 AM on November 29, 2018: member

Backported in #14835.
MarcoFalke referenced this in commit d8bc0ce1da on Nov 30, 2018
codablock referenced this in commit 2058a0fa39 on Oct 14, 2019
deadalnix referenced this in commit d6c2ce7439 on Apr 17, 2020
ftrader referenced this in commit 657f9dc3cf on Aug 17, 2020
practicalswift deleted the branch on Apr 10, 2021
PastaPastaPasta referenced this in commit 3aac3d0a30 on Jun 27, 2021
PastaPastaPasta referenced this in commit c789e4d711 on Jun 28, 2021
PastaPastaPasta referenced this in commit e8f1d9811c on Jun 29, 2021
PastaPastaPasta referenced this in commit e6d9381123 on Jul 1, 2021
PastaPastaPasta referenced this in commit cde9d18379 on Jul 1, 2021
PastaPastaPasta referenced this in commit 364f19ebfe on Jul 1, 2021
PastaPastaPasta referenced this in commit 87979a56bf on Jul 3, 2021
DrahtBot locked this on Aug 16, 2022