RFQ Relayer: add request status count metric #2856
Conversation
WalkthroughThese changes introduce metrics recording with OpenTelemetry into the RFQ (Request For Quote) service's relayer component. A new 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: |
3e0dc86
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://04759448.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-status-metric.sanguine-fe.pages.dev |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- services/rfq/relayer/reldb/base/quote.go: Added
GetStatusCountsfunction to retrieve counts of quote requests by status. - services/rfq/relayer/reldb/db.go: Introduced
GetStatusCountsmethod in theReaderinterface for status count metrics. - services/rfq/relayer/service/otel.go: Added
otelRecorderstruct for recording request status counts as metrics. - services/rfq/relayer/service/otel_generated.go: Introduced
iOtelRecorderinterface withRecordStatusCountmethod. - services/rfq/relayer/service/relayer.go: Integrated
otelRecorderinNewRelayerand added periodic recording of status counts inStartfunction.
5 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback for status gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not register callback for status count gauge'
| return nil, fmt.Errorf("could not register callback for status gauge") | |
| return nil, fmt.Errorf("could not register callback for status count gauge") |
services/rfq/relayer/service/otel.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // RecordstatusCounts records the request status count. |
There was a problem hiding this comment.
📚 spelling: Typo in method name: should be 'RecordStatusCount'
| // RecordstatusCounts records the request status count. | |
| func (o *otelRecorder) RecordStatusCount(status, count int) { |
| // RecordStatuses records the request status count. | ||
| RecordStatusCount(status, count int) |
There was a problem hiding this comment.
📚 spelling: Method name in comment should match the method name.
| // RecordStatuses records the request status count. | |
| RecordStatusCount(status, count int) | |
| // RecordStatusCount records the request status count. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Renamed gauge from
statustostatus_count - Updated methods and variables to reflect the new gauge name in
services/rfq/relayer/service/otel.go
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create last block gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not create status count gauge'
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback for status gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not register callback for status count gauge'
services/rfq/relayer/service/otel.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // RecordstatusCounts records the request status count. |
There was a problem hiding this comment.
📚 spelling: Typo in method name: should be 'RecordStatusCount'
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
otelRecorderstruct for status count tracking - Set up gauge metric and callback for status counts
- Implemented
RecordStatusCountmethod for updating counts - Observing and recording status counts via
recordStatusCountsmethod - Renamed gauge from
statustostatus_count
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create last block gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not create status count gauge'
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback for status gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not register callback for status count gauge'
| return nil | ||
| } | ||
|
|
||
| // RecordStatusCounts records the request status count. |
There was a problem hiding this comment.
📚 spelling: Typo in method name: should be 'RecordStatusCount'
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- services/rfq/relayer/reldb/base/quote.go (1 hunks)
- services/rfq/relayer/reldb/db.go (1 hunks)
- services/rfq/relayer/service/otel.go (1 hunks)
- services/rfq/relayer/service/otel_generated.go (1 hunks)
- services/rfq/relayer/service/relayer.go (5 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/relayer/service/otel_generated.go
Additional context used
GitHub Check: Lint (services/rfq)
services/rfq/relayer/service/otel.go
[failure] 68-68:
unnecessary conversion (unconvert)
Additional comments not posted (5)
services/rfq/relayer/service/otel.go (2)
79-82: LGTM!The
RecordStatusCountmethod looks good to me.
1-12: LGTM!The file-level comments and imports look good to me.
services/rfq/relayer/reldb/db.go (1)
51-52: LGTM!The addition of the
GetStatusCountsmethod to theReaderinterface looks good to me.services/rfq/relayer/service/relayer.go (2)
146-150: LGTM!The initialization of the
otelRecorderin theNewRelayerfunction looks good to me.
65-66: LGTM!The addition of the
otelRecorderfield to theRelayerstruct looks good to me.
services/rfq/relayer/service/otel.go
Outdated
| o.statusCounts.Range(func(status int, count int) bool { | ||
| opts := metric.WithAttributes( | ||
| attribute.Int("status", int(status)), |
There was a problem hiding this comment.
Remove unnecessary conversion.
The conversion to int is unnecessary here.
- attribute.Int("status", int(status)),
+ attribute.Int("status", status),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.
| o.statusCounts.Range(func(status int, count int) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int("status", int(status)), | |
| o.statusCounts.Range(func(status int, count int) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int("status", status), |
Tools
GitHub Check: Lint (services/rfq)
[failure] 68-68:
unnecessary conversion (unconvert)
services/rfq/relayer/service/otel.go
Outdated
| func newOtelRecorder(meterHandler metrics.Handler, signer signer.Signer) (_ iOtelRecorder, err error) { | ||
| or := otelRecorder{ | ||
| metrics: meterHandler, | ||
| meter: meterHandler.Meter(meterName), | ||
| statusCounts: hashmap.New[int, int](), | ||
| signer: signer, | ||
| } | ||
|
|
||
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create last block gauge") | ||
| } | ||
|
|
||
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback for status gauge") | ||
| } | ||
|
|
||
| return &or, nil |
There was a problem hiding this comment.
Improve error messages for better debugging.
The error messages in the newOtelRecorder method could be more specific to aid in debugging.
- return nil, fmt.Errorf("could not create last block gauge")
+ return nil, fmt.Errorf("could not create status count gauge: %w", err)
- return nil, fmt.Errorf("could not register callback for status gauge")
+ return nil, fmt.Errorf("could not register callback for status count gauge: %w", err)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 newOtelRecorder(meterHandler metrics.Handler, signer signer.Signer) (_ iOtelRecorder, err error) { | |
| or := otelRecorder{ | |
| metrics: meterHandler, | |
| meter: meterHandler.Meter(meterName), | |
| statusCounts: hashmap.New[int, int](), | |
| signer: signer, | |
| } | |
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create last block gauge") | |
| } | |
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not register callback for status gauge") | |
| } | |
| return &or, nil | |
| func newOtelRecorder(meterHandler metrics.Handler, signer signer.Signer) (_ iOtelRecorder, err error) { | |
| or := otelRecorder{ | |
| metrics: meterHandler, | |
| meter: meterHandler.Meter(meterName), | |
| statusCounts: hashmap.New[int, int](), | |
| signer: signer, | |
| } | |
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create status count gauge: %w", err) | |
| } | |
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not register callback for status count gauge: %w", err) | |
| } | |
| return &or, nil |
| func (r *Relayer) recordMetrics(ctx context.Context) (err error) { | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("could not record metrics: %w", ctx.Err()) | ||
| case <-time.After(defaultMetricsInterval * time.Second): | ||
| statusCounts, err := r.db.GetStatusCounts(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get status counts: %w", err) | ||
| } | ||
| for status, count := range statusCounts { | ||
| r.otelRecorder.RecordStatusCount(int(status.Int()), count) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error messages for better debugging.
The error messages in the recordMetrics method could be more specific to aid in debugging.
- return fmt.Errorf("could not record metrics: %w", ctx.Err())
+ return fmt.Errorf("context done while recording metrics: %w", ctx.Err())
- return fmt.Errorf("could not get status counts: %w", err)
+ return fmt.Errorf("could not get status counts from database: %w", err)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 (r *Relayer) recordMetrics(ctx context.Context) (err error) { | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return fmt.Errorf("could not record metrics: %w", ctx.Err()) | |
| case <-time.After(defaultMetricsInterval * time.Second): | |
| statusCounts, err := r.db.GetStatusCounts(ctx) | |
| if err != nil { | |
| return fmt.Errorf("could not get status counts: %w", err) | |
| } | |
| for status, count := range statusCounts { | |
| r.otelRecorder.RecordStatusCount(int(status.Int()), count) | |
| } | |
| } | |
| } | |
| } | |
| func (r *Relayer) recordMetrics(ctx context.Context) (err error) { | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return fmt.Errorf("context done while recording metrics: %w", ctx.Err()) | |
| case <-time.After(defaultMetricsInterval * time.Second): | |
| statusCounts, err := r.db.GetStatusCounts(ctx) | |
| if err != nil { | |
| return fmt.Errorf("could not get status counts from database: %w", err) | |
| } | |
| for status, count := range statusCounts { | |
| r.otelRecorder.RecordStatusCount(int(status.Int()), count) | |
| } | |
| } | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2856 +/- ##
===================================================
+ Coverage 25.35936% 25.61864% +0.25928%
===================================================
Files 784 769 -15
Lines 56350 55323 -1027
Branches 80 80
===================================================
- Hits 14290 14173 -117
+ Misses 40581 39677 -904
+ Partials 1479 1473 -6
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.
PR Summary
(updates since last review)
- Added
otelRecorderstruct for status count tracking - Initialized gauge metric for status counts
- Registered callback to record status counts
- Implemented
RecordStatusCountmethod - Renamed gauge from
statustostatus_count
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create last block gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not create status count gauge'
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback for status gauge") |
There was a problem hiding this comment.
📚 spelling: Typo in error message: should be 'could not register callback for status count gauge'
| return nil | ||
| } | ||
|
|
||
| // RecordStatusCounts records the request status count. |
There was a problem hiding this comment.
📚 spelling: Typo in method name: should be 'RecordStatusCount'
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/service/otel.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/otel.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
otelRecorderstruct for OpenTelemetry metrics recording (ethergo/listener/otel.go) - Added
RecordLastBlockmethod for metrics recording (ethergo/listener/otel_generated.go) - Enhanced error handling and added
GetStatusCountsmethod (services/rfq/relayer/reldb/base/quote.go) - Added
setRelayRaceLostmethod for status updates (services/rfq/relayer/service/chainindexer.go) - Modified semaphore acquisition logic and added metrics recording (
services/rfq/relayer/service/relayer.go)
6 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added metrics recording for quote request statuses using OpenTelemetry (
services/rfq/relayer/service/relayer.go) - Introduced method to retrieve quote request counts based on status (
services/rfq/relayer/service/relayer.go) - Enhanced
Relayerstruct to include OpenTelemetry metrics recorder (services/rfq/relayer/service/relayer.go) - Periodic recording of metrics for improved monitoring (
services/rfq/relayer/service/relayer.go)
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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/relayer/reldb/base/quote.go (1 hunks)
- services/rfq/relayer/service/relayer.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/reldb/base/quote.go
- services/rfq/relayer/service/relayer.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated
services/rfq/go.modto addgithub.aaakk.us.kg/cornelk/hashmap v1.0.8as a direct dependency - Removed
github.com/cornelk/hashmap v1.0.8from indirect dependencies inservices/rfq/go.mod
No major changes found since last review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added metrics recording functionality using OpenTelemetry to monitor quote request statuses (
services/rfq/relayer/service/otel.go) - Introduced
RecordStatusCountmethod iniOtelRecorderinterface for metrics monitoring (services/rfq/relayer/service/otel_generated.go) - Enhanced
Relayerstruct withotelRecorderfield for periodic metrics recording (services/rfq/relayer/service/relayer.go)
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
services/rfq/relayer/service/otel.go
Outdated
| } | ||
|
|
||
| o.statusCounts.Range(func(status int, count int) bool { | ||
| fmt.Printf("recordStatusCounts with status: %d and count: %d\n", status, count) |
There was a problem hiding this comment.
🪶 style: Remove or replace debug print statements with proper logging.
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| // RecordStatusCounts records the request status count. | ||
| func (o *otelRecorder) RecordStatusCount(status, count int) { | ||
| fmt.Printf("RecordStatusCount with status: %d and count: %d\n", status, count) |
There was a problem hiding this comment.
🪶 style: Remove or replace debug print statements with proper logging.
| if err != nil { | ||
| return fmt.Errorf("could not get status counts: %w", err) | ||
| } | ||
| fmt.Printf("got status counts: %v\n", statusCounts) |
There was a problem hiding this comment.
🪶 style: Consider removing or replacing this debug print statement before merging.
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- services/rfq/go.mod (2 hunks)
- services/rfq/relayer/service/otel.go (1 hunks)
- services/rfq/relayer/service/otel_generated.go (1 hunks)
- services/rfq/relayer/service/relayer.go (5 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/go.mod
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/otel_generated.go
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/service/otel.go
[warning] 40-49: services/rfq/relayer/service/otel.go#L40-L49
Added lines #L40 - L49 were not covered by tests
[warning] 51-51: services/rfq/relayer/service/otel.go#L51
Added line #L51 was not covered by tests
[warning] 53-54: services/rfq/relayer/service/otel.go#L53-L54
Added lines #L53 - L54 were not covered by tests
[warning] 56-56: services/rfq/relayer/service/otel.go#L56
Added line #L56 was not covered by tests
[warning] 58-58: services/rfq/relayer/service/otel.go#L58
Added line #L58 was not covered by tests
[warning] 61-62: services/rfq/relayer/service/otel.go#L61-L62
Added lines #L61 - L62 were not covered by tests
[warning] 64-65: services/rfq/relayer/service/otel.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 67-67: services/rfq/relayer/service/otel.go#L67
Added line #L67 was not covered by tests
[warning] 69-76: services/rfq/relayer/service/otel.go#L69-L76
Added lines #L69 - L76 were not covered by tests
[warning] 78-78: services/rfq/relayer/service/otel.go#L78
Added line #L78 was not covered by tests
[warning] 82-82: services/rfq/relayer/service/otel.go#L82
Added line #L82 was not covered by tests
[warning] 84-84: services/rfq/relayer/service/otel.go#L84
Added line #L84 was not covered by testsservices/rfq/relayer/service/relayer.go
[warning] 146-149: services/rfq/relayer/service/relayer.go#L146-L149
Added lines #L146 - L149 were not covered by tests
[warning] 168-168: services/rfq/relayer/service/relayer.go#L168
Added line #L168 was not covered by tests
[warning] 290-295: services/rfq/relayer/service/relayer.go#L290-L295
Added lines #L290 - L295 were not covered by tests
[warning] 380-389: services/rfq/relayer/service/relayer.go#L380-L389
Added lines #L380 - L389 were not covered by tests
[warning] 391-393: services/rfq/relayer/service/relayer.go#L391-L393
Added lines #L391 - L393 were not covered by tests
Additional comments not posted (2)
services/rfq/relayer/service/otel.go (1)
27-38: LGTM! Struct definition looks good.The
otelRecorderstruct is well-defined and aligns with the purpose of metrics recording.services/rfq/relayer/service/relayer.go (1)
65-66: LGTM! Struct definition looks good.The addition of the
otelRecorderfield is appropriate for recording metrics.
services/rfq/relayer/service/otel.go
Outdated
| func (o *otelRecorder) recordStatusCounts(_ context.Context, observer metric.Observer) (err error) { | ||
| if o.metrics == nil || o.statusCountGauge == nil || o.statusCounts == nil { | ||
| fmt.Println("something is nil") | ||
| return nil | ||
| } | ||
|
|
||
| o.statusCounts.Range(func(status int, count int) bool { | ||
| fmt.Printf("recordStatusCounts with status: %d and count: %d\n", status, count) | ||
| opts := metric.WithAttributes( | ||
| attribute.Int("status", status), | ||
| attribute.String("wallet", o.signer.Address().Hex()), | ||
| ) | ||
| observer.ObserveInt64(o.statusCountGauge, int64(count), opts) | ||
|
|
||
| return true | ||
| }) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Add tests for the recordStatusCounts method.
These lines are not covered by tests. Ensure that this method is tested to verify its functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 61-62: services/rfq/relayer/service/otel.go#L61-L62
Added lines #L61 - L62 were not covered by tests
[warning] 64-65: services/rfq/relayer/service/otel.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 67-67: services/rfq/relayer/service/otel.go#L67
Added line #L67 was not covered by tests
[warning] 69-76: services/rfq/relayer/service/otel.go#L69-L76
Added lines #L69 - L76 were not covered by tests
[warning] 78-78: services/rfq/relayer/service/otel.go#L78
Added line #L78 was not covered by tests
services/rfq/relayer/service/otel.go
Outdated
| func (o *otelRecorder) RecordStatusCount(status, count int) { | ||
| fmt.Printf("RecordStatusCount with status: %d and count: %d\n", status, count) | ||
| o.statusCounts.Set(status, count) | ||
| } |
There was a problem hiding this comment.
Add tests for the RecordStatusCount method.
These lines are not covered by tests. Ensure that this method is tested to verify its functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 82-82: services/rfq/relayer/service/otel.go#L82
Added line #L82 was not covered by tests
[warning] 84-84: services/rfq/relayer/service/otel.go#L84
Added line #L84 was not covered by tests
services/rfq/relayer/service/otel.go
Outdated
| func newOtelRecorder(meterHandler metrics.Handler, signer signer.Signer) (_ iOtelRecorder, err error) { | ||
| or := otelRecorder{ | ||
| metrics: meterHandler, | ||
| meter: meterHandler.Meter(meterName), | ||
| statusCounts: hashmap.New[int, int](), | ||
| signer: signer, | ||
| } | ||
|
|
||
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create status count gauge") | ||
| } | ||
|
|
||
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not register callback for status count gauge") | ||
| } | ||
|
|
||
| return &or, nil |
There was a problem hiding this comment.
Improve error messages and add tests.
The error messages could be more specific to aid in debugging. Additionally, these lines are not covered by tests.
- return nil, fmt.Errorf("could not create status count gauge")
+ return nil, fmt.Errorf("could not create status count gauge: %w", err)
- return nil, fmt.Errorf("could not register callback for status count gauge")
+ return nil, fmt.Errorf("could not register callback for status count gauge: %w", err)Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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 newOtelRecorder(meterHandler metrics.Handler, signer signer.Signer) (_ iOtelRecorder, err error) { | |
| or := otelRecorder{ | |
| metrics: meterHandler, | |
| meter: meterHandler.Meter(meterName), | |
| statusCounts: hashmap.New[int, int](), | |
| signer: signer, | |
| } | |
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create status count gauge") | |
| } | |
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not register callback for status count gauge") | |
| } | |
| return &or, nil | |
| func newOtelRecorder(meterHandler metrics.Handler, signer signer.Signer) (_ iOtelRecorder, err error) { | |
| or := otelRecorder{ | |
| metrics: meterHandler, | |
| meter: meterHandler.Meter(meterName), | |
| statusCounts: hashmap.New[int, int](), | |
| signer: signer, | |
| } | |
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create status count gauge: %w", err) | |
| } | |
| _, err = or.meter.RegisterCallback(or.recordStatusCounts, or.statusCountGauge) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not register callback for status count gauge: %w", err) | |
| } | |
| return &or, nil | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 40-49: services/rfq/relayer/service/otel.go#L40-L49
Added lines #L40 - L49 were not covered by tests
[warning] 51-51: services/rfq/relayer/service/otel.go#L51
Added line #L51 was not covered by tests
[warning] 53-54: services/rfq/relayer/service/otel.go#L53-L54
Added lines #L53 - L54 were not covered by tests
[warning] 56-56: services/rfq/relayer/service/otel.go#L56
Added line #L56 was not covered by tests
[warning] 58-58: services/rfq/relayer/service/otel.go#L58
Added line #L58 was not covered by tests
| func (r *Relayer) recordMetrics(ctx context.Context) (err error) { | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("could not record metrics: %w", ctx.Err()) | ||
| case <-time.After(defaultMetricsInterval * time.Second): | ||
| statusCounts, err := r.db.GetStatusCounts(ctx, reldb.Seen, reldb.NotEnoughInventory, reldb.CommittedPending, reldb.CommittedConfirmed, reldb.RelayStarted, reldb.RelayCompleted, reldb.ProvePosting, reldb.ProvePosted, reldb.ClaimPending) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get status counts: %w", err) | ||
| } | ||
| fmt.Printf("got status counts: %v\n", statusCounts) | ||
| for status, count := range statusCounts { | ||
| r.otelRecorder.RecordStatusCount(int(status.Int()), count) | ||
| } |
There was a problem hiding this comment.
LGTM! But add tests for the recordMetrics method.
The method implementation is well-structured and aligns with its purpose. However, these lines are not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 380-389: services/rfq/relayer/service/relayer.go#L380-L389
Added lines #L380 - L389 were not covered by tests
[warning] 391-393: services/rfq/relayer/service/relayer.go#L391-L393
Added lines #L391 - L393 were not covered by tests
| otelRecorder, err := newOtelRecorder(metricHandler, sg) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get otel recorder: %w", err) | ||
| } |
There was a problem hiding this comment.
Improve error messages and add tests.
The error messages could be more specific to aid in debugging. Additionally, these lines are not covered by tests.
- return nil, fmt.Errorf("could not get otel recorder: %w", err)
+ return nil, fmt.Errorf("could not initialize otel recorder: %w", err)Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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.
| otelRecorder, err := newOtelRecorder(metricHandler, sg) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get otel recorder: %w", err) | |
| } | |
| otelRecorder, err := newOtelRecorder(metricHandler, sg) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not initialize otel recorder: %w", err) | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 146-149: services/rfq/relayer/service/relayer.go#L146-L149
Added lines #L146 - L149 were not covered by tests
| g.Go(func() error { | ||
| err = r.recordMetrics(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not record metrics: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Improve error messages and add tests.
The error messages could be more specific to aid in debugging. Additionally, these lines are not covered by tests.
- return fmt.Errorf("could not record metrics: %w", err)
+ return fmt.Errorf("error recording metrics: %w", err)Do you want me to generate the unit testing code or open a GitHub issue to track this task?
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.
| g.Go(func() error { | |
| err = r.recordMetrics(ctx) | |
| if err != nil { | |
| return fmt.Errorf("could not record metrics: %w", err) | |
| } | |
| return nil | |
| g.Go(func() error { | |
| err = r.recordMetrics(ctx) | |
| if err != nil { | |
| return fmt.Errorf("error recording metrics: %w", err) | |
| } | |
| return nil |
Tools
GitHub Check: codecov/patch
[warning] 290-295: services/rfq/relayer/service/relayer.go#L290-L295
Added lines #L290 - L295 were not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated
docs/bridge/docs/rfq/Relayer/Relayer.mdto include newstatus_countmetric for trackingQuoteRequestStatus. - Added a new panel in
services/rfq/relayer/dashboards/dashboard.jsonto display non-terminalQuoteRequestStatuscounts. - Introduced OpenTelemetry support in
services/rfq/relayer/service/otel.gofor recording status count metrics. - Enhanced
services/rfq/relayer/service/relayer.gowith OpenTelemetry metrics recording for quote request statuses.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- docs/bridge/docs/rfq/Relayer/Relayer.md (1 hunks)
- services/rfq/relayer/dashboards/dashboard.json (13 hunks)
- services/rfq/relayer/service/otel.go (1 hunks)
- services/rfq/relayer/service/relayer.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/service/otel.go
- services/rfq/relayer/service/relayer.go
Additional comments not posted (8)
services/rfq/relayer/dashboards/dashboard.json (7)
21-21: Verify the uniqueness of the dashboard ID.Ensure that the updated ID
770does not conflict with other existing dashboards.
28-28: LGTM!The datasource
uidis correctly set toprometheus.
54-54: LGTM!The datasource
uidis correctly set toprometheus.
133-133: LGTM!The datasource
uidis correctly set toprometheus.
149-149: LGTM!The datasource
uidis correctly set toprometheus.
418-418: LGTM!The datasource
uidis correctly set toprometheus.
Line range hint
431-543:
LGTM!The new panel configuration to display the count of non-terminal
QuoteRequestStatusvalues over time is correctly set up.docs/bridge/docs/rfq/Relayer/Relayer.md (1)
234-234: LGTM!The documentation correctly describes the new
status_countmetric and its purpose.
services/rfq/relayer/service/otel.go
Outdated
|
|
||
| or.statusCountGauge, err = or.meter.Int64ObservableGauge("status_count") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create status count gauge") |
There was a problem hiding this comment.
error should be wrapped here (e.g. fmt.Errorf("could not create status count gauge: %w") but in cases where it's not it's idiomatic to use errors.New() instead of fmt.Errorf
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added status count metric to
services/rfq/relayer/service/otel.go - Registered callback for recording status counts in
otelRecorder - Enhanced relayer dashboard in
services/rfq/relayer/dashboards/dashboard.jsonwith new panels - Updated
docs/bridge/docs/rfq/Relayer/Relayer.mdto includestatus_countmetric
No major changes found since last review.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| func (o *otelRecorder) recordStatusCounts(_ context.Context, observer metric.Observer) (err error) { | ||
| if o.metrics == nil || o.statusCountGauge == nil || o.statusCounts == nil { | ||
| return nil |
There was a problem hiding this comment.
🪶 style: Consider logging an error or warning if any of these conditions are true.
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/service/otel.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/otel.go
Summary by CodeRabbit
New Features
Documentation
QuoteRequestStatusvalues over time.Bug Fixes
55ed21b: docs preview link
c827962: docs preview link