Skip to content

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Jul 3, 2024

Issue Addressed

When adding metrics for

Noted that the DB metrics are already there but we don't track them by column. I would like to be able to tell:

  • How many reads / writes happen on the state diff buffer column?
  • How many bytes do we load / store on the state diff buffer column?

Proposed Changes

  • Switch store metrics from IntCounter to IntCounterVec and label by column enum 3 letter string.
  • Track metrics in do_atomically calls, optimistically assuming success

@dapplion dapplion requested a review from michaelsproul July 3, 2024 11:55
leveldb_batch.put(BytesKey::from_vec(key), &value);
}

KeyValueStoreOp::DeleteKey(key) => {
let col = get_col_from_key(&key).unwrap_or("unknown".to_owned());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

get_col_from_key should never error as we define the DBColumn enum variants. The use of col is purely informational on metrics, so unwrapping to unknown to have something

@dapplion dapplion added the ready-for-review The code is ready for review label Jul 3, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 16, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 18, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at b0b142c

mergify bot added a commit that referenced this pull request Jul 18, 2024
@mergify mergify bot merged commit b0b142c into sigp:unstable Jul 18, 2024
2 checks passed
@dapplion dapplion deleted the store-metrics-vec branch July 24, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants