Skip to content

Add metrics to the database - #2638

Draft
paberr wants to merge 1 commit into
albatrossfrom
pb/db-metrics
Draft

Add metrics to the database#2638
paberr wants to merge 1 commit into
albatrossfrom
pb/db-metrics

Conversation

@paberr

@paberr paberr commented Jun 18, 2024

Copy link
Copy Markdown
Member

What's in this pull request?

Adds detailed metrics to the database crate – similarly to how reth is doing it.
This currently uses the metrics crate, which is not compatible with our prometheus_client crate.
I implemented a connection between these two but it is not really nice. Thus, I'll also open an issue to improve this situation at a later point.

Open TODOs:

  • Initialise database with metrics enabled.

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@paberr
paberr force-pushed the pb/db-metrics branch 3 times, most recently from a8a39ea to bb44f5b Compare June 18, 2024 10:04
@paberr paberr self-assigned this Jun 18, 2024
@paberr paberr added the enhancement New feature or request label Jun 18, 2024
@paberr
paberr requested review from hrxi and jsdanielh June 18, 2024 10:04
@paberr
paberr marked this pull request as ready for review June 18, 2024 10:05
@paberr
paberr marked this pull request as draft June 18, 2024 16:31
@paberr
paberr marked this pull request as ready for review June 18, 2024 16:51
@paberr
paberr force-pushed the pb/db-metrics branch 2 times, most recently from ee897be to b4255db Compare June 18, 2024 20:46

@hrxi hrxi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Most of the comments are minor or questions. The license/version this was taken from should probably be stated.

Comment thread Cargo.toml Outdated
Comment thread client/src/main.rs
Comment thread database/Cargo.toml
Comment thread database/src/lib.rs Outdated
Comment thread database/src/mdbx/database.rs Outdated
Comment thread metrics-server/src/metrics.rs Outdated
Comment thread database/src/mdbx/metrics_handler.rs Outdated
Comment thread database/src/metrics.rs Outdated
Comment thread database/src/mdbx/metrics_handler.rs Outdated
/// If `true`, the backtrace of transaction has already been recorded and logged.
/// See [`MetricsHandler::log_backtrace_on_long_read_transaction`].
backtrace_recorded: AtomicBool,
pub(super) env_metrics: Arc<DatabaseEnvMetrics>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you should be able to get away with a reference here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean with that. Replacing the Arc with a &?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think there's much value in that as the Database struct needs to have an Arc anyways.

@paberr
paberr marked this pull request as draft July 29, 2024 08:49
@paberr

paberr commented Jul 29, 2024

Copy link
Copy Markdown
Member Author

Incompatible with new DB refactor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants