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

Add metrics to the database #2638

Draft
wants to merge 1 commit into
base: albatross
Choose a base branch
from
Draft

Add metrics to the database #2638

wants to merge 1 commit into from

Conversation

paberr
Copy link
Member

@paberr paberr commented Jun 18, 2024

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
Copy link
Member

@hrxi hrxi left a comment

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.

Cargo.toml Outdated Show resolved Hide resolved
client/src/main.rs Show resolved Hide resolved
database/Cargo.toml Show resolved Hide resolved
database/src/lib.rs Outdated Show resolved Hide resolved
database/src/mdbx/database.rs Outdated Show resolved Hide resolved
metrics-server/src/metrics.rs Outdated Show resolved Hide resolved
database/src/mdbx/metrics_handler.rs Outdated Show resolved Hide resolved
database/src/metrics.rs Outdated Show resolved Hide resolved
database/src/mdbx/metrics_handler.rs Outdated Show resolved Hide resolved
/// 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
Member

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
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
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
Copy link
Member Author

paberr commented Jul 29, 2024

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