-
Notifications
You must be signed in to change notification settings - Fork 351
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
feat: add batcher queue len and queue size in bytes metrics #1593
base: staging
Are you sure you want to change the base?
Conversation
batcher/aligned-batcher/src/lib.rs
Outdated
self.metrics.queue_len.set(queue_len as i64); | ||
self.metrics.queue_size_bytes.set(queue_size_bytes as i64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to this logic to aligned-batcher/metrics.rs
and do:
self.metrics.queue_len.set(queue_len as i64); | |
self.metrics.queue_size_bytes.set(queue_size_bytes as i64); | |
self.metrics.update_queue_metrics(queue_len, queue_size_bytes); |
Mostly because they are "measuring the same" and we always want to update them simultaneously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's a good idea 👌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You haven't updated the grafana json! Thou I added them myself and it does seem to be working fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is missing to update the grafana dashbaord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol yes missing graphana visualizations
Sorry! Grafana dashboard was just added @uri-99 @JuArce @MarcosNicolau |
Worked locally for me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think these 2 new boxes in grafana are quite informative, yet they ended up quite in the bottom of the dashboard. i would put them a bit higher
Add batcher queue len and queue size in bytes metrics
Description
This PR adds two new metrics for the batcher:
How to test
make batcher_send_sp1_burst BURST_SIZE=23
Type of change
Please delete options that are not relevant.
Checklist
testnet
, everything else tostaging