Skip to content
This repository has been archived by the owner on Oct 31, 2024. It is now read-only.

feat: track queue size in metrics/graphql #454

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

Freyskeyd
Copy link
Member

Description

This PR adds two new metrics accessible on the default port 3000 (e.g.: localhost:3000):

  • STORAGE_PENDING_POOL_COUNT that tracks the number of certificates in the pending_pool.
  • STORAGE_PRECEDENCE_POOL_COUNT that tracks the number of certificates in the precedence_pool.

⚠️ Keep in my that those values are estimation based on the rocksdb metadata:

Q: Why GetIntProperty can only returns an estimated number of keys in a RocksDB database?

A: Obtaining an accurate number of keys in any LSM databases like RocksDB is a challenging problem as they have duplicate keys and deletion entries (i.e., tombstones) that will require a full compaction in order to get an accurate number of keys. In addition, if the RocksDB database contains merge operators, it will also make the estimated number of keys less accurate.

The GraphQL API is exposing those value under getStoragePoolStats.

PR Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added or updated tests that comprehensively prove my change is effective or that my feature works

@Freyskeyd Freyskeyd requested a review from a team as a code owner February 14, 2024 11:11
@Freyskeyd Freyskeyd requested review from dvdplm and hadjiszs and removed request for a team February 14, 2024 11:11
@Freyskeyd Freyskeyd force-pushed the chore/expose-metrics-storage-pool branch from 2e02bd7 to 5ef7657 Compare February 14, 2024 11:12
Copy link

codecov bot commented Feb 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (release/v0.0.11@c0feab0). Click here to learn what that means.

Additional details and impacted files
@@                Coverage Diff                 @@
##             release/v0.0.11     #454   +/-   ##
==================================================
  Coverage                   ?   69.49%           
==================================================
  Files                      ?      220           
  Lines                      ?    12394           
  Branches                   ?        0           
==================================================
  Hits                       ?     8613           
  Misses                     ?     3781           
  Partials                   ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM. All my comments are nits that can wait.


#[derive(Debug, Deserialize)]
struct Response {
// data: HashMap<String, serde_json::Value>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove I think.

@@ -232,8 +233,11 @@ impl WriteStore for FullNodeStore {
}

impl ReadStore for FullNodeStore {
fn count_certificates_delivered(&self) -> Result<usize, StorageError> {
Ok(self.perpetual_tables.certificates.iter()?.count())
fn count_certificates_delivered(&self) -> Result<u64, StorageError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should call this estimate_certs_delivered?

@@ -33,6 +33,9 @@ pub enum InternalStorageError {
#[error("Invalid query argument: {0}")]
InvalidQueryArgument(&'static str),

#[error("Unexpected DB state: {0}")]
UnexpectedDBState(&'static str),
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really a problem with the DB state if a property is missing. Wouldn't this error be for a situation where there's a new/old/incompatible rocksdb version that does not support some property?

@dvdplm dvdplm merged commit c3e5141 into release/v0.0.11 Feb 14, 2024
20 checks passed
@dvdplm dvdplm deleted the chore/expose-metrics-storage-pool branch February 14, 2024 13:25
Freyskeyd added a commit that referenced this pull request Feb 15, 2024
Signed-off-by: Simon Paitrault <[email protected]>
Co-authored-by: David Palm <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants