paradigmxyz / reth

Modular, contributor-friendly and blazing-fast implementation of the Ethereum protocol, in Rust
https://reth.rs/
Apache License 2.0
3.99k stars 1.21k forks source link

Improve metrics hooks setup (fixes #12672) #12684

Closed nils-mathieu closed 18 hours ago

nils-mathieu commented 2 days ago

This pull request fixes #12672.

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

Changes

Considerations

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

nils-mathieu commented 2 days ago

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.

mattsse commented 2 days ago

regarding the different mdbx type, could you ptal at

https://github.com/paradigmxyz/reth/issues/12259#issuecomment-2452962847

nils-mathieu commented 1 day ago

@mattsse done!