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.
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
Add log output during initial header sync #15615
Conversation
5691f26
to
4e74875
Compare
Tested ACK 4e74875. Sample output with
|
Can we find a way to make it so that the non-debug output is non-quiet but not spammy? I'm afraid the total number of logged lines would be substantial if merged as is. This in turn would educe the signal-to-noise ratio and reduce the value of the log. Logging too much to the non-debug log is almost as bad as logging too little :-) |
I don't think its spammy... At current height, it logs around 284 lines for the header sync... seems acceptable compared to >567000 lines from UpdateTip during IBD. |
@jonasschnelli I'm not very fond of the Can you think of a smart way to reduce the number of logged lines while achieving the stated goals? Let's not forget that this is the logging meeting a first-time |
It's called a debug log. I don't think the criterion to optimize for is usability, but a tradeoff between disk usage and ability to diagnose problems (by an expert user or developer). |
@sipa Clarification: When talking about usability I was referring the "write to standard output for immediate consumption" aspect of Generally I think we print too much debug information to standard output resulting in a low signal-to-noise ratio for the end-user invoking Perhaps this could be solved by having two different log levels for |
4e74875
to
bb29079
Compare
Concept ACK, I think it's better to log something. Could add logic to log only every so many %, but this is a good start. It does fail to build here, might need rebase for recent changes:
|
bb29079
to
d75e704
Compare
Thanks @laanwj! |
Concept ACK d75e704 Agree with @sipa:
Tested on top of master (7524376) using -testnet -debug=net: 143:2019-06-14T07:28:12Z Synchronizing blockheaders, height: 2000 (~0.54%)
159:2019-06-14T07:28:13Z Synchronizing blockheaders, height: 4000 (~1.07%)
167:2019-06-14T07:28:14Z Synchronizing blockheaders, height: 6000 (~1.59%)
194:2019-06-14T07:28:15Z Synchronizing blockheaders, height: 8000 (~2.11%)
201:2019-06-14T07:28:15Z Synchronizing blockheaders, height: 10000 (~2.63%)
206:2019-06-14T07:28:16Z Synchronizing blockheaders, height: 12000 (~3.14%)
213:2019-06-14T07:28:16Z Synchronizing blockheaders, height: 14000 (~3.64%)
231:2019-06-14T07:28:17Z Synchronizing blockheaders, height: 16000 (~4.17%)
248:2019-06-14T07:28:17Z Synchronizing blockheaders, height: 18000 (~4.75%)
273:2019-06-14T07:28:18Z Synchronizing blockheaders, height: 20000 (~5.27%)
......
1239:2019-06-14T07:28:55Z Synchronizing blockheaders, height: 268000 (~50.83%)
1243:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 254000 (~48.93%)
1247:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 260000 (~49.57%)
1251:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 258000 (~49.34%)
1255:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 270000 (~51.15%)
1259:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 270000 (~51.15%)
1263:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 258000 (~49.34%)
1267:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 256000 (~49.13%)
1271:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 258000 (~49.34%)
1275:2019-06-14T07:28:56Z Synchronizing blockheaders, height: 272000 (~51.57%)
......
350831:2019-06-14T07:34:51Z Synchronizing blockheaders, height: 1532000 (~99.94%)
352022:2019-06-14T07:34:51Z Synchronizing blockheaders, height: 1534000 (~99.94%)
353240:2019-06-14T07:34:52Z Synchronizing blockheaders, height: 1536000 (~99.94%)
354471:2019-06-14T07:34:53Z Synchronizing blockheaders, height: 1538000 (~99.94%)
355709:2019-06-14T07:34:53Z Synchronizing blockheaders, height: 1540000 (~99.95%)
357365:2019-06-14T07:34:54Z Synchronizing blockheaders, height: 1542000 (~99.97%)
358206:2019-06-14T07:34:55Z Synchronizing blockheaders, height: 1543389 (~100.00%)
609974:2019-06-14T07:37:43Z Synchronizing blockheaders, height: 1543389 (~100.00%) The full |
Again: the problem is not what we're writing to disk. The problem is that this will also be written to standard output. I'm afraid such verbose logging will flood the console and effectively hide any important non- I'm not convinced this will be a net win if merged as is: sure, we'll have plenty of information about any I agree that it is problematic that we're completely quiet during the header sync, but I'm not at all convinced that the only sane alternative to the status quo is the other extreme: to flood everything we know about it :-) |
@practicalswift: |
@jonasschnelli Sorry: my last comment was written under the incorrect assumption that the number of lines that would be printed was ~5000 judging from @fanquake's posted gist. I read the output as 5000 lines written at a speed of 10 lines/second during the first ten minutes of operation. I now understand that this is incorrect :-) I guess 280 lines is a bit more than I'd like personally but it is way better than the 5000 lines I thought. Anyways I understand that the consensus opinion is to go with current version, so I won't argue further. Let's go with this and tweak later if necessary :-) utACK d75e704 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK d75e704
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
…ages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes #16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in #15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on #16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
Summary: Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) --- Backport of Core [[bitcoin/bitcoin#15615 | PR15615]] Test Plan: ninja check check-functional run bitcoind with a fresh datadir and see the new header sync log output Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7624
…ders Summary: The locking was added in a [[bitcoin/bitcoin#15615 (comment) | previous pull request]] during the review phase, but the reason for this lock was later [[bitcoin/bitcoin#15615 (review) | removed during the same review]]. Backport of Core [[bitcoin/bitcoin#16793 | PR16793]] Depends on D7634 Test Plan: ``` cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=Debug ninja all ckeck-all ``` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7637
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
d75e704 Add log output during initial header sync (Jonas Schnelli) Pull request description: The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync... * users may think the node did fail to start sync * it's a little complicate to check if your getting throttled during header sync (repeatedly calling `getchaintips` or similar) ACKs for top commit: fanquake: Concept ACK bitcoin@d75e704 practicalswift: utACK d75e704 laanwj: Tested ACK d75e704 Tree-SHA512: 2e738571b703d7251290864603c3a829729645962c2fa3187250bab0585e66a5f01fce892e9b5b98da451fab2b40a2e4784f9b2e5a9cad75ff62c535affe7430
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
The non debug log output is completely quiet during the header sync. I see two main reasons to add infos about the state of the initial header sync...
getchaintips
or similar)