RFQ API: fix prometheus setup #2701
Conversation
WalkthroughThe recent changes primarily focus on refactoring and enhancing metric handling across multiple files, optimizing transaction submission processes, and improving error handling in test cases. Metrics-related fields and methods were added to various structs, ensuring efficient recording and tracking. Dependencies for Changes
Poem
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 Configration File (
|
Deploying sanguine-fe with
|
| Latest commit: |
1aef5ad
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e7fe55d0.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-rfq-prom-fix.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2701 +/- ##
===================================================
- Coverage 25.71371% 25.70207% -0.01164%
===================================================
Files 759 759
Lines 54469 54482 +13
Branches 80 80
===================================================
- Hits 14006 14003 -3
- Misses 39020 39034 +14
- Partials 1443 1445 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/api/rest/server.go (3 hunks)
Additional comments not posted (2)
services/rfq/api/rest/server.go (2)
34-35: Added a constantmeterNamefor Prometheus metrics.This is a good practice as it centralizes the metric's name, making it easier to modify in the future if needed.
120-120: Initialization ofmeterusinghandler.Meter(meterName).This change aligns with the new struct definition and properly initializes the
meterfield. Ensure that thehandleris correctly configured to provide aMeterinstance.
| q.latestQuoteAgeGauge, err = q.meter.Float64ObservableGauge("latest_quote_age") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create latest quote age gauge: %w", err) | ||
| } | ||
|
|
||
| _, err = q.meter.RegisterCallback(q.recordLatestQuoteAge, q.latestQuoteAgeGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback: %w", err) | ||
| } |
There was a problem hiding this comment.
Setup of latestQuoteAgeGauge and registration of callback.
- q.latestQuoteAgeGauge, err = q.meter.Float64ObservableGauge("latest_quote_age")
+ q.latestQuoteAgeGauge, err = metric.Float64ObservableGauge(q.meter, "latest_quote_age")The method call seems incorrect as per the typical OpenTelemetry pattern. Please verify the correct method signature and adjust accordingly.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- services/rfq/api/rest/server.go (5 hunks)
- services/rfq/relayer/inventory/manager.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/relayer/inventory/manager.go
Files skipped from review as they are similar to previous changes (1)
- services/rfq/api/rest/server.go
|
bumping |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
services/rfq/api/rest/server.go (1)
Line range hint
328-355: Logging and printing statements inrecordLatestQuoteAgeshould be reviewed for production readiness.The method
recordLatestQuoteAgecontains logging and printing statements that may not be suitable for a production environment due to potential performance impacts or unnecessary output. Consider revising these statements or implementing a more sophisticated logging framework that can be dynamically adjusted based on the environment.- logger.Warnf("recordLatestQuoteAge") - fmt.Printf("recording latest quote age") + if debugEnabled { + logger.Warnf("recordLatest QuoteAge") + fmt.Printf("recording latest quote age") + }This change ensures that logs are only produced when debugging is explicitly enabled, reducing potential clutter and performance overhead in production.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/api/rest/server.go (5 hunks)
Additional comments not posted (1)
services/rfq/api/rest/server.go (1)
129-137: Verify the correct method signature for Prometheus metrics setup.The method call to
q.meter.Float64ObservableGaugeandq.meter.RegisterCallbackin the Prometheus metrics setup appears to be potentially incorrect. Please ensure that the method signatures match the expected patterns in OpenTelemetry or Prometheus libraries.
| @@ -39,6 +41,7 @@ | |||
| engine *gin.Engine | |||
| omnirpcClient omniClient.RPCClient | |||
| handler metrics.Handler | |||
| meter metric.Meter | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure all references to handler are updated to meter.
The search results show multiple occurrences of handler in the file services/rfq/api/rest/server.go. These references should be reviewed and updated to meter if necessary:
services/rfq/api/rest/server.go:handler metrics.Handlerservices/rfq/api/rest/server.go:handler metrics.Handler,services/rfq/api/rest/server.go:if handler == nil {services/rfq/api/rest/server.go:handler: handler,services/rfq/api/rest/server.go:meter: handler.Meter(meterName),services/rfq/api/rest/server.go:if r.handler == nil || r.latestQuoteAgeGauge == nil {
Please review these lines and update the references to meter where applicable.
Analysis chain
Ensure all references to handler are updated to meter.
The replacement of the handler field with meter in the QuoterAPIServer struct has been noted. It is crucial to ensure that all references in the codebase to the old handler field are updated accordingly to prevent any runtime errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all references to `handler` have been updated to `meter`.
# Test: Search for the usage of `handler`. Expect: No occurrences of `handler` outside of historical comments or documentation.
rg --type go $'handler'
Length of output: 64290
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced Prometheus metrics in the quoting API server for better monitoring.
- Exposed Prometheus metrics via a new endpoint.
- Set up a gauge to record the age of the latest quote.
- Added logging for enhanced traceability of the latest quote age.
1 file(s) reviewed, 1 comment(s)
services/rfq/api/rest/server.go
Outdated
| @@ -307,6 +328,9 @@ func (r *QuoterAPIServer) PutRelayAck(c *gin.Context) { | |||
| } | |||
|
|
|||
| func (r *QuoterAPIServer) recordLatestQuoteAge(ctx context.Context, observer metric.Observer) (err error) { | |||
| logger.Warnf("recordLatestQuoteAge") | |||
There was a problem hiding this comment.
Consider using a more descriptive log level than 'Warn' for regular operations.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/api/rest/server.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/api/rest/server.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced Prometheus metrics to monitor the latest quote age in
QuoterAPIServer - Enhanced
recordLatestQuoteAgemethod with logging and printing for better monitoring - Removed redundant error handling logic in
NewInventoryManagerfunction for efficiency - Cleaned up
recordLatestQuoteAgemethod by removing redundant logging and printing statements
1 file(s) reviewed, no comment(s)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/api/rest/server.go (4 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/api/rest/server.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced metrics tracking with Prometheus integration
- Improved error handling in
NewInventoryManager - Refactored nonce and pending transaction handling for different chain IDs
- Replaced various metrics registration methods with direct metrics recording
- Optimized metrics setup for better performance tracking
1 file(s) reviewed, no comment(s)
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- ethergo/submitter/chain_queue.go (6 hunks)
- ethergo/submitter/submitter.go (6 hunks)
Additional context used
GitHub Check: codecov/patch
ethergo/submitter/chain_queue.go
[warning] 150-153: ethergo/submitter/chain_queue.go#L150-L153
Added lines #L150 - L153 were not covered by tests
[warning] 155-157: ethergo/submitter/chain_queue.go#L155-L157
Added lines #L155 - L157 were not covered by tests
[warning] 159-160: ethergo/submitter/chain_queue.go#L159-L160
Added lines #L159 - L160 were not covered by tests
[warning] 163-163: ethergo/submitter/chain_queue.go#L163
Added line #L163 was not covered by tests
[warning] 166-169: ethergo/submitter/chain_queue.go#L166-L169
Added lines #L166 - L169 were not covered by tests
[warning] 171-173: ethergo/submitter/chain_queue.go#L171-L173
Added lines #L171 - L173 were not covered by tests
[warning] 175-177: ethergo/submitter/chain_queue.go#L175-L177
Added lines #L175 - L177 were not covered by testsethergo/submitter/submitter.go
[warning] 126-127: ethergo/submitter/submitter.go#L126-L127
Added lines #L126 - L127 were not covered by tests
[warning] 146-147: ethergo/submitter/submitter.go#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 151-152: ethergo/submitter/submitter.go#L151-L152
Added lines #L151 - L152 were not covered by tests
[warning] 156-157: ethergo/submitter/submitter.go#L156-L157
Added lines #L156 - L157 were not covered by tests
[warning] 161-162: ethergo/submitter/submitter.go#L161-L162
Added lines #L161 - L162 were not covered by tests
GitHub Check: Lint (ethergo)
ethergo/submitter/chain_queue.go
[failure] 150-150:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 166-166:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
Additional comments not posted (2)
ethergo/submitter/chain_queue.go (1)
136-136: Potential race condition in updatingnumPendingTxes.The assignment to
numPendingTxesis not protected by a mutex, which could lead to race conditions if multiple goroutines modify it concurrently. Consider using a mutex to synchronize access tonumPendingTxes.ethergo/submitter/submitter.go (1)
80-80: Unused fields and method intxSubmitterImpl.The fields
numPendingGaugeandnonceGauge, and the methodsetupMetricsare flagged as unused. If these are not expected to be used in the future, consider removing them to clean up the codebase.Also applies to: 82-82, 143-143
ethergo/submitter/chain_queue.go
Outdated
| nonceHist, err := meter.Int64Histogram("current_nonce") | ||
| if err != nil { | ||
| return fmt.Errorf("error creating nonce histogram: %w", err) | ||
| func (t *txSubmitterImpl) recordNumPending(ctx context.Context, observer metric.Observer) (err error) { |
There was a problem hiding this comment.
Renaming unused parameter 'ctx' to underscore.
The parameter ctx is not used within the recordNumPending and recordNonces methods. According to static analysis, it should be renamed to _ to indicate it's intentionally unused.
- func (t *txSubmitterImpl) recordNumPending(ctx context.Context, observer metric.Observer) (err error) {
+ func (t *txSubmitterImpl) recordNumPending(_ context.Context, observer metric.Observer) (err error) {
- func (t *txSubmitterImpl) recordNonces(ctx context.Context, observer metric.Observer) (err error) {
+ func (t *txSubmitterImpl) recordNonces(_ context.Context, observer metric.Observer) (err error) {Also applies to: 166-166
Tools
GitHub Check: codecov/patch
[warning] 150-153: ethergo/submitter/chain_queue.go#L150-L153
Added lines #L150 - L153 were not covered by tests
GitHub Check: Lint (ethergo)
[failure] 150-150:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
ethergo/submitter/chain_queue.go
Outdated
| func (t *txSubmitterImpl) recordNumPending(ctx context.Context, observer metric.Observer) (err error) { | ||
| if t.metrics == nil || t.numPendingGauge == nil || t.numPendingTxes == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add test coverage for newly added lines.
Several lines related to metric recording and observer operations lack test coverage. Consider adding unit tests to ensure these functionalities work as expected and to maintain a high code coverage standard.
Also applies to: 155-157, 159-160, 163-163, 166-169, 171-173, 175-177
Tools
GitHub Check: codecov/patch
[warning] 150-153: ethergo/submitter/chain_queue.go#L150-L153
Added lines #L150 - L153 were not covered by tests
GitHub Check: Lint (ethergo)
[failure] 150-150:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
| return fmt.Errorf("could not setup metrics: %w", err) | ||
| } |
There was a problem hiding this comment.
Add test coverage for newly added lines.
Several lines related to metric setup and error handling lack test coverage. Consider adding unit tests to ensure these functionalities work as expected and to maintain a high code coverage standard.
Also applies to: 146-147, 151-152, 156-157, 161-162
Tools
GitHub Check: codecov/patch
[warning] 126-127: ethergo/submitter/submitter.go#L126-L127
Added lines #L126 - L127 were not covered by tests
| "go.opentelemetry.io/otel/trace" | ||
| "golang.org/x/sync/errgroup" | ||
| ) | ||
|
|
||
| var logger = log.Logger("ethergo-submitter") | ||
|
|
||
| const meterName = "github.com/synapsecns/sanguine/services/rfq/api/rest" |
There was a problem hiding this comment.
Consider encapsulating meterName within txSubmitterImpl.
The meterName constant is globally defined but only used within the txSubmitterImpl struct. Consider moving it inside the struct to encapsulate it properly and to enhance readability and maintainability.
- const meterName = "github.com/synapsecns/sanguine/services/rfq/api/rest"
+ // Now inside the txSubmitterImpl struct.Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced metrics tracking for transaction submissions, including pending transactions and current nonces.
- Improved internal structure for handling and recording metrics.
- Revamped metrics handling for quote requests in the
Managerstruct. - Removed redundant error handling in the
NewInventoryManagerfunction.
2 file(s) reviewed, no comment(s)
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- ethergo/submitter/chain_queue.go (6 hunks)
- services/rfq/relayer/quoter/quoter.go (6 hunks)
Additional context used
GitHub Check: codecov/patch
ethergo/submitter/chain_queue.go
[warning] 151-153: ethergo/submitter/chain_queue.go#L151-L153
Added lines #L151 - L153 were not covered by tests
[warning] 155-160: ethergo/submitter/chain_queue.go#L155-L160
Added lines #L155 - L160 were not covered by tests
[warning] 163-163: ethergo/submitter/chain_queue.go#L163
Added line #L163 was not covered by tests
[warning] 167-169: ethergo/submitter/chain_queue.go#L167-L169
Added lines #L167 - L169 were not covered by tests
[warning] 171-177: ethergo/submitter/chain_queue.go#L171-L177
Added lines #L171 - L177 were not covered by testsservices/rfq/relayer/quoter/quoter.go
[warning] 126-127: services/rfq/relayer/quoter/quoter.go#L126-L127
Added lines #L126 - L127 were not covered by tests
[warning] 131-132: services/rfq/relayer/quoter/quoter.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 402-402: services/rfq/relayer/quoter/quoter.go#L402
Added line #L402 was not covered by tests
[warning] 406-414: services/rfq/relayer/quoter/quoter.go#L406-L414
Added lines #L406 - L414 were not covered by tests
[warning] 416-429: services/rfq/relayer/quoter/quoter.go#L416-L429
Added lines #L416 - L429 were not covered by tests
ethergo/submitter/chain_queue.go
Outdated
| func (t *txSubmitterImpl) recordNumPending(_ context.Context, observer metric.Observer) (err error) { | ||
| if t.metrics == nil || t.numPendingGauge == nil || t.numPendingTxes == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add test coverage for newly added lines.
Several lines related to metric recording and observer operations lack test coverage. Consider adding unit tests to ensure these functionalities work as expected and to maintain a high code coverage standard.
Would you like me to help draft some unit tests for these new functionalities?
Tools
GitHub Check: codecov/patch
[warning] 151-153: ethergo/submitter/chain_queue.go#L151-L153
Added lines #L151 - L153 were not covered by tests
ethergo/submitter/chain_queue.go
Outdated
| func (t *txSubmitterImpl) recordNonces(_ context.Context, observer metric.Observer) (err error) { | ||
| if t.metrics == nil || t.nonceGauge == nil || t.currentNonces == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add test coverage for newly added lines.
Several lines related to metric recording and observer operations lack test coverage. Consider adding unit tests to ensure these functionalities work as expected and to maintain a high code coverage standard.
Would you like me to help draft some unit tests for these new functionalities?
Tools
GitHub Check: codecov/patch
[warning] 167-169: ethergo/submitter/chain_queue.go#L167-L169
Added lines #L167 - L169 were not covered by tests
ethergo/submitter/chain_queue.go
Outdated
| if registerErr != nil { | ||
| span.AddEvent("could not register pending txes", trace.WithAttributes(attribute.String("error", registerErr.Error()))) | ||
| } | ||
| t.numPendingTxes[uint32(chainID.Int64())] = len(cq.reprocessQueue) |
There was a problem hiding this comment.
Potential race condition in updating numPendingTxes.
The assignment to numPendingTxes is not protected by a mutex, which could lead to race conditions if multiple goroutines modify it concurrently. Consider using a mutex to synchronize access to numPendingTxes.
- t.numPendingTxes[uint32(chainID.Int64())] = len(cq.reprocessQueue)
+ t.numPendingTxesMux.Lock()
+ t.numPendingTxes[uint32(chainID.Int64())] = len(cq.reprocessQueue)
+ t.numPendingTxesMux.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.numPendingTxes[uint32(chainID.Int64())] = len(cq.reprocessQueue) | |
| t.numPendingTxesMux.Lock() | |
| t.numPendingTxes[uint32(chainID.Int64())] = len(cq.reprocessQueue) | |
| t.numPendingTxesMux.Unlock() |
ethergo/submitter/chain_queue.go
Outdated
| if registerErr != nil { | ||
| span.AddEvent("could not register nonce", trace.WithAttributes(attribute.String("error", registerErr.Error()))) | ||
| } | ||
| t.currentNonces[uint32(chainID.Int64())] = currentNonce |
There was a problem hiding this comment.
Consider adding a mutex for thread-safe updates to currentNonces.
Direct assignment to currentNonces without synchronization could lead to race conditions in a concurrent environment. Consider using a mutex to protect these writes.
- t.currentNonces[uint32(chainID.Int64())] = currentNonce
+ t.currentNoncesMux.Lock()
+ t.currentNonces[uint32(chainID.Int64())] = currentNonce
+ t.currentNoncesMux.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.currentNonces[uint32(chainID.Int64())] = currentNonce | |
| t.currentNoncesMux.Lock() | |
| t.currentNonces[uint32(chainID.Int64())] = currentNonce | |
| t.currentNoncesMux.Unlock() |
| return nil, fmt.Errorf("error creating quote amount gauge: %w", err) | ||
| } |
There was a problem hiding this comment.
Add test coverage for error handling in metric setup.
The error handling branches during metric setup are not covered by tests. Adding unit tests for these cases would ensure robustness and prevent potential runtime issues.
Would you like me to assist in creating these tests?
Tools
GitHub Check: codecov/patch
[warning] 126-127: services/rfq/relayer/quoter/quoter.go#L126-L127
Added lines #L126 - L127 were not covered by tests
| return nil, fmt.Errorf("could not register callback: %w", err) | ||
| } |
There was a problem hiding this comment.
Add test coverage for error handling in metric callback registration.
The error handling branches during the registration of metric callbacks are not covered by tests. It is crucial to test these to ensure that the metric system behaves as expected under all conditions.
Would you like me to assist in creating these tests?
Tools
GitHub Check: codecov/patch
[warning] 131-132: services/rfq/relayer/quoter/quoter.go#L131-L132
Added lines #L131 - L132 were not covered by tests
| if m.meter == nil || m.quoteAmountHist == nil { | ||
| // recordQuoteAmounts records the latest quotes from the relayer. | ||
| func (m *Manager) recordQuoteAmounts(_ context.Context, observer metric.Observer) (err error) { | ||
| if m.meter == nil || m.quoteAmountGauge == nil || m.currentQuotes == nil { |
There was a problem hiding this comment.
Add test coverage for the recordQuoteAmounts method.
The recordQuoteAmounts method lacks test coverage, especially for its error handling paths. Ensuring this method is well-tested is critical for the reliability of metric recording.
Would you like me to help draft some unit tests for this method?
Tools
GitHub Check: codecov/patch
[warning] 402-402: services/rfq/relayer/quoter/quoter.go#L402
Added line #L402 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (7)
agents/go.sumis excluded by!**/*.sumcontrib/opbot/go.sumis excluded by!**/*.sumcontrib/promexporter/go.sumis excluded by!**/*.sumethergo/go.sumis excluded by!**/*.sumservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- agents/go.mod (1 hunks)
- contrib/opbot/go.mod (1 hunks)
- contrib/promexporter/go.mod (1 hunks)
- ethergo/go.mod (1 hunks)
- ethergo/submitter/chain_queue.go (5 hunks)
- ethergo/submitter/submitter.go (7 hunks)
- services/cctp-relayer/go.mod (1 hunks)
- services/omnirpc/go.mod (1 hunks)
- services/rfq/go.mod (1 hunks)
Files skipped from review due to trivial changes (7)
- agents/go.mod
- contrib/opbot/go.mod
- contrib/promexporter/go.mod
- ethergo/go.mod
- services/cctp-relayer/go.mod
- services/omnirpc/go.mod
- services/rfq/go.mod
Files skipped from review as they are similar to previous changes (1)
- ethergo/submitter/submitter.go
| func (t *txSubmitterImpl) recordNonces(_ context.Context, observer metric.Observer) (err error) { | ||
| if t.metrics == nil || t.nonceGauge == nil || t.currentNonces == nil { | ||
| return nil | ||
| } | ||
| attributes := attribute.NewSet( | ||
| attribute.Int(metrics.ChainID, chainID), | ||
| attribute.String("wallet", t.signer.Address().Hex()), | ||
| ) | ||
| nonceHist.Record(ctx, int64(nonce), metric.WithAttributeSet(attributes)) | ||
|
|
||
| t.currentNonces.Range(func(chainID uint32, nonce uint64) bool { | ||
| opts := metric.WithAttributes( | ||
| attribute.Int(metrics.ChainID, int(chainID)), | ||
| attribute.String("wallet", t.signer.Address().Hex()), | ||
| ) | ||
| observer.ObserveInt64(t.nonceGauge, int64(nonce), opts) | ||
| return true | ||
| }) | ||
|
|
There was a problem hiding this comment.
Add test coverage for newly added lines.
Several lines related to metric recording and observer operations lack test coverage. Consider adding unit tests to ensure these functionalities work as expected and to maintain a high code coverage standard.
Would you like me to help draft some unit tests for these new functionalities?
Enhance error handling for incomplete nonce metrics setup.
Like the previous method, consider providing explicit error handling or logging for cases where the nonce metrics setup is incomplete.
- return nil
+ return fmt.Errorf("nonce metrics setup is incomplete")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (t *txSubmitterImpl) recordNonces(_ context.Context, observer metric.Observer) (err error) { | |
| if t.metrics == nil || t.nonceGauge == nil || t.currentNonces == nil { | |
| return nil | |
| } | |
| attributes := attribute.NewSet( | |
| attribute.Int(metrics.ChainID, chainID), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| nonceHist.Record(ctx, int64(nonce), metric.WithAttributeSet(attributes)) | |
| t.currentNonces.Range(func(chainID uint32, nonce uint64) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int(metrics.ChainID, int(chainID)), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| observer.ObserveInt64(t.nonceGauge, int64(nonce), opts) | |
| return true | |
| }) | |
| func (t *txSubmitterImpl) recordNonces(_ context.Context, observer metric.Observer) (err error) { | |
| if t.metrics == nil || t.nonceGauge == nil || t.currentNonces == nil { | |
| return fmt.Errorf("nonce metrics setup is incomplete") | |
| } | |
| t.currentNonces.Range(func(chainID uint32, nonce uint64) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int(metrics.ChainID, int(chainID)), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| observer.ObserveInt64(t.nonceGauge, int64(nonce), opts) | |
| return true | |
| }) |
| if registerErr != nil { | ||
| span.AddEvent("could not register nonce", trace.WithAttributes(attribute.String("error", registerErr.Error()))) | ||
| } | ||
| t.currentNonces.Set(uint32(chainID.Int64()), currentNonce) |
There was a problem hiding this comment.
Consider using thread-safe updates for currentNonces.
Direct assignment to currentNonces without synchronization could lead to race conditions in a concurrent environment. Consider using a mutex to protect these writes.
- t.currentNonces.Set(uint32(chainID.Int64()), currentNonce)
+ t.currentNoncesMux.Lock()
+ t.currentNonces.Set(uint32(chainID.Int64()), currentNonce)
+ t.currentNoncesMux.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.currentNonces.Set(uint32(chainID.Int64()), currentNonce) | |
| t.currentNoncesMux.Lock() | |
| t.currentNonces.Set(uint32(chainID.Int64()), currentNonce) | |
| t.currentNoncesMux.Unlock() |
| if registerErr != nil { | ||
| span.AddEvent("could not register pending txes", trace.WithAttributes(attribute.String("error", registerErr.Error()))) | ||
| } | ||
| t.numPendingTxes.Set(uint32(chainID.Int64()), len(cq.reprocessQueue)) |
There was a problem hiding this comment.
Potential race condition in updating numPendingTxes.
The assignment to numPendingTxes is not protected by a mutex, which could lead to race conditions if multiple goroutines modify it concurrently. Consider using a mutex to synchronize access to numPendingTxes.
- t.numPendingTxes.Set(uint32(chainID.Int64()), len(cq.reprocessQueue))
+ t.numPendingTxesMux.Lock()
+ t.numPendingTxes.Set(uint32(chainID.Int64()), len(cq.reprocessQueue))
+ t.numPendingTxesMux.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.numPendingTxes.Set(uint32(chainID.Int64()), len(cq.reprocessQueue)) | |
| t.numPendingTxesMux.Lock() | |
| t.numPendingTxes.Set(uint32(chainID.Int64()), len(cq.reprocessQueue)) | |
| t.numPendingTxesMux.Unlock() |
| func (t *txSubmitterImpl) recordNumPending(_ context.Context, observer metric.Observer) (err error) { | ||
| if t.metrics == nil || t.numPendingGauge == nil || t.numPendingTxes == nil { | ||
| return nil | ||
| } | ||
| return meter | ||
|
|
||
| t.numPendingTxes.Range(func(chainID uint32, numPending int) bool { | ||
| opts := metric.WithAttributes( | ||
| attribute.Int(metrics.ChainID, int(chainID)), | ||
| attribute.String("wallet", t.signer.Address().Hex()), | ||
| ) | ||
| observer.ObserveInt64(t.numPendingGauge, int64(numPending), opts) | ||
|
|
||
| return true | ||
| }) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Ensure proper error handling for metrics setup.
Currently, the method exits without any error if the metrics setup is not complete. Consider logging or handling this scenario more explicitly to avoid silent failures in metrics recording.
- return nil
+ return fmt.Errorf("metrics setup is incomplete")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (t *txSubmitterImpl) recordNumPending(_ context.Context, observer metric.Observer) (err error) { | |
| if t.metrics == nil || t.numPendingGauge == nil || t.numPendingTxes == nil { | |
| return nil | |
| } | |
| return meter | |
| t.numPendingTxes.Range(func(chainID uint32, numPending int) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int(metrics.ChainID, int(chainID)), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| observer.ObserveInt64(t.numPendingGauge, int64(numPending), opts) | |
| return true | |
| }) | |
| return nil | |
| func (t *txSubmitterImpl) recordNumPending(_ context.Context, observer metric.Observer) (err error) { | |
| if t.metrics == nil || t.numPendingGauge == nil || t.numPendingTxes == nil { | |
| return fmt.Errorf("metrics setup is incomplete") | |
| } | |
| t.numPendingTxes.Range(func(chainID uint32, numPending int) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int(metrics.ChainID, int(chainID)), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| observer.ObserveInt64(t.numPendingGauge, int64(numPending), opts) | |
| return true | |
| }) | |
| return nil |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced new metrics for tracking pending transactions and current nonces.
- Simplified code for updating data structures and metrics recording.
- Added
stripLinksfunction for input sanitization in OpBot commands. - Updated dependencies in multiple
go.modfiles, includinggithub.com/cornelk/hashmap. - Enhanced error handling and middleware in Screener API.
26 file(s) reviewed, no comment(s)
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Enhanced metrics handling for current nonces and pending transactions
- Updated metrics for quote requests to a gauge-based system
- Improved error handling in inventory manager by removing unnecessary checks
- Added
github.com/cornelk/hashmapv1.0.8 as a new indirect dependency - Added null checks for
respandresp.Bodyinserver_test.goto prevent runtime errors
1 file(s) reviewed, no comment(s)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/relapi/server_test.go (5 hunks)
Additional comments not posted (5)
services/rfq/relayer/relapi/server_test.go (5)
30-33: Good practice to ensure resources are properly releasedAdding checks to ensure
respandresp.Bodyare notnilbefore attempting to close the response body is a good practice to avoid potential nil pointer dereferences.
54-57: Proper resource managementThe addition of nil checks before closing the response body in the deferred function helps prevent runtime panics due to nil dereferences. This is a crucial step in handling HTTP responses properly.
93-96: Consistent resource handling across test casesThe pattern of checking for non-nil
respandresp.Bodybefore closure is consistently applied here, which is good for maintaining uniform error handling and resource management within the test suite.
131-134: Ensuring clean-up in test casesThe consistent application of nil checks before resource release across different tests in this file helps ensure that resources are always properly cleaned up, reducing the risk of resource leaks in tests.
171-171: Consider adding error handling for the response closureWhile it's good to close the response body, consider handling potential errors from
resp.Body.Close()to ensure that all aspects of resource management are covered.- closeErr := resp.Body.Close() + closeErr := resp.Body.Close() + c.NoError(closeErr)
Summary by CodeRabbit
Bug Fixes
Chores
github.com/cornelk/hashmap v1.0.8across multiple modules for better performance and compatibility.Refactor