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

Adds logging of column family size and database size on startup and s… #8336

Merged
merged 16 commits into from
Mar 29, 2024

Conversation

elijahhampton
Copy link
Contributor

@elijahhampton elijahhampton commented Mar 2, 2024

Motivation

What are the most important goals of the ticket or PR?

This PR allows for zebra users to monitor memory usage of the zebra node. Users can now monitor database and column family sizes. This PR addresses the following issue: #7416

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Specifications

Not applicable.

Complex Code or Requirements

No.

Solution

A function was added to the DiskDb struct to get metrics from each column family handle, print each metric and also calculate the total metric across live database size, total sst files size, and size of tables in memory.

A call to log_db_metrics was added right after the state service initialization. This ensures that as soon as the database is initialized and ready, its metrics are logged.

To handle various shutdown scenarios (e.g., graceful shutdown, errors, SIGINT), the logging of metrics at shutdown was encapsulated within the Drop trait for StateService. The Drop trait's drop method is automatically called when an instance goes out of scope, making it a reliable place to perform cleanup tasks and final logging actions.

Finally, the logic to build the column families vector was encapsulated into a function construct_column_families.

Testing and Review

Testing can be manually completed. See testing instructions below. This PR is not blocking any other work.

Testing instructions
Manually compare the total with the size on disk using du, and the size in memory using top.

RocksDB uses extra files for old data and deleted data, so the RocksDB disk sizes should be smaller. Live disk should also be smaller than total disk.

Zebra uses memory outside RocksDB, so the RocksDB memory usage should be smaller.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

@elijahhampton elijahhampton requested a review from a team as a code owner March 2, 2024 19:57
@elijahhampton elijahhampton requested review from oxarbitrage and removed request for a team March 2, 2024 19:57
@github-actions github-actions bot added the C-feature Category: New features label Mar 2, 2024
@mpguerra
Copy link
Contributor

mpguerra commented Mar 4, 2024

Thank you for the PR! We will try to review shortly (sometime this week)

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Hey, this looks really good, sorry for the delay in the review. I left a few suggestions but i think the only one important is to reduce the log level of the column families.

zebra-state/src/service/finalized_state/disk_db.rs Outdated Show resolved Hide resolved
zebra-state/src/service/finalized_state/disk_db.rs Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor

It seems we need to add the print_db_metrics to the ZebraDb structure. I think this can be just something as:

    pub fn print_db_metrics(&self) {
        self.db.print_db_metrics();
    }

But i did not tried it, let me know if you want me to try to fix that part.

https://github.com/ZcashFoundation/zebra/actions/runs/8246495198/job/22552667617?pr=8336#step:6:522

@elijahhampton
Copy link
Contributor Author

elijahhampton commented Mar 12, 2024

It seems we need to add the print_db_metrics to the ZebraDb structure. I think this can be just something as:


    pub fn print_db_metrics(&self) {

        self.db.print_db_metrics();

    }

But i did not tried it, let me know if you want me to try to fix that part.

https://github.com/ZcashFoundation/zebra/actions/runs/8246495198/job/22552667617?pr=8336#step:6:522

Okay, I just saw these comments. I will take care of this asap. Of course feel free to edit/fix anything!

@elijahhampton elijahhampton requested a review from a team as a code owner March 23, 2024 01:18
@elijahhampton elijahhampton requested review from oxarbitrage and removed request for a team March 23, 2024 01:18
oxarbitrage
oxarbitrage previously approved these changes Mar 26, 2024
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Just needs to remove a file generated by prop tests.

@oxarbitrage
Copy link
Contributor

I created a follow up ticket to convert bytes to human readable form at #8380

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Thanks again!

@mergify mergify bot merged commit 49fca30 into ZcashFoundation:main Mar 29, 2024
102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants