Log public key in addition to IP address - #5678
Conversation
|
Example |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5678 +/- ##
=========================================
- Coverage 78.3% 78.3% -0.0%
=========================================
Files 817 817
Lines 68982 68979 -3
Branches 8332 8308 -24
=========================================
- Hits 54023 54019 -4
- Misses 14959 14960 +1
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@a1q123456 could you update the example, please? I think by adding the public key to the prefix it'll change the output format a little. |
|
there are very valid reasons for not exposing IP's of validators. one simple one is I can now simply peer with any validator I wish. exposing the IP of validators opens them up to other attacks as well like ddos.. not a fan of this at all. if you require this debug build your own version of rippled and deploy it on the servers you need to monitor. it should not be system wide change. |
Hey Fomo, we’re already logging IP addresses and this PR doesn’t log more IP addresses. Instead, it only adds public keys to help us narrow down issues, as IP addresses are masked out in UNL logs and there’s no way to figure out which validator it is in logs. |
a1q123456
added
the
Needs additional review
a841fb2 to
a5fd74e
Compare
bthomee
left a comment
bthomee
left a comment
There was a problem hiding this comment.
Due to adding the fingerprint as a sink prefix, the logs will be growing as in the past only IDs were logged and now IP address and public key as well. By how much do you expect the logs to grow?
| PeerImp::makePrefix(std::string const& fingerprint) | ||
| { | ||
| std::stringstream ss; | ||
| ss << "[" << std::setfill('0') << std::setw(3) << fingerprint << "] "; |
There was a problem hiding this comment.
The std::setw(3) seems unnecessary, since the updated fingerprint now contains the IP address, public key, and node ID - which will be much longer than 3 characters.
| case Slot::closing: | ||
| JLOG(m_journal.trace()) << beast::leftw(18) << "Logic finished " | ||
| << slot->remote_endpoint(); | ||
| << slot->fingerprint(); |
There was a problem hiding this comment.
I'm noticing a general inconsistency in how the logging happens. In PeerImp the sinks were modified to use the fingerprint as prefix, but here the sinks are unmodified but the fingerprint is added explicitly to the log message.
I would suggest to revisit the codebase add the fingerprint as prefix to sinks wherever possible. If it isn't possible in most places, so you'll have to add it manually to the log lines for many sinks, then maybe it is better to not use the fingerprint as sink prefix anywhere and just add to each log line. I'm otherwise concerned the logs end up in a hard-to-parse mess.
There was a problem hiding this comment.
Tried running rippled for around 10 minutes for each branch and the result shows that the develop branch writes around 50 MB logs per minute and this PR writes 66 MB logs per minute, so I'd say this PR is going to increase the log file size by around 30% - 32%.
| Version | Duration (min) | Growth rate (bytes/min) | Growth rate (MB/min) |
|---|---|---|---|
| PR | 10.888 | 66,739,865 | 66.74 MB/min |
| develop | 10.727 | 50,785,364 | 50.79 MB/min |
6537490 to
c106139
Compare
8ac14de to
476b2e4
Compare
bthomee
removed
the
Needs additional review
a1q123456
added
the
Ready to merge
To protect the identity of UNL validators, the IP addresses are redacted from the log messages sent to the common Grafana instance. However, without such identifying information it is challenging to debug issues. This change adds a node's public key to logs to improve our ability to debug issues.
f87e421 to
e2ee5db
Compare
To protect the identity of UNL validators, the IP addresses are redacted from the log messages sent to the common Grafana instance. However, without such identifying information it is challenging to debug issues. This change adds a node's public key to logs to improve our ability to debug issues. Co-authored-by: Bart Thomee <11445373+bthomee@users.noreply.github.com>
5 participants
High Level Overview of Change
This PR adds public keys to logs.
Context of Change
Some validators used to complain that we're logging ip addresses which can expose them to attacks. However, after we masked out the ip addresses from the logs, we're unable to debug some issues. This PR adds public key to logs to help us debug issues.
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
There's no API impact.
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
Regression test.