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

Improve metrics hooks setup (fixes #12672) #12684

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nils-mathieu
Copy link

@nils-mathieu nils-mathieu commented Nov 19, 2024

This pull request fixes #12672.

It adds the type HooksBuilder, responsible for creating new custom instances of the Hooks collection.

Changes

  • Add the HooksBuilder type.
  • Add the Hooks::builder method to create a new HooksBuilder.
  • Change the calls to Hooks::new by Hooks::builder.
  • Remove Hooks::new.

Considerations

  • I added some convenience methods to HooksBuilder to ease its usage a bit.
    • HooksBuilder::with_database_metrics
    • HooksBuilder::with_static_file_provider
    • HooksBuilder::with_default_collector

I made that choice because moving the cloning logic to the caller of Hooks::new requires us to write a bit of boilerplate due to having to move resources into the static hook closure:

    .with_hook({
        let db = factory.db_ref().clone();
        move || db.report_metrics()
    })

Replaced by:

    .with_database_metrics(factory.db_ref().clone())

I'm not sure whether this was a good idea. It is a bit faster to write, but maybe it makes the logic a bit harder to reason about. If someone reads that, they might assume that with_database_metrics is setting a specific "database_metrics" field in the builder, when it is not.

I can remove those convenience methods if needed.

Other

  • I had to fix a compilation error here (there was a comparaison between i32 and u32:

    pub const fn mode(&self) -> Mode {
    let mode = self.0.mi_mode;
    if (mode & ffi::MDBX_RDONLY) != 0 {
    Mode::ReadOnly
    } else if (mode & ffi::MDBX_UTTERLY_NOSYNC) != 0 {
    Mode::ReadWrite { sync_mode: SyncMode::UtterlyNoSync }
    } else if (mode & ffi::MDBX_NOMETASYNC) != 0 {
    Mode::ReadWrite { sync_mode: SyncMode::NoMetaSync }
    } else if (mode & ffi::MDBX_SAFE_NOSYNC) != 0 {
    Mode::ReadWrite { sync_mode: SyncMode::SafeNoSync }
    } else {
    Mode::ReadWrite { sync_mode: SyncMode::Durable }
    }
    }

  • I changed a format! to a format_args! in the Debug implementation of Hooks to save on an allocation.

@nils-mathieu
Copy link
Author

nils-mathieu commented Nov 19, 2024

The ffi::MDBX_RDONLY type and friends seem to have different types on my machine (Windows 11) than they are on the testing environment.

Should I duplicate the function? Ideally the constant themselves should change not to change type depending on the compilation target, I guess.

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great start

crates/cli/commands/src/stage/run.rs Outdated Show resolved Hide resolved
crates/node/metrics/src/hooks.rs Outdated Show resolved Hide resolved
crates/node/builder/src/launch/common.rs Outdated Show resolved Hide resolved
crates/storage/libmdbx-rs/src/environment.rs Outdated Show resolved Hide resolved
@mattsse
Copy link
Collaborator

mattsse commented Nov 19, 2024

regarding the different mdbx type, could you ptal at

#12259 (comment)

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.

Improve metrics hook setup
2 participants