Add signature verification/aggregation metrics and panic handlers #551
Conversation
I threw in the panic handlers just to be complete, I've seen too many panics in cryptography code.
blssig/metrics.go
Outdated
| verifyAggregate: measurements.Must(meter.Int64Histogram( | ||
| "f3_blssig_verify_aggregate", | ||
| metric.WithDescription("Aggregate signatures verified."), | ||
| )), | ||
| aggregate: measurements.Must(meter.Int64Histogram( | ||
| "f3_blssig_aggregate", | ||
| metric.WithDescription("Signatures aggregated."), | ||
| )), | ||
| } |
There was a problem hiding this comment.
do we need custom buckets here?
There was a problem hiding this comment.
Probably yes, adjusted based on expected network size and quorum? We can refine later if defaults don't make sense.
There was a problem hiding this comment.
I'll leave the defaults for now.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
==========================================
- Coverage 80.61% 80.31% -0.30%
==========================================
Files 46 47 +1
Lines 4230 4324 +94
==========================================
+ Hits 3410 3473 +63
- Misses 495 518 +23
- Partials 325 333 +8
|
blssig/metrics.go
Outdated
| verifyAggregate: measurements.Must(meter.Int64Histogram( | ||
| "f3_blssig_verify_aggregate", | ||
| metric.WithDescription("Aggregate signatures verified."), | ||
| )), | ||
| aggregate: measurements.Must(meter.Int64Histogram( | ||
| "f3_blssig_aggregate", | ||
| metric.WithDescription("Signatures aggregated."), | ||
| )), | ||
| } |
There was a problem hiding this comment.
Probably yes, adjusted based on expected network size and quorum? We can refine later if defaults don't make sense.
Co-authored-by: Masih H. Derkani <m@derkani.org>
Co-authored-by: Masih H. Derkani <m@derkani.org>
|
I would prefer if the cache had explicit # cache queries and # cache hits to allow calculating the cache hit ratio. |
|
Ahh, I see, you are doing it though a label, I guess it makes sense. |
I threw in the panic handlers just to be complete, I've seen too many panics in cryptography code.