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

fix(metrics): removed influx and added l2_state_size data #196

Merged
merged 13 commits into from
Jul 18, 2024
Merged

Conversation

antiyro
Copy link
Member

@antiyro antiyro commented Jul 6, 2024

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Testing
  • Other (please describe):

What is the current behavior?

Resolves: #NA

What is the new behavior?

Does this introduce a breaking change?

Other information

@antiyro antiyro changed the title added storage metric fix(metrics): removed influx and added l2_state_size endpoint Jul 6, 2024
@antiyro antiyro marked this pull request as ready for review July 6, 2024 17:07
Copy link
Member

@jbcaron jbcaron left a comment

Choose a reason for hiding this comment

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

The get_directory_size function looks efficient overall, but here are a few points to consider to ensure it doesn’t introduce unnecessary overhead:

  • Error Handling: The function currently ignores errors when reading directory entries or fetching metadata.
  • Concurrency: If the directory contains a large number of files or deeply nested structures, processing them sequentially could become a bottleneck. Consider using a concurrent approach to traverse the directory entries in parallel.
  • Metrics Frequency Consideration: Since this function is used for metrics, consider how frequently these metrics need to be collected. If the function is called frequently, ensure that it does not introduce significant performance overhead.

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

## Next release

- fix(metrics): removed influx and added l2_state_size endpoint
Copy link
Member

Choose a reason for hiding this comment

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

nit: its not really an endpoint

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry i forgot to change this

@@ -125,3 +125,24 @@ pub fn trim_hash(hash: &Felt) -> String {

format!("{}...{}", prefix, suffix)
}

pub fn get_directory_size(path: &Path) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we were using https://docs.rs/rocksdb/latest/rocksdb/struct.DBCommon.html#method.get_column_family_metadata_cf
which returns, for a given column, its size as

pub struct ColumnFamilyMetaData {
    // The size of this column family in bytes, which is equal to the sum of
    // the file size of its "levels".
    pub size: u64,
    // The name of the column family.
    pub name: String,
    // The number of files in this column family.
    pub file_count: usize,
}

Plus, we can do a sum gauge and a individual gauge for each column

crates/client/sync/src/l2.rs Outdated Show resolved Hide resolved
@antiyro antiyro changed the title fix(metrics): removed influx and added l2_state_size endpoint fix(metrics): removed influx and added l2_state_size data Jul 18, 2024
Copy link
Member

@cchudant cchudant left a comment

Choose a reason for hiding this comment

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

Perfect :)

@antiyro antiyro merged commit 3582291 into main Jul 18, 2024
7 checks passed
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.

4 participants