add metrics for alerting #2806
add metrics for alerting #2806
Conversation
This reverts commit 3a28511.
WalkthroughThe recent code changes involve various updates, including changes to import paths, function parameters, and dependencies. Key adjustments include the switch from 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: |
661c235
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3f50ebb5.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-http-capture-test.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2806 +/- ##
===================================================
- Coverage 25.59422% 25.55824% -0.03598%
===================================================
Files 758 765 +7
Lines 54399 54726 +327
Branches 80 80
===================================================
+ Hits 13923 13987 +64
- Misses 39044 39304 +260
- Partials 1432 1435 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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
Outside diff range and nitpick comments (2)
core/metrics/instrumentation/httpcapture/http.go (2)
Line range hint
10-10: Parameter validation in NewCaptureTransportAdd checks to ensure that neither
underlyingTransportnorhandlerarenilto prevent runtime panics.+ if underlyingTransport == nil || handler == nil { + return nil, errors.New("invalid parameters: transport and handler must not be nil") + }
Line range hint
70-70: Optimize body capture logic in RoundTripThe current implementation reads the request and response bodies unconditionally. Consider adding a configuration option to toggle this behavior based on the needs of the application to reduce overhead.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (17)
- contrib/promexporter/exporters/exporter.go (2 hunks)
- contrib/promexporter/go.mod (1 hunks)
- contrib/promexporter/internal/gql/explorer/contrib/main.go (1 hunks)
- core/ginhelper/export_test.go (1 hunks)
- core/ginhelper/server.go (1 hunks)
- core/ginhelper/testing.go (2 hunks)
- core/ginhelper/wait_test.go (3 hunks)
- core/go.mod (1 hunks)
- core/metrics/base.go (1 hunks)
- core/metrics/instrumentation/httpcapture/http.go (2 hunks)
- core/metrics/instrumentation/httpcapture/http_test.go (1 hunks)
- core/metrics/metrics.go (1 hunks)
- core/metrics/mocks/handler.go (1 hunks)
- core/metrics/tester.go (2 hunks)
- ethergo/client/capture_client.go (2 hunks)
- ethergo/client/client_test.go (3 hunks)
- services/explorer/api/server.go (3 hunks)
Files skipped from review due to trivial changes (4)
- contrib/promexporter/go.mod
- core/ginhelper/server.go
- core/go.mod
- core/metrics/metrics.go
Additional comments not posted (21)
core/ginhelper/export_test.go (3)
7-7: Variable Definition ApprovedThe
DefaultServerTimeoutvariable correctly referencesserverStartTimeout, ensuring consistency in timeout settings across tests.
9-12: Consider Thread Safety for Timeout ResetThe
ResetServerTimeoutfunction operates correctly. However, if used in a concurrent environment, consider making access toserverStartTimeoutthread-safe.
14-17: Validate Timeout ValuesThe
SetServerTimeoutfunction effectively sets the server timeout. Consider adding validation for thetimeoutparameter to ensure it's not negative, which could cause runtime errors.core/ginhelper/testing.go (1)
Line range hint
13-41: Enhance Error Handling inWaitForStartThe
WaitForStartfunction is well-structured with appropriate context management and retry logic. Consider enhancing error messages to provide more context about the failure, especially in network operations.ethergo/client/capture_client.go (1)
25-25: Proper Use of Handler in Transport ConfigurationThe
newCaptureClientfunction correctly passes thehandlertoNewCaptureTransport. Ensure thathandleris not null before this call to avoid runtime errors.core/ginhelper/wait_test.go (3)
33-35: Good Practice: Resetting State in TearDownThe
TearDownTestfunction correctly resets the server timeout after each test, ensuring test isolation and reliability.
37-44: Enhance Assertions in Failure Test CaseIn
TestWaitForStartFail, consider explicitly checking the type of error returned byWaitForStartto ensure it's a timeout error. This will make the test more robust and clear.
Line range hint
48-62: Ensure Proper Cleanup After Successful Test
TestWaitForStartSucceedeffectively tests the successful scenario. Consider adding logic to stop the server after the test to prevent resource leakage.contrib/promexporter/internal/gql/explorer/contrib/main.go (1)
58-58: Improve Error Handling in Main FunctionIn the
mainfunction, consider handling specific errors more gracefully instead of usingpanic. This could improve the robustness and user experience of the application.core/metrics/tester.go (2)
67-76: Optimize the SpanAttributeByName function for attribute lookupThe current implementation iterates over all attributes to find a match by name. Consider using a map for attribute storage to improve lookup efficiency if the attributes list can be large.
78-87: Add error handling in SpanHasExceptionThe function assumes the presence of an "exception" event. Consider adding error handling or a fallback mechanism if the "exception" event is not found.
core/metrics/instrumentation/httpcapture/http.go (1)
1-1: Documentation clarity in package httpcaptureThe package comment could be more descriptive about how it captures requests and responses, including details on what is captured and how it is used.
core/metrics/mocks/handler.go (1)
28-226: Comprehensive review of autogenerated mock HandlerThe mock implementations cover a wide range of functionalities. Ensure that the mock behaviors are correctly implemented and align with the expected interfaces and behaviors in tests.
contrib/promexporter/exporters/exporter.go (2)
44-44: Optimize HTTP transport setup in makeHTTPClientThe assignment to
httpClient.Transportis straightforward but does not consider existing transport configurations which might be overridden.
44-44: Documentation for exporter server setupThe function
StartExporterServerlacks detailed comments explaining its steps and the rationale behind its design decisions. Adding detailed comments can improve maintainability.core/metrics/base.go (1)
129-129: Add a blank line before return statement for consistency.The codebase generally separates logical blocks with blank lines. Adding a blank line before the return statement in the
Gin()method would improve readability.+ // Add a blank line for better separation of logic and return statementethergo/client/client_test.go (3)
12-12: Updated import path forhttpcaptureconfirmed.The import path has been updated to reflect the new package structure. This change aligns with the PR's goal to organize
httpcaptureinto its own package.
82-82: Ensure trace span naming consistency.The usage of
httpcapture.RequestSpanNameensures that span names are consistent and standardized across different parts of the application. This is crucial for effective monitoring and debugging.
91-93: Verify correctness of span attributes and events.The usage of
metrics.SpanHasException,metrics.SpanAttributeByName, andmetrics.SpanEventByNameis crucial for detailed trace analysis. However, ensure that these functions are implemented correctly and efficiently to handle the expected data types and scenarios.Verification successful
Verification successful for span attributes and events.
The functions
SpanHasException,SpanAttributeByName, andSpanEventByNameare implemented correctly and efficiently for their intended use in tests.
SpanEventByNameiterates over events to find the first match.SpanAttributeByNameiterates over attributes to find the first match.SpanHasExceptionchecks for an "exception" event.These functions handle the expected data types and scenarios appropriately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and efficiency of span attribute and event extraction functions. # Test: Search for the implementation of these functions in the codebase. Expect: Efficient and correct implementations. rg --type go $'SpanHasException|SpanAttributeByName|SpanEventByName'Length of output: 1275
Script:
#!/bin/bash # Extract the full implementations of SpanHasException, SpanAttributeByName, and SpanEventByName from core/metrics/tester.go # Search for the implementations of the functions rg --type go --context 10 'func SpanHasException|func SpanAttributeByName|func SpanEventByName' core/metrics/tester.goLength of output: 1139
services/explorer/api/server.go (2)
7-7: Approved import path update forhttpcapture.The update correctly reflects the new package structure, ensuring that
httpcapturefunctionalities are properly referenced.
176-176: Approved update to HTTP client transport configuration.The integration of
httpcapture.NewCaptureTransportenhances the server's metrics capabilities. Ensure that this change integrates smoothly with existing metrics collection and monitoring setups.Verification successful
Approved update to HTTP client transport configuration.
The integration of
httpcapture.NewCaptureTransportenhances the server's metrics capabilities. The implementation is consistent across the codebase, with no immediate issues or conflicts detected. Ensure that this change integrates smoothly with existing metrics collection and monitoring setups.
services/explorer/api/server.gocore/metrics/instrumentation/httpcapture/http_test.goethergo/client/capture_client.gocontrib/promexporter/exporters/exporter.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify integration with existing metrics. # Description: Check for any potential conflicts or errors in metrics after integrating the new HTTP transport. # Search for error logs related to HTTP metrics after deployment. rg --type log 'error.*HTTP.*metrics'Length of output: 187
Script:
#!/bin/bash # List all log files in the repository. fd --type f --extension log # Search for error logs related to HTTP metrics without type filtering. rg 'error.*HTTP.*metrics'Length of output: 57
Script:
#!/bin/bash # Search for httpcapture integration in the codebase. rg 'httpcapture.NewCaptureTransport' # Search for the configuration of httpcapture in the codebase. rg 'httpcapture'Length of output: 2633
| // SpanEventByName returns the value of the first event with the given name. | ||
| // it is a helper function for tests. | ||
| func SpanEventByName(stub tracetest.SpanStub, name string) *attribute.Value { | ||
| for _, event := range stub.Events { | ||
| if event.Name == name { | ||
| return &event.Attributes[0].Value | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Optimize the SpanEventByName function for empty events
The current implementation returns the first attribute value of the first event with a matching name. Consider handling cases where the event has no attributes to prevent potential runtime errors.
- return &event.Attributes[0].Value
+ if len(event.Attributes) > 0 {
+ return &event.Attributes[0].Value
+ }
+ return nilCommittable 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.
| // SpanEventByName returns the value of the first event with the given name. | |
| // it is a helper function for tests. | |
| func SpanEventByName(stub tracetest.SpanStub, name string) *attribute.Value { | |
| for _, event := range stub.Events { | |
| if event.Name == name { | |
| return &event.Attributes[0].Value | |
| } | |
| } | |
| return nil | |
| } | |
| // SpanEventByName returns the value of the first event with the given name. | |
| // it is a helper function for tests. | |
| func SpanEventByName(stub tracetest.SpanStub, name string) *attribute.Value { | |
| for _, event := range stub.Events { | |
| if event.Name == name { | |
| if len(event.Attributes) > 0 { | |
| return &event.Attributes[0].Value | |
| } | |
| return nil | |
| } | |
| } | |
| return nil | |
| } |
| func (t *HTTPCaptureSuite) SetupTest() { | ||
| t.TestSuite.SetupTest() | ||
| var logger = log.Logger("httpcapture-test") | ||
| t.server = ginhelper.New(logger) | ||
| t.server.POST("/test", func(c *gin.Context) { | ||
| c.JSON(200, gin.H{"message": "hello world"}) | ||
| }) | ||
|
|
||
| var err error | ||
| t.port, err = freeport.GetFreePort() | ||
| t.Require().NoError(err) | ||
|
|
||
| go func() { | ||
| connection := baseServer.Server{} | ||
| _ = connection.ListenAndServe(t.GetTestContext(), fmt.Sprintf(":%d", t.port), t.server) | ||
| }() | ||
|
|
||
| err = ginhelper.WaitForStart(t.GetTestContext(), t.port) | ||
| t.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
Ensure proper cleanup in HTTPCaptureSuite SetupTest
The current implementation starts an HTTP server but does not ensure it is properly shut down, which can lead to resource leaks in tests.
+ defer t.Cleanup(func() {
+ connection.Close()
+ })Committable suggestion was skipped due to low confidence.
| func (t *HTTPCaptureSuite) TestCaptureTransport() { | ||
| httpClient := new(http.Client) | ||
|
|
||
| mocktracer := metrics.NewTestTracer(t.GetTestContext(), t.T()) | ||
|
|
||
| httpClient.Transport = httpcapture.NewCaptureTransport(httpClient.Transport, mocktracer) | ||
|
|
||
| const testRequestBody = "hi" | ||
| req, err := http.NewRequestWithContext(t.GetTestContext(), http.MethodPost, fmt.Sprintf("http://localhost:%d/test", t.port), strings.NewReader(testRequestBody)) | ||
| t.Require().NoError(err) | ||
|
|
||
| res, err := httpClient.Do(req) | ||
| defer func() { | ||
| if res != nil && res.Body != nil { | ||
| _ = res.Body.Close() | ||
| _ = req.Body.Close() | ||
| } | ||
| }() | ||
| t.Require().NoError(err) | ||
| t.Require().NotNil(res) | ||
|
|
||
| span := mocktracer.GetSpansByName(httpcapture.RequestSpanName)[0] | ||
|
|
||
| reqBody := metrics.SpanEventByName(span, httpcapture.RequestEventName) | ||
| t.Equal(reqBody.AsString(), testRequestBody) | ||
|
|
||
| respBody := metrics.SpanEventByName(span, httpcapture.ResponseEventName) | ||
| t.Equal(respBody.AsString(), `{"message":"hello world"}`) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Enhance error handling in TestCaptureTransport
The current test assumes successful HTTP request and response handling without considering potential network errors or incorrect server responses. Please add tests to handle and assert HTTP errors and response status codes.
- Consider scenarios where the server is unreachable or returns an error.
- Verify that the client handles these errors gracefully and that appropriate status codes are returned.
Analysis chain
Enhance error handling in TestCaptureTransport
The test assumes successful HTTP request and response handling without considering potential network errors or incorrect server responses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure that HTTP server handles errors correctly and returns expected status codes.
# Test: Add error handling tests in HTTPCaptureSuite.
echo "Add tests to handle and assert HTTP errors and response status codes."
Length of output: 148
Script:
#!/bin/bash
# Search for test functions in http_test.go that handle HTTP errors and response status codes
fd 'http_test.go' | xargs rg -A 10 -E 'func \(t \*HTTPCaptureSuite\) Test.*\('
Length of output: 154
Script:
#!/bin/bash
# Search for test functions in http_test.go that handle HTTP errors and response status codes
fd 'http_test.go' | xargs rg -A 10 'func (t \*HTTPCaptureSuite) Test'
Length of output: 69
Script:
#!/bin/bash
# Search for test functions in http_test.go with extended context to capture full test definitions
fd 'http_test.go' | xargs rg -A 20 'func (t \*HTTPCaptureSuite) Test'
Length of output: 69
Script:
#!/bin/bash
# List the contents of http_test.go for manual inspection
fd 'http_test.go' | xargs cat
Length of output: 2392
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)
- core/bytes_test.go (1 hunks)
Additional comments not posted (1)
core/bytes_test.go (1)
79-80: Approved change in loop iteration style.Switching to index-based iteration in test loops is a good practice, particularly when using closures in Go. This prevents the common mistake of capturing the loop variable, which can lead to unexpected behavior when running tests, especially if they were to be run in parallel in the future.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Refactored
httpcaptureinto its own package for better modularity. - Moved
WaitForStarttoginhelperfor improved maintainability. - Added OpenTelemetry Gin metrics middleware.
- Introduced helper functions and mocks for metrics handling in tests.
- Updated import paths and function references across multiple files.
18 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
trajan0x
force-pushed
the
fix/http-capture-test
branch
from
aadfac4
to
514bdd1
Compare
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added metrics for alerting
- Refactored code for modularity and maintainability
- Introduced new middleware for OpenTelemetry metrics in the Gin framework
- Updated import paths and function references
- Enhanced test structure and added mock interfaces for metrics handling
21 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added OpenTelemetry metrics middleware for enhanced observability.
- Refactored middleware usage to support variadic arguments.
- Introduced helper functions and mock interfaces for metrics handling.
- Updated import paths and function references for improved modularity.
- Enhanced test structures with indexed test cases.
21 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings
| func initMetrics() { | ||
| metricExporter, err := stdoutmetric.New() | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
🪶 style: Avoid using panic for error handling in production code. Consider returning an error or logging it instead.
| } | ||
| reader := metric.NewPeriodicReader(metricExporter, metric.WithInterval(1*time.Second)) | ||
|
|
||
| fmt.Println(metricExporter) |
There was a problem hiding this comment.
🪶 style: Remove or replace the debug print statement before deploying to production.
| resource.WithSchemaURL(semconv.SchemaURL), | ||
| ) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
🪶 style: Avoid using panic for error handling in production code. Consider returning an error or logging it instead.
|
|
||
| go func() { | ||
| for { | ||
| _, _ = http.DefaultClient.Get("http://localhost:9199/test/1") |
There was a problem hiding this comment.
🪶 style: Consider adding a delay or condition to avoid overwhelming the server with continuous requests.
| } | ||
| recorder := cfg.recorder | ||
| if recorder == nil { | ||
| recorder = GetRecorder("") |
There was a problem hiding this comment.
🧠 logic: Consider handling the case where GetRecorder returns nil to avoid potential nil pointer dereference.
| if len(route) <= 0 { | ||
| route = "nonconfigured" |
There was a problem hiding this comment.
🪶 style: Use route := ginCtx.FullPath() ?: "nonconfigured" for more concise code.
|
|
||
| defer func() { | ||
|
|
||
| resAttributes := append(reqAttributes[0:0], reqAttributes...) |
There was a problem hiding this comment.
🪶 style: Consider using copy instead of append(reqAttributes[0:0], reqAttributes...) for better performance.
| func Version() string { | ||
| return "1.0.0" |
There was a problem hiding this comment.
🪶 style: Consider making the version string a constant to avoid magic numbers.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced
signozEnabledflag and conditionalsignozClientinitialization inBotstruct - Added tracing middleware to bot server
- Added
requiresSignozfunction to handle Signoz configuration checks - Modified
traceCommandto userequiresSignoz - Introduced tracing middleware in
middleware.gofor enhanced observability
3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| )) | ||
|
|
||
| defer func() { | ||
| metrics.EndSpan(span) |
There was a problem hiding this comment.
🪶 style: Consider adding error handling within the deferred function to capture any potential issues when ending the span.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added tracing and metrics middleware to
opbot - Introduced
otelRecorderfor metrics recording inmiddleware.go - Updated
go.modfiles to include new dependencies for logging and OpenTelemetry - Removed unused dependency from
core/go.mod - Enhanced test structure with helper functions for span and attribute handling
4 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| } | ||
|
|
||
| var err error | ||
| otr.attemptsCounter, err = meter.Int64UpDownCounter(metricName("slacker.request_count"), metric.WithDescription("Number of Requests"), metric.WithUnit("Count")) |
There was a problem hiding this comment.
🪶 style: Consider handling the error more gracefully or providing a fallback mechanism.
| if err != nil { | ||
| log.Warnf("failed to create counter: %v", err) | ||
| } | ||
| otr.totalDuration, err = meter.Int64Histogram(metricName("slacker.duration"), metric.WithDescription("Time Taken by request"), metric.WithUnit("Milliseconds")) |
There was a problem hiding this comment.
🪶 style: Consider handling the error more gracefully or providing a fallback mechanism.
| log.Warnf("failed to create histogram: %v", err) | ||
| } | ||
|
|
||
| otr.activeRequestsCounter, err = meter.Int64UpDownCounter("slacker.active_requests") |
There was a problem hiding this comment.
🪶 style: Consider handling the error more gracefully or providing a fallback mechanism.
trajan0x
force-pushed
the
fix/http-capture-test
branch
from
e5ffb75
to
9475162
Compare
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added tracing and metrics middleware to
botmd.go - Introduced
otelRecorderfor detailed metrics inmiddleware.go - Updated
go.modincontrib/opbotfor logging and OpenTelemetry dependencies - Removed indirect dependency on
github.com/stretchr/objxincore/go.mod
4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| activeRequestsCounter metric.Int64UpDownCounter | ||
| } | ||
|
|
||
| func newOtelRecorder() otelRecorder { |
There was a problem hiding this comment.
🪶 style: 🪶 style: Consider adding error handling within the deferred function to capture any potential issues when ending the span.
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced metrics for tracking gas balances and converting gas balances from wei to ether.
- Added
recordBalancemethod andtoFloatfunction for better readability in metrics. - Enhanced observability with
gasBalanceGaugeandcurrentGasBalanceshashmap. - Improved modularity and maintainability by updating import paths and function references.
- Added helper functions and mock interfaces for metrics handling in tests.
2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
ethergo/submitter/submitter.go
Outdated
| t.gasBalanceGauge, err = t.meter.Float64ObservableGauge("gas_balance") | ||
| if err != nil { | ||
| return fmt.Errorf("could not create nonce gauge: %w", err) | ||
| } |
There was a problem hiding this comment.
📚 spelling: Ensure the error message here is accurate. It should mention gas balance gauge instead of nonce gauge.
There was a problem hiding this comment.
Actionable comments posted: 21
Outside diff range and nitpick comments (2)
ethergo/submitter/submitter.go (2)
Line range hint
95-109: Consider context cancellation for resource cleanup in reaper processThe reaper process within the
Startmethod could benefit from better handling of context cancellation. Ensure that resources are properly cleaned up when the context is cancelled, to avoid potential memory leaks or hanging goroutines.go func() { defer cancel() // Ensure resources are cleaned up for { select { case <-ctx.Done(): return case <-time.After(t.config.GetReaperInterval()): err := t.db.DeleteTXS(ctx, t.config.GetMaxRecordAge(), db.ReplacedOrConfirmed, db.Replaced, db.Confirmed) if err != nil { logger.Errorf("could not flush old records: %v", err) } } } }()
Line range hint
125-142: Potential simplification of metric setup insetupMetricsmethodThe method
setupMetricssets up multiple metrics and registers callbacks. Consider refactoring to reduce duplication and improve readability, possibly by introducing a helper function to handle common setup steps.func (t *txSubmitterImpl) setupMetric(name string, callback metric.Callback) error { gauge, err := t.meter.Int64ObservableGauge(name) if err != nil { return fmt.Errorf("could not create %s gauge: %w", name, err) } _, err = t.meter.RegisterCallback(callback, gauge) if err != nil { return fmt.Errorf("could not register callback for %s: %w", name, err) } return nil } func (t *txSubmitterImpl) setupMetrics() (err error) { if err = t.setupMetric("num_pending_txes", t.recordNumPending); err != nil { return err } if err = t.setupMetric("current_nonce", t.recordNonces); err != nil { return err } return nil }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
Files selected for processing (26)
- .golangci.yml (1 hunks)
- contrib/opbot/botmd/botmd.go (2 hunks)
- contrib/opbot/botmd/commands.go (2 hunks)
- contrib/opbot/botmd/middleware.go (1 hunks)
- contrib/opbot/go.mod (3 hunks)
- contrib/promexporter/exporters/exporter.go (3 hunks)
- contrib/screener-api/screener/screener.go (1 hunks)
- core/go.mod (1 hunks)
- core/metrics/base.go (3 hunks)
- core/metrics/instrumentation/otelginmetrics/README.md (1 hunks)
- core/metrics/instrumentation/otelginmetrics/config.go (1 hunks)
- core/metrics/instrumentation/otelginmetrics/example/main.go (1 hunks)
- core/metrics/instrumentation/otelginmetrics/middleware.go (1 hunks)
- core/metrics/instrumentation/otelginmetrics/option.go (1 hunks)
- core/metrics/instrumentation/otelginmetrics/otelrecorder.go (1 hunks)
- core/metrics/instrumentation/otelginmetrics/recorder.go (1 hunks)
- core/metrics/instrumentation/otelginmetrics/version.go (1 hunks)
- core/metrics/metrics.go (1 hunks)
- core/metrics/null.go (1 hunks)
- ethergo/submitter/chain_queue.go (3 hunks)
- ethergo/submitter/submitter.go (3 hunks)
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (1 hunks)
- services/omnirpc/modules/harmonyproxy/harmonyproxy.go (1 hunks)
- services/omnirpc/modules/receiptsbackup/receiptsbackup.go (1 hunks)
- services/omnirpc/proxy/server.go (1 hunks)
- services/scribe/api/server.go (1 hunks)
Files skipped from review due to trivial changes (4)
- .golangci.yml
- core/metrics/instrumentation/otelginmetrics/README.md
- core/metrics/instrumentation/otelginmetrics/version.go
- services/omnirpc/proxy/server.go
Files skipped from review as they are similar to previous changes (3)
- core/go.mod
- core/metrics/base.go
- core/metrics/metrics.go
Additional context used
GitHub Check: codecov/patch
core/metrics/instrumentation/otelginmetrics/config.go
[warning] 20-29: core/metrics/instrumentation/otelginmetrics/config.go#L20-L29
Added lines #L20 - L29 were not covered by tests
[warning] 33-44: core/metrics/instrumentation/otelginmetrics/config.go#L33-L44
Added lines #L33 - L44 were not covered by testscontrib/opbot/botmd/botmd.go
[warning] 29-33: contrib/opbot/botmd/botmd.go#L29-L33
Added lines #L29 - L33 were not covered by tests
[warning] 35-35: contrib/opbot/botmd/botmd.go#L35
Added line #L35 was not covered by tests
[warning] 41-44: contrib/opbot/botmd/botmd.go#L41-L44
Added lines #L41 - L44 were not covered by testscore/metrics/instrumentation/otelginmetrics/example/main.go
[warning] 19-22: core/metrics/instrumentation/otelginmetrics/example/main.go#L19-L22
Added lines #L19 - L22 were not covered by tests
[warning] 24-33: core/metrics/instrumentation/otelginmetrics/example/main.go#L24-L33
Added lines #L24 - L33 were not covered by tests
[warning] 35-36: core/metrics/instrumentation/otelginmetrics/example/main.go#L35-L36
Added lines #L35 - L36 were not covered by tests
[warning] 39-47: core/metrics/instrumentation/otelginmetrics/example/main.go#L39-L47
Added lines #L39 - L47 were not covered by tests
[warning] 50-53: core/metrics/instrumentation/otelginmetrics/example/main.go#L50-L53
Added lines #L50 - L53 were not covered by tests
[warning] 55-60: core/metrics/instrumentation/otelginmetrics/example/main.go#L55-L60
Added lines #L55 - L60 were not covered by tests
[warning] 62-65: core/metrics/instrumentation/otelginmetrics/example/main.go#L62-L65
Added lines #L62 - L65 were not covered by tests
[warning] 68-68: core/metrics/instrumentation/otelginmetrics/example/main.go#L68
Added line #L68 was not covered by testscore/metrics/instrumentation/otelginmetrics/middleware.go
[warning] 14-34: core/metrics/instrumentation/otelginmetrics/middleware.go#L14-L34
Added lines #L14 - L34 were not covered by tests
[warning] 36-42: core/metrics/instrumentation/otelginmetrics/middleware.go#L36-L42
Added lines #L36 - L42 were not covered by tests
[warning] 44-53: core/metrics/instrumentation/otelginmetrics/middleware.go#L44-L53
Added lines #L44 - L53 were not covered by tests
[warning] 55-61: core/metrics/instrumentation/otelginmetrics/middleware.go#L55-L61
Added lines #L55 - L61 were not covered by tests
[warning] 63-65: core/metrics/instrumentation/otelginmetrics/middleware.go#L63-L65
Added lines #L63 - L65 were not covered by testscontrib/opbot/botmd/middleware.go
[warning] 21-32: contrib/opbot/botmd/middleware.go#L21-L32
Added lines #L21 - L32 were not covered by tests
[warning] 34-34: contrib/opbot/botmd/middleware.go#L34
Added line #L34 was not covered by tests
[warning] 46-53: contrib/opbot/botmd/middleware.go#L46-L53
Added lines #L46 - L53 were not covered by tests
[warning] 55-63: contrib/opbot/botmd/middleware.go#L55-L63
Added lines #L55 - L63 were not covered by tests
[warning] 65-68: contrib/opbot/botmd/middleware.go#L65-L68
Added lines #L65 - L68 were not covered by tests
[warning] 70-70: contrib/opbot/botmd/middleware.go#L70
Added line #L70 was not covered by tests
[warning] 74-75: contrib/opbot/botmd/middleware.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 79-80: contrib/opbot/botmd/middleware.go#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 84-85: contrib/opbot/botmd/middleware.go#L84-L85
Added lines #L84 - L85 were not covered by tests
[warning] 88-104: contrib/opbot/botmd/middleware.go#L88-L104
Added lines #L88 - L104 were not covered by testscontrib/promexporter/exporters/exporter.go
[warning] 44-44: contrib/promexporter/exporters/exporter.go#L44
Added line #L44 was not covered by tests
[warning] 63-63: contrib/promexporter/exporters/exporter.go#L63
Added line #L63 was not covered by testscontrib/opbot/botmd/commands.go
[warning] 21-33: contrib/opbot/botmd/commands.go#L21-L33
Added lines #L21 - L33 were not covered by tests
[warning] 41-41: contrib/opbot/botmd/commands.go#L41
Added line #L41 was not covered by tests
GitHub Check: Lint (ethergo)
ethergo/submitter/chain_queue.go
[failure] 179-179:
(*txSubmitterImpl).recordBalance- resulterris alwaysnil(unparam)
[failure] 197-197:
functoFloatis unused (unused)
Additional comments not posted (15)
core/metrics/instrumentation/otelginmetrics/recorder.go (1)
10-25: Interface definition forRecorderlooks good.The methods are well-documented and provide clear interfaces for metrics recording. This should facilitate easy implementation and integration.
core/metrics/instrumentation/otelginmetrics/config.go (1)
33-44: Add test coverage forDefaultAttributesfunction.The
DefaultAttributesfunction, which dynamically generates attributes based on inputs, lacks test coverage. This is critical as it directly affects the metrics attributes.Tools
GitHub Check: codecov/patch
[warning] 33-44: core/metrics/instrumentation/otelginmetrics/config.go#L33-L44
Added lines #L33 - L44 were not covered by testscore/metrics/instrumentation/otelginmetrics/example/main.go (1)
26-26: Remove or replace debug print statement.The print statement for the metric exporter is likely a leftover from debugging. Consider removing it or replacing it with a structured logging statement if needed.
- fmt.Println(metricExporter) + log.Debug("Metric Exporter initialized: %v", metricExporter)Likely invalid or redundant comment.
core/metrics/instrumentation/otelginmetrics/option.go (2)
20-26: Configuration option for attributes is well-implemented.The
WithAttributesfunction provides a flexible way to customize the recorded attributes, adhering to best practices in configuration design.
28-34: Comprehensive configuration options provided.The various configuration options such as
WithRecordInFlightDisabled,WithRecordDurationDisabled, etc., are well-implemented, providing granular control over the metrics recording behavior.Also applies to: 36-42, 44-50, 52-58, 60-66, 68-74
core/metrics/null.go (1)
60-61: Approved: UpdatedGin()method to return a slice of handlers.This change aligns with the updates in the metrics interface to support multiple middleware handlers, enhancing flexibility in configuring Gin routes.
core/metrics/instrumentation/otelginmetrics/otelrecorder.go (1)
12-12: Consider updating the instrumentation name for consistency.The instrumentation name should reflect the current repository path to avoid confusion and maintain consistency across different parts of the application.
services/scribe/api/server.go (1)
57-57: Approved: Correct use of variadic spread for middleware application.Using the spread operator here allows for flexible configuration of middleware functions, which can vary in number based on runtime conditions. This is a good practice for modular middleware management.
services/omnirpc/modules/receiptsbackup/receiptsbackup.go (1)
68-68: Ensure Middleware Compatibility:The added line where the middleware is used (
router.Use(r.handler.Gin()...)) should be checked for compatibility with the rest of the middleware stack. This is crucial as middleware can affect the flow and error handling of the entire application.Verification successful
Middleware Compatibility Verified:
The middleware usage in
receiptsbackup.gois consistent with the middleware patterns observed across other files in the project, indicating compatibility with the existing middleware stack.
services/scribe/api/server.goservices/omnirpc/proxy/server.goservices/omnirpc/modules/harmonyproxy/harmonyproxy.goservices/omnirpc/modules/confirmedtofinalized/finalizedproxy.goservices/omnirpc/modules/receiptsbackup/receiptsbackup.gocontrib/promexporter/exporters/exporter.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify middleware compatibility. # Test: Search for middleware usage in the project. Expect: Consistent middleware usage across similar routers. rg --type go 'router.Use'Length of output: 930
services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (1)
69-69: Check Middleware Integration:The middleware usage in
router.Use(r.handler.Gin()...)should be verified for integration with other parts of the application, especially given the critical nature of handling finalized transactions.Verification successful
Middleware Integration Verified:
The middleware usage in
router.Use(r.handler.Gin()...)is consistent with other parts of the application. No issues found with the integration.
services/scribe/api/server.goservices/omnirpc/proxy/server.goservices/omnirpc/modules/harmonyproxy/harmonyproxy.goservices/omnirpc/modules/receiptsbackup/receiptsbackup.gocontrib/promexporter/exporters/exporter.gocontrib/screener-api/screener/screener.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify middleware integration. # Test: Search for middleware usage in the project. Expect: Consistent middleware usage across similar routers. rg --type go 'router.Use'Length of output: 930
contrib/screener-api/screener/screener.go (1)
89-89: Metrics middleware integration looks good.The addition of metrics middleware will enhance observability. Ensure to monitor the performance to detect any potential overhead introduced by this change.
services/omnirpc/modules/harmonyproxy/harmonyproxy.go (1)
64-64: Metrics middleware integration in HarmonyProxy looks correct.This change should help in better monitoring and observability of the HarmonyProxy service. It's recommended to keep an eye on the performance metrics to ensure there's no significant impact.
contrib/opbot/go.mod (2)
16-16: Dependency ongithub.com/hedzr/logadded.Ensure that this logging library integrates well with the existing system and does not introduce any redundancy or conflicts.
28-30: Addition of OpenTelemetry dependencies.These dependencies are crucial for enhanced observability. Ensure that their versions are compatible with other project dependencies and monitor any potential performance impacts.
ethergo/submitter/submitter.go (1)
86-92: Addition of new metrics fields for gas balance managementThe introduction of
gasBalanceGaugeandcurrentGasBalancesfields aligns with the PR's objective to enhance metrics for gas management. Ensure that these new metrics are integrated properly into the monitoring systems and that they are being updated at appropriate intervals in the codebase.
ethergo/submitter/submitter.go
Outdated
| t.gasBalanceGauge, err = t.meter.Float64ObservableGauge("gas_balance") | ||
| if err != nil { | ||
| return fmt.Errorf("could not create nonce gauge: %w", err) | ||
| } |
There was a problem hiding this comment.
Error message copy-paste mistake in gas balance gauge setup
The error message in the creation of the gasBalanceGauge incorrectly refers to "nonce gauge". This should be corrected to reflect the actual metric being initialized.
- return fmt.Errorf("could not create nonce gauge: %w", err)
+ return fmt.Errorf("could not create gas balance 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.
| t.gasBalanceGauge, err = t.meter.Float64ObservableGauge("gas_balance") | |
| if err != nil { | |
| return fmt.Errorf("could not create nonce gauge: %w", err) | |
| } | |
| t.gasBalanceGauge, err = t.meter.Float64ObservableGauge("gas_balance") | |
| if err != nil { | |
| return fmt.Errorf("could not create gas balance gauge: %w", err) | |
| } |
| start := time.Now() | ||
| reqAttributes := cfg.attributes(service, route, ginCtx.Request) | ||
|
|
||
| if cfg.recordInFlight { | ||
| recorder.AddInflightRequests(ctx, 1, reqAttributes) | ||
| defer recorder.AddInflightRequests(ctx, -1, reqAttributes) | ||
| } |
There was a problem hiding this comment.
Add tests for in-flight request recording logic.
This block handles the recording of in-flight requests which is a key metric for understanding server load. Ensuring this functionality is correctly implemented and tested is crucial.
Tools
GitHub Check: codecov/patch
[warning] 36-42: core/metrics/instrumentation/otelginmetrics/middleware.go#L36-L42
Added lines #L36 - L42 were not covered by tests
| defer func() { | ||
|
|
||
| resAttributes := append(reqAttributes[0:0], reqAttributes...) | ||
|
|
||
| if cfg.groupedStatus { | ||
| code := int(ginCtx.Writer.Status()/100) * 100 | ||
| resAttributes = append(resAttributes, semconv.HTTPStatusCodeKey.Int(code)) | ||
| } else { | ||
| resAttributes = append(resAttributes, semconv.HTTPAttributesFromHTTPStatusCode(ginCtx.Writer.Status())...) | ||
| } |
There was a problem hiding this comment.
Ensure test coverage for request and response attribute handling.
This section modifies request attributes based on response status and should be covered by tests to validate its behavior across different HTTP status codes.
Tools
GitHub Check: codecov/patch
[warning] 44-53: core/metrics/instrumentation/otelginmetrics/middleware.go#L44-L53
Added lines #L44 - L53 were not covered by tests
| recorder.AddRequests(ctx, 1, resAttributes) | ||
|
|
||
| if cfg.recordSize { | ||
| requestSize := computeApproximateRequestSize(ginCtx.Request) | ||
| recorder.ObserveHTTPRequestSize(ctx, requestSize, resAttributes) | ||
| recorder.ObserveHTTPResponseSize(ctx, int64(ginCtx.Writer.Size()), resAttributes) | ||
| } |
There was a problem hiding this comment.
Test coverage needed for request and response size metrics.
Recording the sizes of requests and responses is important for monitoring traffic and resource usage. Tests should verify that these metrics are accurately recorded and reported.
Tools
GitHub Check: codecov/patch
[warning] 55-61: core/metrics/instrumentation/otelginmetrics/middleware.go#L55-L61
Added lines #L55 - L61 were not covered by tests
| if cfg.recordDuration { | ||
| recorder.ObserveHTTPRequestDuration(ctx, time.Since(start), resAttributes) | ||
| } |
There was a problem hiding this comment.
Missing tests for request duration observation.
Observing request durations is vital for performance monitoring. This functionality should have corresponding tests to ensure accurate timing measurements.
Tools
GitHub Check: codecov/patch
[warning] 63-65: core/metrics/instrumentation/otelginmetrics/middleware.go#L63-L65
Added lines #L63 - L65 were not covered by tests
| @@ -41,7 +41,7 @@ | |||
| httpClient := new(http.Client) | |||
| handler.ConfigureHTTPClient(httpClient) | |||
|
|
|||
| httpClient.Transport = instrumentation.NewCaptureTransport(httpClient.Transport, handler) | |||
| httpClient.Transport = httpcapture.NewCaptureTransport(httpClient.Transport, handler) | |||
There was a problem hiding this comment.
Add Error Handling in HTTP Client Configuration:
The configuration of the HTTP client lacks error handling for ConfigureHTTPClient. It's crucial to handle potential configuration errors to prevent runtime issues.
- handler.ConfigureHTTPClient(httpClient)
+ if err := handler.ConfigureHTTPClient(httpClient); err != nil {
+ return nil, fmt.Errorf("failed to configure HTTP client: %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.
| httpClient.Transport = httpcapture.NewCaptureTransport(httpClient.Transport, handler) | |
| httpClient.Transport = httpcapture.NewCaptureTransport(httpClient.Transport, handler) | |
| if err := handler.ConfigureHTTPClient(httpClient); err != nil { | |
| return nil, fmt.Errorf("failed to configure HTTP client: %w", err) | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 44-44: contrib/promexporter/exporters/exporter.go#L44
Added line #L44 was not covered by tests
| // nolint: traceCommand, gocognit | ||
| // TODO: add trace middleware. | ||
| func (b *Bot) traceCommand() *slacker.CommandDefinition { | ||
| return &slacker.CommandDefinition{ | ||
| return b.requiresSignoz(&slacker.CommandDefinition{ |
There was a problem hiding this comment.
Enhance Trace Command with Middleware Integration:
The traceCommand function uses requiresSignoz to conditionally modify the command based on configuration. Consider integrating middleware to handle such conditional logic more cleanly across the bot application.
// Suggestion to integrate middleware for handling conditional logic
+func ConditionalMiddleware() gin.HandlerFunc {
+ return func(c *gin.Context) {
+ if !bot.signozEnabled {
+ c.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": "Signoz is not configured"})
+ return
+ }
+ c.Next()
+ }
+}Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 41-41: contrib/opbot/botmd/commands.go#L41
Added line #L41 was not covered by tests
| func (b *Bot) requiresSignoz(definition *slacker.CommandDefinition) *slacker.CommandDefinition { | ||
| if b.signozEnabled { | ||
| return definition | ||
| } | ||
| return &slacker.CommandDefinition{ | ||
| Command: definition.Command, | ||
| Description: fmt.Sprintf("normally this would \"%s\", but signoz is not configured", definition.Description), | ||
| Examples: definition.Examples, | ||
| Handler: func(ctx *slacker.CommandContext) { | ||
| _, err := ctx.Response().Reply("cannot run command: signoz is not configured") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Conditional Command Handling Based on Configuration:
The requiresSignoz function modifies command definitions based on whether Signoz is enabled. This approach allows dynamic adjustment of bot capabilities based on configuration, which is a good practice. However, ensure this function is covered by unit tests to verify its behavior under different configurations.
+func TestRequiresSignoz(t *testing.T) {
+ bot := Bot{signozEnabled: true}
+ cmd := &slacker.CommandDefinition{Command: "test", Description: "A test command"}
+
+ modifiedCmd := bot.requiresSignoz(cmd)
+
+ if modifiedCmd.Description != "A test command" {
+ t.Errorf("Expected unmodified command description, got %s", modifiedCmd.Description)
+ }
+}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 (b *Bot) requiresSignoz(definition *slacker.CommandDefinition) *slacker.CommandDefinition { | |
| if b.signozEnabled { | |
| return definition | |
| } | |
| return &slacker.CommandDefinition{ | |
| Command: definition.Command, | |
| Description: fmt.Sprintf("normally this would \"%s\", but signoz is not configured", definition.Description), | |
| Examples: definition.Examples, | |
| Handler: func(ctx *slacker.CommandContext) { | |
| _, err := ctx.Response().Reply("cannot run command: signoz is not configured") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| }, | |
| } | |
| } | |
| func (b *Bot) requiresSignoz(definition *slacker.CommandDefinition) *slacker.CommandDefinition { | |
| if b.signozEnabled { | |
| return definition | |
| } | |
| return &slacker.CommandDefinition{ | |
| Command: definition.Command, | |
| Description: fmt.Sprintf("normally this would \"%s\", but signoz is not configured", definition.Description), | |
| Examples: definition.Examples, | |
| Handler: func(ctx *slacker.CommandContext) { | |
| _, err := ctx.Response().Reply("cannot run command: signoz is not configured") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| }, | |
| } | |
| } | |
| +func TestRequiresSignoz(t *testing.T) { | |
| + bot := Bot{signozEnabled: true} | |
| + cmd := &slacker.CommandDefinition{Command: "test", Description: "A test command"} | |
| + | |
| + modifiedCmd := bot.requiresSignoz(cmd) | |
| + | |
| + if modifiedCmd.Description != "A test command" { | |
| + t.Errorf("Expected unmodified command description, got %s", modifiedCmd.Description) | |
| + } | |
| +} |
Tools
GitHub Check: codecov/patch
[warning] 21-33: contrib/opbot/botmd/commands.go#L21-L33
Added lines #L21 - L33 were not covered by tests
| func (t *txSubmitterImpl) recordBalance(_ context.Context, observer metric.Observer) (err error) { | ||
| if t.metrics == nil || t.gasBalanceGauge == nil { | ||
| return nil | ||
| } | ||
|
|
||
| t.currentGasBalances.Range(func(chainID uint32, gasPrice *big.Int) bool { | ||
| opts := metric.WithAttributes( | ||
| attribute.Int(metrics.ChainID, int(chainID)), | ||
| attribute.String("wallet", t.signer.Address().Hex()), | ||
| ) | ||
|
|
||
| observer.ObserveFloat64(t.gasBalanceGauge, toFloat(gasPrice), opts) | ||
| return true | ||
| }) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Refactor Always Nil Error Return:
The function recordBalance always returns nil, which is redundant. Consider refactoring this function to remove the unnecessary error return value.
- func (t *txSubmitterImpl) recordBalance(_ context.Context, observer metric.Observer) (err error) {
+ func (t *txSubmitterImpl) recordBalance(_ context.Context, observer metric.Observer) {
// Implementation remains the same
- return nil
}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) recordBalance(_ context.Context, observer metric.Observer) (err error) { | |
| if t.metrics == nil || t.gasBalanceGauge == nil { | |
| return nil | |
| } | |
| t.currentGasBalances.Range(func(chainID uint32, gasPrice *big.Int) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int(metrics.ChainID, int(chainID)), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| observer.ObserveFloat64(t.gasBalanceGauge, toFloat(gasPrice), opts) | |
| return true | |
| }) | |
| return nil | |
| } | |
| func (t *txSubmitterImpl) recordBalance(_ context.Context, observer metric.Observer) { | |
| if t.metrics == nil || t.gasBalanceGauge == nil { | |
| return | |
| } | |
| t.currentGasBalances.Range(func(chainID uint32, gasPrice *big.Int) bool { | |
| opts := metric.WithAttributes( | |
| attribute.Int(metrics.ChainID, int(chainID)), | |
| attribute.String("wallet", t.signer.Address().Hex()), | |
| ) | |
| observer.ObserveFloat64(t.gasBalanceGauge, toFloat(gasPrice), opts) | |
| return true | |
| }) | |
| } |
Tools
GitHub Check: Lint (ethergo)
[failure] 179-179:
(*txSubmitterImpl).recordBalance- resulterris alwaysnil(unparam)
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Introduced middleware for tracing and metrics in Slack bot applications
- Added HTTP request/response capture with a new test suite
- Implemented metrics handling with autogenerated mock functions
- Enhanced gas balance recording in Ethereum-related functions
- Updated dependencies in
go.modfor logging, OpenTelemetry, and metrics
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| t.gasBalanceGauge, err = t.meter.Float64ObservableGauge("gas_balance") | ||
| if err != nil { | ||
| return fmt.Errorf("could not create gas balance gauge: %w", err) |
There was a problem hiding this comment.
📚 spelling: Ensure the error message here is accurate. It should mention gas balance gauge instead of nonce gauge.
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)
- ethergo/submitter/submitter.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- ethergo/submitter/submitter.go
Description
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores