Skip to content

event loop: add LogOptions struct and reduce the log size #200

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 5, 2025
Merged

event loop: add LogOptions struct and reduce the log size #200

merged 1 commit into from
Sep 5, 2025

Conversation

@ismaelsadeeq
Copy link
Member

@ismaelsadeeq ismaelsadeeq commented Sep 2, 2025 •
edited
Loading

This PR fixes #190 by reducing the maximum size of characters to from 1000 to 200.

This is achieved by creating a new struct LogOptions
The LogOptions struct will contain
i) The log callback function pointer
ii) The maximum character to pass to LogEscape

The main motivation is to reduce the verbosity of the logs and make
LogEscape max_size configurable.

@DrahtBot
Copy link

DrahtBot commented Sep 2, 2025 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ryanofsky, Eunovo

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@ismaelsadeeq ismaelsadeeq mentioned this pull request Sep 2, 2025
Closed
@ismaelsadeeq ismaelsadeeq marked this pull request as draft September 2, 2025 15:56
@ismaelsadeeq
Copy link
Member Author

In draft while C.I is red.

Eunovo
Eunovo reviewed Sep 3, 2025
View reviewed changes
@ismaelsadeeq ismaelsadeeq marked this pull request as ready for review September 3, 2025 07:49
@Eunovo
Copy link
Contributor

Eunovo commented Sep 3, 2025

utACK e6013d7

ryanofsky
ryanofsky approved these changes Sep 3, 2025
View reviewed changes
Copy link
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK e6013d7. Thanks for the patch! Happy to merge as-is if you prefer, but commented below to recommend not changing the EventLoop constructor to make the PR a little smaller and make this easier to merge into bitcoin

@ismaelsadeeq ismaelsadeeq changed the title event loop: make log options configurable event loop: add LogOptions struct and reduce the log size Sep 5, 2025
@ismaelsadeeq
Copy link
Member Author

Forced pushed to address @ryanofsky recent comment

ce5bc1d9e28..d9eb7fb8c

ryanofsky
ryanofsky approved these changes Sep 5, 2025
View reviewed changes
Copy link
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK d9eb7fb. Looks great, thanks for the update!

@DrahtBot DrahtBot requested a review from Eunovo September 5, 2025 13:56
@ismaelsadeeq
eventloop: add LogOptions struct
8500340 
- The LogOptions struct will contain
  i) The log callback function pointer
  ii) The maximum character to pass to LogEscape

- This also reduce the verbosity of the logs and make
  LogEscape max_size configurable.
ryanofsky
ryanofsky approved these changes Sep 5, 2025
View reviewed changes
Copy link
Collaborator

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 8500340. Just whitespace cleanup since last review. Nice to have a shrinking PR

Eunovo
Eunovo approved these changes Sep 5, 2025
View reviewed changes
Copy link
Contributor

@Eunovo Eunovo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReACK 8500340

@ryanofsky ryanofsky merged commit 72dce11 into bitcoin-core:master Sep 5, 2025
8 checks passed
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 5, 2025
@ryanofsky
Squashed 'src/ipc/libmultiprocess/' changes from 1b8d4a6f1e54..13424c…
a334bbe 
…f2ecc1

13424cf2ecc1 Merge bitcoin-core/libmultiprocess#205: cmake: check for Cap'n Proto / Clang / C++20 incompatibility
72dce118649b Merge bitcoin-core/libmultiprocess#200: event loop: add LogOptions struct and reduce the log size
85003409f964 eventloop: add `LogOptions` struct
657d80622f81 cmake: capnproto pkg missing helpful error
d314057775a5 cmake: check for Cap'n Proto / Clang / C++20 incompatibility
878e84dc3030 Merge bitcoin-core/libmultiprocess#203: cmake: search capnproto in package mode only
1a85da5873c2 Merge bitcoin-core/libmultiprocess#202: doc: correct the build instructions for the example
df01873e1ecb Merge bitcoin-core/libmultiprocess#197: ci: Add freebsd and macos build
3bee07ab3367 cmake: search capnproto in package mode only
b6d3dc44194c doc: correct the build instructions for example
fa1ac3000055 ci: Add macos and freebsd task

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 13424cf2ecc1e5eadc85556cf1f4c65e915f702a
@ryanofsky ryanofsky mentioned this pull request Sep 5, 2025
Merged
fanquake added a commit to bitcoin/bitcoin that referenced this pull request Sep 8, 2025
@fanquake
Merge #33322: Update libmultiprocess subtree to improve build and logs
9c6fa07 
a334bbe Squashed 'src/ipc/libmultiprocess/' changes from 1b8d4a6f1e54..13424cf2ecc1 (Ryan Ofsky)

Pull request description:

  Includes:

  - bitcoin-core/libmultiprocess#197
  - bitcoin-core/libmultiprocess#202
  - bitcoin-core/libmultiprocess#203
  - bitcoin-core/libmultiprocess#200
  - bitcoin-core/libmultiprocess#205

  These changes should give better feedback when there are build errors, and also make IPC logs more readable.

  The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh)

ACKs for top commit:
  Sjors:
    ACK a4ee70e

Tree-SHA512: ddddd0ed44522ade98a5b94f44b57210794d64f8c378a00438082b8c377f41e9b86c0c5ed29add45472549758f7478ab220af8e268b90b30f57a236c639497d3
Sjors added a commit to Sjors/sv2-tp that referenced this pull request Sep 23, 2025
@Sjors
Squashed 'src/ipc/libmultiprocess/' changes from 1b8d4a6f1e..47d79db8a5
132621f 
47d79db8a5 Merge bitcoin-core/libmultiprocess#201: bug: fix mptest hang, ProxyClient<Thread> deadlock in disconnect handler
f15ae9c9b9 Merge bitcoin-core/libmultiprocess#211: Add .gitignore
4a269b21b8 bug: fix ProxyClient<Thread> deadlock if disconnected as IPC call is returning
85df96482c Use try_emplace in SetThread instead of threads.find
ca9b380ea9 Use std::optional in ConnThreads to allow shortening locks
9b07991135 doc: describe ThreadContext struct and synchronization requirements
d60db601ed proxy-io.h: add Waiter::m_mutex thread safety annotations
4e365b019a ci: Use -Wthread-safety not -Wthread-safety-analysis
15d7bafbb0 Add .gitignore
fe1cd8c761 Merge bitcoin-core/libmultiprocess#208: ci: Test minimum cmake version in olddeps job
b713a0b7bf Merge bitcoin-core/libmultiprocess#207: ci: output CMake version in CI script
0f580397c9 ci: Test minimum cmake version in olddeps job
d603dcc0ee ci: output CMake version in CI script
13424cf2ec Merge bitcoin-core/libmultiprocess#205: cmake: check for Cap'n Proto / Clang / C++20 incompatibility
72dce11864 Merge bitcoin-core/libmultiprocess#200: event loop: add LogOptions struct and reduce the log size
85003409f9 eventloop: add `LogOptions` struct
657d80622f cmake: capnproto pkg missing helpful error
d314057775 cmake: check for Cap'n Proto / Clang / C++20 incompatibility
878e84dc30 Merge bitcoin-core/libmultiprocess#203: cmake: search capnproto in package mode only
1a85da5873 Merge bitcoin-core/libmultiprocess#202: doc: correct the build instructions for the example
df01873e1e Merge bitcoin-core/libmultiprocess#197: ci: Add freebsd and macos build
3bee07ab33 cmake: search capnproto in package mode only
b6d3dc4419 doc: correct the build instructions for example
fa1ac30000 ci: Add macos and freebsd task

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 47d79db8a5528097b408e18f7b0bae11a6702d26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@Eunovo Eunovo Eunovo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reduce MAX_SIZE in LogEscape to 100

4 participants

@ismaelsadeeq @DrahtBot @Eunovo @ryanofsky