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

feat: add batcher queue len and queue size in bytes metrics #1593

Open
wants to merge 4 commits into
base: staging
Choose a base branch
from

Conversation

JulianVentura
Copy link
Collaborator

Add batcher queue len and queue size in bytes metrics

Description

This PR adds two new metrics for the batcher:

  • Queue length: The number of proofs waiting in the batcher's queue to be included in a batch
  • Queue size: The sum of the sizes of the verification data, measured in bytes, of the proofs waiting in the batcher's queue.
image

How to test

  1. Start all services locally, including the metrics
  2. Start sending proofs
  3. Go to the "System Data" dashboard and go to the bottom on the left size (the batcher side). You should see four new graphs showing the queue len and queue size.
  4. Stop sending proofs and the metrics should be reset back to zero.
  5. Send a burst of many proofs to see a spike in the graphs with make batcher_send_sp1_burst BURST_SIZE=23

Type of change

Please delete options that are not relevant.

  • New feature
  • Bug fix
  • Optimization
  • Refactor

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

Comment on lines 1238 to 1239
self.metrics.queue_len.set(queue_len as i64);
self.metrics.queue_size_bytes.set(queue_size_bytes as i64);
Copy link
Collaborator

@MarcosNicolau MarcosNicolau Dec 9, 2024

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:

Suggested change
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.

Copy link
Collaborator Author

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 👌🏻

Copy link
Collaborator

@MarcosNicolau MarcosNicolau left a 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.

Copy link
Collaborator

@JuArce JuArce left a 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

Copy link
Contributor

@uri-99 uri-99 left a 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

@JulianVentura
Copy link
Collaborator Author

Sorry! Grafana dashboard was just added @uri-99 @JuArce @MarcosNicolau

@JulianVentura JulianVentura self-assigned this Dec 10, 2024
@PatStiles
Copy link
Contributor

Worked locally for me!

Copy link
Contributor

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

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.

5 participants