feat(zetaclient): add TSS sign latency histogram #2567
Conversation
gartnera
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe recent changes enhance the observability and control flow of the signing process within the system. Key modifications include the introduction of a new histogram metric for tracking TSS keysign latency, adjustments to the message signing methods to incorporate closures for better lifecycle management, and updates to corresponding test cases for improved validation. These changes collectively aim to bolster performance monitoring and error handling, ensuring a more robust and maintainable codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConcurrentKeysignsTracker
participant Metrics
Client->>ConcurrentKeysignsTracker: StartMsgSign()
ConcurrentKeysignsTracker->>Metrics: Register start time
ConcurrentKeysignsTracker-->>Client: Closure (end function)
Client->>ConcurrentKeysignsTracker: Complete signing (with result)
ConcurrentKeysignsTracker->>Metrics: Log end time and success/failure
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2567 +/- ##
===========================================
+ Coverage 46.63% 46.64% +0.01%
===========================================
Files 464 464
Lines 30843 30850 +7
===========================================
+ Hits 14383 14390 +7
Misses 15604 15604
Partials 856 856
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- contrib/localnet/docker-compose-monitoring.yml (1 hunks)
- zetaclient/metrics/metrics.go (1 hunks)
- zetaclient/tss/concurrent_keysigns_tracker.go (2 hunks)
- zetaclient/tss/concurrent_keysigns_tracker_test.go (1 hunks)
- zetaclient/tss/tss_signer.go (2 hunks)
Files skipped from review due to trivial changes (1)
- contrib/localnet/docker-compose-monitoring.yml
Additional context used
Path-based instructions (4)
zetaclient/tss/concurrent_keysigns_tracker_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/tss/concurrent_keysigns_tracker.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/metrics/metrics.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/tss/tss_signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
zetaclient/tss/tss_signer.go
[warning] 249-249: zetaclient/tss/tss_signer.go#L249
Added line #L249 was not covered by tests
[warning] 251-251: zetaclient/tss/tss_signer.go#L251
Added line #L251 was not covered by tests
[warning] 331-331: zetaclient/tss/tss_signer.go#L331
Added line #L331 was not covered by tests
[warning] 333-333: zetaclient/tss/tss_signer.go#L333
Added line #L333 was not covered by tests
Additional comments not posted (4)
zetaclient/tss/concurrent_keysigns_tracker_test.go (1)
21-24: Ensure comprehensive test coverage.The test correctly captures and invokes the return values from
StartMsgSign. However, consider adding more assertions to verify the state before and after each invocation to ensure comprehensive test coverage.zetaclient/tss/concurrent_keysigns_tracker.go (1)
30-54: Ensure thread safety and metric accuracy.The closure correctly handles the end of the signing process and updates the metrics. Ensure that the mutex is properly used to maintain thread safety. The use of
prometheus.Labels{"result": result}is appropriate for categorizing the results.zetaclient/metrics/metrics.go (1)
108-114: Ensure appropriate bucket ranges for latency.The
SignLatencyhistogram vector is correctly defined. Ensure that the bucket ranges (1, 15, 30, 60, 120, 240 seconds) are appropriate for the expected latency distribution of TSS keysign operations.zetaclient/tss/tss_signer.go (1)
249-251: Ensure thorough testing of new tracking logic.The changes to track the signing process using
StartMsgSignandEndMsgSignimprove the control flow. However, these lines are not covered by tests.Would you like me to generate the unit tests for these changes or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 249-249: zetaclient/tss/tss_signer.go#L249
Added line #L249 was not covered by tests
[warning] 251-251: zetaclient/tss/tss_signer.go#L251
Added line #L251 was not covered by tests
gartnera
force-pushed
the
zetaclient-keysign-histogram
branch
from
ec33f27 to
1f4e963
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Add a
zetaclient_sign_latencyhistogram metric to allow easily debugging how many keysigns are active. Output:Closes #2564
Summary by CodeRabbit
New Features
Bug Fixes
Tests