Skip to content
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

feat: Event streams #261

Merged
merged 1 commit into from
Jul 26, 2023
Merged

feat: Event streams #261

merged 1 commit into from
Jul 26, 2023

Conversation

dphilipson
Copy link
Collaborator

@dphilipson dphilipson commented Jul 18, 2023

Introduce streams of events from the op pool and builder, which provide detailed debug information and can be consumed from broadcast channels. As an initial application, provide logging based on the content of these streams, which can be used to debug the path a user op takes.

To make this possible, several refactors are required:

  • The input side of the channel is threaded through various structs used
    throughout, such as the bundle proposer.
  • A new emit submodule is added in each of cli::node, builder,
    op_pool, and common. In each case, this module defines the
    relevant event types, including Display implementations for the
    default logging. The one in common also defines some helper
    functions for managing event streams. them into log messages (logged
    via tracing).
  • The transaction tracker now has separate steps for submitting a
    transaction and then waiting for it, rather than a singule function
    which does both. This is because we do not know the transaction hash
    until the transaction is sent, due to last-minute changes made by the
    transaction sender, and we want to be able to log the send as it
    occurs.

Addresses #247, although to close it may need further work to pump these events to analytics tooling.

src/cli/node.rs Outdated Show resolved Hide resolved
.map(|hash| format!("{hash:?}"))
.collect::<Vec<_>>()
.join(", ");
info!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may make sense for these event loggers to take in a log level in case we want to turn them down from info.

Copy link
Collaborator Author

@dphilipson dphilipson Jul 23, 2023

Choose a reason for hiding this comment

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

This is a good idea, although it's oddly difficult with the logging crate to have a log level specified at runtime, as the event! macro expects the level to be a constant. While I could hardcode each branch like

// The enum part of level isn't public, so no `match`ing
if level == Level::TRACE {
    event!(Level::TRACE, "{}", ev)
} else if level == Level::DEBUG {
    event!(Level::DEBUG, "{}", ev)
} else if level == Level::INFO {
    // ...
}

That's awkward enough that I'm inclined to leave it hardcoded until we have a specific use in mind. If we want to change the level in code we can just change it (and after the latest refactor it only needs to be changed in one place), and if we want the level to be user-configurable then we should be sure that the use case can't equally be served by the user filtering out messages from a specific module (in this case common::emit), as the tracing crate seems to be designed to encourage that usage.

src/cli/node.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dancoombs dancoombs left a comment

Choose a reason for hiding this comment

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

This is awesome! I can really see this becoming the basis of our mempool visibility system. For now the logging is going to provide us some much needed insights into why UOs are stuck/dropped.

@dphilipson dphilipson force-pushed the dp/events branch 2 times, most recently from 2b4c521 to 5865336 Compare July 24, 2023 00:47
Copy link
Collaborator

@dancoombs dancoombs left a comment

Choose a reason for hiding this comment

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

Nice!

Introduce streams of events from the op pool and builder, which provide
detailed debug information and can be consumed from broadcast channels.
As an initial application, provide logging based on the content of these
streams, which can be used to debug the path a user op takes.

To make this possible, several refactors are required:

* The input side of the channel is threaded through various structs used
  throughout, such as the bundle proposer.
* A new `emit` submodule is added in each of `cli::node``, `builder`,
  `op_pool`, and `common`. In each case, this module defines the
  relevant event types, including `Display` implementations for the
  default logging. The one in `common` also defines some helper
  functions for managing event streams.  them into log messages (logged
  via `tracing`).
* The transaction tracker now has separate steps for submitting a
  transaction and then waiting for it, rather than a singule function
  which does both. This is because we do not know the transaction hash
  until the transaction is sent, due to last-minute changes made by the
  transaction sender, and we want to be able to log the send as it
  occurs.
@dphilipson dphilipson merged commit a89be00 into main Jul 26, 2023
4 checks passed
@dphilipson dphilipson deleted the dp/events branch July 26, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants