omnirpc op #989
Merged
omnirpc op #989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #989 +/- ##
====================================================
+ Coverage 40.95557% 52.17995% +11.22437%
====================================================
Files 155 466 +311
Lines 11344 28785 +17441
Branches 79 79
====================================================
+ Hits 4646 15020 +10374
- Misses 6093 12295 +6202
- Partials 605 1470 +865
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Warning Rate Limit Exceeded@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 41 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update enhances the Omnirpc service with a new Changes
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 (
|
Qodana for Go2 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (6)
- services/omnirpc/cmd/cmd.go (1 hunks)
- services/omnirpc/cmd/commands.go (1 hunks)
- services/omnirpc/cmd/flags.go (1 hunks)
- services/omnirpc/proxy/confirmable.go (3 hunks)
- services/omnirpc/proxy/server.go (1 hunks)
- services/omnirpc/proxy/simpleproxy.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/omnirpc/proxy/server.go
Additional comments: 8
services/omnirpc/cmd/flags.go (1)
- 44-47: LGTM!
services/omnirpc/proxy/confirmable.go (3)
- 4-4: The import of the
bytespackage is appropriate for the functionality introduced.- 57-65: The
rewriteConfirmableRequestfunction correctly replaceslatestBlockwithfinalizedBlock. Ensure that all relevant RPC methods are covered.- 89-95: The
rewriteConfirmableRequestsfunction correctly applies the rewrite logic to a list of requests.services/omnirpc/proxy/simpleproxy.go (3)
- 20-31: The
SimpleProxystruct is correctly defined with appropriate fields for its functionality.- 33-41: The
NewSimpleProxyfunction is correctly implemented for creating a new instance ofSimpleProxy.- 44-70: The
Runmethod correctly sets up the router and endpoints. Ensure logging and error handling are consistent with project standards.services/omnirpc/cmd/commands.go (1)
- 157-174: The
latest-rewritecommand is correctly implemented and follows the pattern of other commands in the file. Ensure that the usage description is clear and concise.
Comment on lines
75
to
130
| // ProxyRequest proxies a request to the proxyURL | ||
| func (r *SimpleProxy) ProxyRequest(c *gin.Context) (err error) { | ||
| ctx, span := r.tracer.Start(c, "ProxyRequest", | ||
| trace.WithAttributes(attribute.String("endpoint", r.proxyURL)), | ||
| ) | ||
|
|
||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| requestID := []byte(c.GetHeader(omniHTTP.XRequestIDString)) | ||
|
|
||
| rawBody, err := io.ReadAll(c.Request.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("could not read request body: %w", err) | ||
| } | ||
|
|
||
| // make sure it's not a batch | ||
| if rpc.IsBatch(rawBody) { | ||
| err = c.Error(batchErr) | ||
| return err | ||
| } | ||
|
|
||
| rpcRequests, err := rpc.ParseRPCPayload(rawBody) | ||
| if err != nil { | ||
| return fmt.Errorf("could not parse payload: %w", err) | ||
| } | ||
|
|
||
| rpcRequest := rpcRequests[0] | ||
|
|
||
| span.SetAttributes(attribute.String("original-body", string(rawBody))) | ||
|
|
||
| rpcRequest = rewriteConfirmableRequest(rpcRequest) | ||
|
|
||
| body, err := json.Marshal(rpcRequest) | ||
| if err != nil { | ||
| return fmt.Errorf("could not marshal request") | ||
| } | ||
|
|
||
| req := r.client.NewRequest() | ||
| resp, err := req. | ||
| SetContext(ctx). | ||
| SetRequestURI(r.proxyURL). | ||
| SetBody(body). | ||
| SetHeaderBytes(omniHTTP.XRequestID, requestID). | ||
| SetHeaderBytes(omniHTTP.XForwardedFor, []byte(r.proxyURL)). | ||
| SetHeaderBytes(omniHTTP.ContentType, omniHTTP.JSONType). | ||
| SetHeaderBytes(omniHTTP.Accept, omniHTTP.JSONType). | ||
| Do() | ||
| if err != nil { | ||
| return fmt.Errorf("could not get response from %s: %w", r.proxyURL, err) | ||
| } | ||
|
|
||
| c.Data(resp.StatusCode(), gin.MIMEJSON, resp.Body()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The ProxyRequest method correctly proxies requests and applies the rewrite logic for confirmable requests. However, consider improving error handling and logging for better maintainability.
- return fmt.Errorf("could not get response from %s: %w", r.proxyURL, err)
+ log.Errorf("could not get response from %s: %v", r.proxyURL, err)
+ return errCommittable 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.
Suggested change
| // ProxyRequest proxies a request to the proxyURL | |
| func (r *SimpleProxy) ProxyRequest(c *gin.Context) (err error) { | |
| ctx, span := r.tracer.Start(c, "ProxyRequest", | |
| trace.WithAttributes(attribute.String("endpoint", r.proxyURL)), | |
| ) | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| requestID := []byte(c.GetHeader(omniHTTP.XRequestIDString)) | |
| rawBody, err := io.ReadAll(c.Request.Body) | |
| if err != nil { | |
| return fmt.Errorf("could not read request body: %w", err) | |
| } | |
| // make sure it's not a batch | |
| if rpc.IsBatch(rawBody) { | |
| err = c.Error(batchErr) | |
| return err | |
| } | |
| rpcRequests, err := rpc.ParseRPCPayload(rawBody) | |
| if err != nil { | |
| return fmt.Errorf("could not parse payload: %w", err) | |
| } | |
| rpcRequest := rpcRequests[0] | |
| span.SetAttributes(attribute.String("original-body", string(rawBody))) | |
| rpcRequest = rewriteConfirmableRequest(rpcRequest) | |
| body, err := json.Marshal(rpcRequest) | |
| if err != nil { | |
| return fmt.Errorf("could not marshal request") | |
| } | |
| req := r.client.NewRequest() | |
| resp, err := req. | |
| SetContext(ctx). | |
| SetRequestURI(r.proxyURL). | |
| SetBody(body). | |
| SetHeaderBytes(omniHTTP.XRequestID, requestID). | |
| SetHeaderBytes(omniHTTP.XForwardedFor, []byte(r.proxyURL)). | |
| SetHeaderBytes(omniHTTP.ContentType, omniHTTP.JSONType). | |
| SetHeaderBytes(omniHTTP.Accept, omniHTTP.JSONType). | |
| Do() | |
| if err != nil { | |
| return fmt.Errorf("could not get response from %s: %w", r.proxyURL, err) | |
| } | |
| c.Data(resp.StatusCode(), gin.MIMEJSON, resp.Body()) | |
| return nil | |
| } | |
| if err != nil { | |
| log.Errorf("could not get response from %s: %v", r.proxyURL, err) | |
| return err | |
| } |
trajan0x
requested review from
dwasse,
aureliusbtc and
ChiTimesChi
as code owners
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (40)
- agents/go.mod (2 hunks)
- agents/go.sum (3 hunks)
- contrib/git-changes-action/go.mod (1 hunks)
- contrib/git-changes-action/go.sum (2 hunks)
- contrib/promexporter/go.mod (2 hunks)
- contrib/promexporter/go.sum (3 hunks)
- contrib/release-copier-action/go.mod (1 hunks)
- contrib/release-copier-action/go.sum (2 hunks)
- contrib/screener-api/go.mod (2 hunks)
- contrib/screener-api/go.sum (3 hunks)
- core/go.mod (3 hunks)
- core/go.sum (3 hunks)
- core/metrics/README.md (1 hunks)
- core/metrics/base.go (5 hunks)
- core/metrics/experimental.go (1 hunks)
- core/metrics/logger_generated.go (1 hunks)
- core/metrics/metrics.go (1 hunks)
- ethergo/go.mod (3 hunks)
- ethergo/go.sum (3 hunks)
- go.work.sum (2 hunks)
- services/cctp-relayer/go.mod (2 hunks)
- services/cctp-relayer/go.sum (3 hunks)
- services/explorer/go.mod (2 hunks)
- services/explorer/go.sum (3 hunks)
- services/omnirpc/cmd/commands.go (1 hunks)
- services/omnirpc/go.mod (3 hunks)
- services/omnirpc/go.sum (3 hunks)
- services/omnirpc/modules/README.md (1 hunks)
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (1 hunks)
- services/omnirpc/proxy/confirmable.go (3 hunks)
- services/rfq/go.mod (2 hunks)
- services/rfq/go.sum (3 hunks)
- services/scribe/go.mod (2 hunks)
- services/scribe/go.sum (3 hunks)
- services/sinner/go.mod (2 hunks)
- services/sinner/go.sum (3 hunks)
- services/stiprelayer/go.mod (2 hunks)
- services/stiprelayer/go.sum (3 hunks)
- tools/go.mod (1 hunks)
- tools/go.sum (2 hunks)
Files not reviewed due to errors (3)
- (no review received)
- (no review received)
- (no review received)
Files skipped from review due to trivial changes (3)
- contrib/git-changes-action/go.mod
- core/metrics/logger_generated.go
- services/omnirpc/modules/README.md
Files skipped from review as they are similar to previous changes (1)
- services/omnirpc/cmd/commands.go
Additional comments: 79
contrib/release-copier-action/go.mod (1)
- 32-33: Version updates for
go.uber.org/multierrandgo.uber.org/zaplook good. Ensure compatibility with the rest of the project.core/metrics/experimental.go (1)
- 1-59: Implementation of
wrappedSugarLoggerfor context-aware logging usingotelzap.SugaredLoggeris consistent and enhances observability.services/omnirpc/proxy/confirmable.go (3)
- 4-4: Addition of the
bytespackage supports the functionality for rewriting block queries.- 13-13: Renaming
rpc2package alias toethergoRPCimproves clarity.- 57-65: Implementation of
rewriteConfirmableRequest()correctly modifies request params based on block numbers.services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (3)
- 21-32:
FinalizedProxystruct definition and its fields are well-structured for handling proxy requests with the goal of rewriting block queries.- 45-71:
Runmethod setup and server start logic are correctly implemented. Ensure routing and error handling are thoroughly tested.- 77-131:
ProxyRequestmethod correctly rewrites and forwards requests. Ensure batch requests are properly identified and handled as not supported.tools/go.mod (1)
- 84-85: Version updates for
go.uber.org/multierrandgo.uber.org/zaplook good. Ensure compatibility with the rest of the project.core/metrics/README.md (1)
- 36-54: Documentation updates clearly explain the introduction of the new
otelzap logger, its limitations, and how to use it. Ensure all necessary details are included for a smooth transition.core/metrics/base.go (4)
- 11-17: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [14-28]
Addition of
otelzapandzapimports supports the new logging mechanism.
- 47-48: Adding
experimentalLoggerfield tobaseHandlerstruct aligns with the introduction of the new logging mechanism.- 165-167: Implementation of
ExperimentalLogger()method inbaseHandlercorrectly provides access to the new logger.- 215-234: Ensure the initialization of
betaLoggerand handling of experimental global logger settings are correctly implemented. Review potential issues with global logger replacement and synchronization on shutdown.core/go.mod (3)
- 41-41: Ensure compatibility and check for any breaking changes or security vulnerabilities in
github.com/uptrace/opentelemetry-go-extra/otelzap v0.2.3.- 56-56: Verify that
go.uber.org/zap v1.26.0does not introduce breaking changes that affect existing logging functionality.- 153-153: Check for compatibility and security issues with the newly added
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3.contrib/screener-api/go.mod (2)
- 140-141: Added
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these additions are necessary and check for compatibility issues.- 160-161: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify compatibility and absence of breaking changes.contrib/promexporter/go.mod (2)
- 193-194: Added
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these additions are necessary and check for compatibility issues.- 211-212: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify compatibility and absence of breaking changes.services/omnirpc/go.mod (4)
- 40-40: Added dependency
github.com/uptrace/opentelemetry-go-extra/otelzap v0.2.3is not mentioned in the PR summary or objectives. Verify its necessity for the feature implementation.- 214-214: Added dependency
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3is correctly aligned with the PR objectives for enhanced integration test accuracy through telemetry support.- 234-234: Updated
go.uber.org/multierrtov1.11.0aligns with the PR's aim for dependency version updates. Ensure compatibility with existing code.- 235-235: Updated
go.uber.org/zaptov1.26.0is in line with the PR objectives. Confirm no breaking changes affect the current logging implementation.ethergo/go.mod (4)
- 57-57: Updated
go.uber.org/zaptov1.26.0aligns with the PR objectives for the ethergo service. Ensure compatibility with existing logging features.- 226-226: Added dependency
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3is correctly aligned with the PR objectives for enhanced telemetry support in the ethergo service.- 227-227: Added dependency
github.com/uptrace/opentelemetry-go-extra/otelzap v0.2.3is not explicitly mentioned in the PR summary or objectives for the ethergo service. Verify its necessity for the feature implementation.- 247-247: Updated
go.uber.org/multierrtov1.11.0aligns with the PR's aim for dependency version updates in the ethergo service. Ensure compatibility with existing error handling mechanisms.Verification successful
The verification process has identified that the
multierr.usage within the repository is actually linked github.com/hashicorp/go-multierrorrather thango.uber.org/multierras initially intended for update verification. This indicates that the specific update togo.uber.org/multierr v1.11.0might not directly impact the existing codebase since the found usage pertains to a different multi-error handling package.Given this context, the original aim to ensure compatibility with existing error handling mechanisms after updating
go.uber.org/multierrtov1.11.0in the ethergo service might not be directly relevant if the service primarily usesgithub.aaakk.us.kg/hashicorp/go-multierrorfor error aggregation.Therefore, while the update to
go.uber.org/multierr v1.11.0is noted, the direct compatibility concern highlighted does not apply to the identified usage within the codebase. It's advisable to ensure that any other dependencies or indirect usages ofgo.uber.org/multierrare also compatible, but no direct issues were found based on the script output.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify compatibility of go.uber.org/multierr v1.11.0 with existing code in ethergo service rg --type go 'multierr.' --exec ../ethergoLength of output: 451
Script:
#!/bin/bash # Corrected script to verify compatibility of go.uber.org/multierr v1.11.0 with existing code in ethergo service rg --type go 'multierr.' ../ethergoLength of output: 86
Script:
#!/bin/bash # Search for 'multierr.' usage within Go files across the entire repository rg --type go 'multierr.'Length of output: 252
services/explorer/go.mod (2)
- 235-236: Added new dependencies
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these libraries are compatible with the existing codebase and their versions align with other opentelemetry dependencies.- 257-258: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify that these updates do not introduce breaking changes or conflicts with the current usage patterns in the codebase.services/stiprelayer/go.mod (2)
- 221-222: Added new dependencies
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these libraries are compatible with the existing codebase and their versions align with other opentelemetry dependencies.- 245-246: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify that these updates do not introduce breaking changes or conflicts with the current usage patterns in the codebase.services/scribe/go.mod (2)
- 252-253: Added dependencies
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these versions are compatible with other opentelemetry dependencies and the project's telemetry strategy.- 275-276: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify these updates do not introduce breaking changes or incompatibilities with existing logging and error handling implementations.services/rfq/go.mod (2)
- 232-233: Added dependencies
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these versions are compatible with other opentelemetry dependencies and the project's telemetry strategy.- 255-256: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify these updates do not introduce breaking changes or incompatibilities with existing logging and error handling implementations.services/cctp-relayer/go.mod (2)
- 233-234: Adding
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3introduces new dependencies for telemetry and logging. Ensure these libraries are compatible with the existing telemetry setup and that their features are necessary for the project's requirements.- 259-260: Updating
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0could introduce breaking changes or new features. Verify that these version updates do not break existing functionality and that any new features or changes are compatible with the project's current usage of these libraries.services/sinner/go.mod (2)
- 266-267: Adding
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3introduces new dependencies for telemetry and logging. Ensure these versions are compatible with the rest of the project's dependencies and that they do not introduce any known security vulnerabilities.- 292-293: Updating
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0should improve error handling and logging capabilities. Verify that these version updates are compatible with existing code and do not introduce breaking changes.agents/go.mod (2)
- 72-73: Added indirect dependencies
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3. Ensure these additions are necessary for the project's functionality and do not introduce redundancy with existing dependencies.- 294-295: Updated versions of
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify that these updates do not introduce breaking changes or compatibility issues with the project's existing codebase.contrib/release-copier-action/go.sum (3)
- 94-94: The removal of
github.com/benbjohnson/clock v1.3.0needs verification to ensure no dependencies within the project are affected.- 94-94:
go.uber.org/multierrupdated tov1.11.0. Verify this update doesn't introduce breaking changes or compatibility issues with the project's usage of this module.- 97-97:
go.uber.org/zapupdated tov1.26.0. Ensure this version is compatible with the project and doesn't introduce any breaking changes or performance regressions.contrib/screener-api/go.sum (2)
- 468-469: Added versions
v0.2.3ofgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelutilandgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap. Ensure these versions are compatible with the project's usage of OpenTelemetry and do not introduce breaking changes or known vulnerabilities.- 539-543: Updated version from
v1.10.0tov1.11.0forgo.uber.org/multierrand fromv1.25.0tov1.26.0forgo.uber.org/zap. Verify that these updates do not introduce breaking changes or incompatibilities with the project's error handling and logging functionalities.ethergo/go.sum (3)
- 170-175: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0is not explicitly mentioned. Ensure this removal does not affect any part of the project that relies on specific features or bug fixes introduced inv1.3.0.
- 1041-1042: Added dependencies
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3are introduced. Confirm these versions are compatible with the project's existing OpenTelemetry setup and do not introduce breaking changes.- 1141-1146: Updated versions for
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Ensure these updates do not introduce breaking changes or conflicts with the project's error handling and logging mechanisms.services/omnirpc/go.sum (3)
- 130-135: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0and the update github.com/benbjohnson/immutable v0.4.3should be verified for compatibility and potential breaking changes. Ensure that the updated versions of these dependencies do not introduce issues with existing code that relies on them.
- 1035-1038: The updates to
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3indicate improvements or changes in telemetry handling. Confirm that these updates align with the project's telemetry strategy and do not affect the observability features negatively.- 1145-1152: The updates to
go.uber.org/multierr v1.11.0andgo.uber.org/zap v1.26.0suggest enhancements in error handling and logging. It's crucial to ensure that these updates do not conflict with the project's existing error handling and logging implementations. Additionally, verify that the new versions do not introduce any performance regressions or unwanted behavior changes.services/stiprelayer/go.sum (3)
- 174-179: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0and its replacement withv1.1.0is not explicitly mentioned. Ensure this downgrade aligns with project requirements.
- 1079-1080: Added
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andotelzap v0.2.3. Confirm these versions are compatible with the project's existing telemetry setup.- 1187-1192: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify these updates do not introduce breaking changes or compatibility issues with the project's error handling and logging mechanisms.services/rfq/go.sum (3)
- 180-185: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
Ensure that the removal of
github.com/benbjohnson/clock v1.3.0and the addition ofv1.1.0does not introduce any backward compatibility issues or remove any features relied upon by the project.
- 1087-1088: The addition of
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3introduces new dependencies. Verify that these are necessary for the project and assess their impact on the project's performance and maintainability.- 1195-1200: The updates to
go.uber.org/multierrfromv1.10.0tov1.11.0andgo.uber.org/zapfromv1.25.0tov1.26.0should be checked for compatibility with the existing codebase. Ensure that there are no breaking changes or deprecated functionalities that could affect the project.services/cctp-relayer/go.sum (3)
- 185-190: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0is not explicitly mentioned. Ensure this removal does not impact any existing functionality that relies on this dependency.
- 1100-1101: Adding
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3introduces new dependencies. Verify their usage aligns with the project's telemetry and logging standards.- 1214-1219: Updating
go.uber.org/multierrfromv1.10.0tov1.11.0andgo.uber.org/zapfromv1.25.0tov1.26.0should be checked for compatibility with existing error handling and logging implementations.services/scribe/go.sum (3)
- 162-167: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0is not explicitly mentioned. Ensure this removal is intentional and does not affect any dependencies requiring this specific version.
- 1098-1099: Updated versions of
github.com/uptrace/opentelemetry-go-extra/otelutilandgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzaptov0.2.3. Verify these updates for compatibility with the project's current usage of OpenTelemetry, especially in terms of new features or breaking changes.- 1212-1217: Updates to
go.uber.org/multierrandgo.uber.org/zapsignificantly jump versions. Specifically,multierrfromv1.6.0tov1.11.0andzapfromv1.23.0tov1.26.0. Ensure these updates are tested for compatibility with existing error handling and logging implementations.go.work.sum (2)
- 1011-1011: Ensure the addition of
github.com/benbjohnson/clock v1.1.0is necessary for the project's dependencies and is used within the codebase.- 1761-1761: Verify the addition of
go.uber.org/goleak v1.2.0/go.modis required for detecting goroutine leaks in tests or the main application, and confirm its usage.services/sinner/go.sum (2)
- 1166-1167: The addition of
github.com/uptrace/opentelemetry-go-extra/otelutilv0.2.3 andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzapv0.2.3 introduces new dependencies. Ensure that these versions are compatible with the rest of the project's dependencies and that they do not introduce breaking changes.- 1280-1285: The update to
go.uber.org/zapfrom an unspecified previous version to v1.26.0 could introduce significant changes, given the jump in version numbers. Verify that this update does not break existing logging functionality and is compatible with the project's logging configuration.agents/go.sum (3)
- 201-206: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0and the absence of a newer version raise concerns about potential dependency issues. Verify that this removal does not affect the functionality relying on this package.
- 1176-1177: The addition of
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andotelzap v0.2.3aligns with enhancing observability. Ensure these versions are compatible with other opentelemetry dependencies in the project.- 1295-1300: The update from
go.uber.org/multierr v1.6.0tov1.11.0andgo.uber.org/zapfromv1.23.0tov1.26.0seems appropriate for maintaining up-to-date dependencies. Confirm that these updates do not introduce breaking changes or compatibility issues with the project's existing codebase.contrib/promexporter/go.sum (3)
- 212-217: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0is not mentioned in the summary. Ensure this removal does not impact any dependencies that might require this specific version.
- 1593-1594: Added
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andotelzap v0.2.3. Confirm these versions are compatible with the project's existing OpenTelemetry instrumentation.- 1768-1776: Updated
go.uber.org/multierrtov1.11.0andgo.uber.org/zaptov1.26.0. Verify these updates do not introduce breaking changes or compatibility issues with the project's current logging and error handling patterns.services/explorer/go.sum (3)
- 233-238: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-6]
The removal of
github.com/benbjohnson/clock v1.3.0and no addition of a newer version might impact modules relying on this dependency for time manipulation or mocking. Verify if this removal is intentional and if any replacement or upgrade is required.
- 1641-1642: The addition of
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3introduces new dependencies for OpenTelemetry instrumentation. Ensure that these versions are compatible with other OpenTelemetry components used in the project to avoid version conflicts or unexpected behavior.- 1816-1824: The upgrade to
go.uber.org/multierr v1.11.0andgo.uber.org/zap v1.26.0introduces newer versions of these libraries. Confirm that these upgrades do not introduce breaking changes or require code modifications elsewhere in the project. Additionally, review the changelogs of these versions for any performance improvements or bug fixes that could benefit the project.
core/metrics/metrics.go
Outdated
Comment on lines
43
to
44
| // ExperimentalLogger returns an experimental logger. | ||
| ExperimentalLogger() ExperimentalLogger |
There was a problem hiding this comment.
Adding ExperimentalLogger() to the Handler interface requires all implementations of this interface to include the new method. Ensure all concrete implementations are updated accordingly.
Comment on lines
41
to
55
| var latestBlock []byte | ||
| var finalizedBlock []byte | ||
|
|
||
| func init() { | ||
| var err error | ||
| latestBlock, err = rpc.LatestBlockNumber.MarshalText() | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not marshall test from latest block number")) | ||
| } | ||
|
|
||
| finalizedBlock, err = rpc.FinalizedBlockNumber.MarshalText() | ||
| if err != nil { | ||
| panic(fmt.Errorf("could not marshall test from finalized block number")) | ||
| } | ||
| } |
There was a problem hiding this comment.
Initialization of latestBlock and finalizedBlock in the init() function is correctly implemented. However, consider handling errors more gracefully than using panic.
- panic(fmt.Errorf("could not marshall test from latest block number"))
+ log.Fatalf("could not marshall test from latest block number: %v", 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.
Suggested change
| var latestBlock []byte | |
| var finalizedBlock []byte | |
| func init() { | |
| var err error | |
| latestBlock, err = rpc.LatestBlockNumber.MarshalText() | |
| if err != nil { | |
| panic(fmt.Errorf("could not marshall test from latest block number")) | |
| } | |
| finalizedBlock, err = rpc.FinalizedBlockNumber.MarshalText() | |
| if err != nil { | |
| panic(fmt.Errorf("could not marshall test from finalized block number")) | |
| } | |
| } | |
| var latestBlock []byte | |
| var finalizedBlock []byte | |
| func init() { | |
| var err error | |
| latestBlock, err = rpc.LatestBlockNumber.MarshalText() | |
| if err != nil { | |
| log.Fatalf("could not marshall test from latest block number: %v", err) | |
| } | |
| finalizedBlock, err = rpc.FinalizedBlockNumber.MarshalText() | |
| if err != nil { | |
| panic(fmt.Errorf("could not marshall test from finalized block number")) | |
| } | |
| } |
There was a problem hiding this comment.
this is done on purpose as a run-time assert. This fails all tests on even being compiled
There was a problem hiding this comment.
@trajan0x: Understood, your rationale for using panic as a run-time assert to ensure that any marshalling issues with latestBlock and finalizedBlock are caught at compile time makes sense. This approach effectively prevents the application from running in an unstable state if critical configurations are not set up correctly. Thank you for the clarification.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 18
Configuration used: .coderabbit.yaml
Files selected for processing (36)
- agents/go.mod (8 hunks)
- agents/go.sum (15 hunks)
- contrib/promexporter/go.mod (7 hunks)
- contrib/promexporter/go.sum (13 hunks)
- contrib/screener-api/go.mod (7 hunks)
- contrib/screener-api/go.sum (19 hunks)
- core/ginhelper/experimental_logger.go (1 hunks)
- core/ginhelper/server.go (4 hunks)
- core/ginhelper/server_test.go (1 hunks)
- core/ginhelper/suite_test.go (3 hunks)
- core/go.mod (8 hunks)
- core/go.sum (21 hunks)
- core/metrics/base.go (4 hunks)
- core/metrics/logger/doc.go (1 hunks)
- core/metrics/logger/experimental.go (1 hunks)
- core/metrics/logger/logger_generated.go (1 hunks)
- core/metrics/metrics.go (2 hunks)
- core/metrics/null.go (2 hunks)
- ethergo/go.mod (8 hunks)
- ethergo/go.sum (15 hunks)
- go.work.sum (9 hunks)
- services/cctp-relayer/go.mod (7 hunks)
- services/cctp-relayer/go.sum (15 hunks)
- services/explorer/go.mod (7 hunks)
- services/explorer/go.sum (13 hunks)
- services/omnirpc/go.mod (8 hunks)
- services/omnirpc/go.sum (16 hunks)
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (1 hunks)
- services/rfq/go.mod (7 hunks)
- services/rfq/go.sum (15 hunks)
- services/scribe/go.mod (7 hunks)
- services/scribe/go.sum (15 hunks)
- services/sinner/go.mod (7 hunks)
- services/sinner/go.sum (15 hunks)
- services/stiprelayer/go.mod (7 hunks)
- services/stiprelayer/go.sum (15 hunks)
Files not summarized due to errors (1)
- core/go.sum: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- core/metrics/logger/doc.go
- core/metrics/logger/logger_generated.go
Files skipped from review as they are similar to previous changes (20)
- agents/go.sum
- contrib/promexporter/go.sum
- contrib/screener-api/go.sum
- core/go.mod
- core/metrics/metrics.go
- ethergo/go.mod
- ethergo/go.sum
- services/cctp-relayer/go.mod
- services/cctp-relayer/go.sum
- services/explorer/go.mod
- services/explorer/go.sum
- services/omnirpc/go.sum
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go
- services/rfq/go.mod
- services/rfq/go.sum
- services/scribe/go.mod
- services/scribe/go.sum
- services/sinner/go.sum
- services/stiprelayer/go.mod
- services/stiprelayer/go.sum
Additional comments: 52
core/metrics/null.go (1)
- 24-26: The
ExperimentalLoggermethod correctly returns a new instance ofNullLogger. Ensure that this aligns with the intended use cases for null logging.core/metrics/base.go (1)
- 47-48: The addition of the
experimentalLoggerfield to thebaseHandlerstruct is correct. Ensure that its usage is consistent with the logging strategy.contrib/screener-api/go.mod (7)
- 40-40: The update of
github.com/bytedance/sonicto v1.10.0 is correct. Ensure compatibility with existing code.- 44-45: The addition of new dependencies
github.com/chenzhuoyu/base64xandgithub.aaakk.us.kg/chenzhuoyu/iasmis noted. Verify their usage in the project for compatibility and security.- 65-65: The update of
github.com/gin-contrib/zapto v0.2.0 is correct. Ensure compatibility with existing logging configurations.- 75-75: The update of
github.com/go-playground/validator/v10to v10.15.3 is correct. Verify that all validation logic remains functional.- 97-97: The update of
github.com/klauspost/cpuid/v2to v2.2.5 is correct. Ensure that there are no performance regressions in CPU feature detection.- 115-115: The update of
github.com/pelletier/go-toml/v2to v2.1.0 is correct. Verify compatibility with existing TOML configurations.- 162-162: The update of
go.uber.org/zapto v1.26.0 is correct. Ensure that all logging functionality remains consistent with the update.contrib/promexporter/go.mod (8)
- 64-64: Ensure
github.com/bytedance/sonicv1.10.0 is compatible with the rest of the project dependencies and Go version 1.21.- 69-70: Added dependencies
github.com/chenzhuoyu/base64xandgithub.aaakk.us.kg/chenzhuoyu/iasm. Verify their usage within the project and ensure there are no known security vulnerabilities with these versions.- 95-95: Updated
github.com/gin-contrib/zapto v0.2.0. Confirm compatibility withgithub.aaakk.us.kg/gin-gonic/ginv1.9.1 and other logging dependencies.- 107-107: Updated
github.com/go-playground/validator/v10to v10.15.3. Check for breaking changes that might affect validation logic in the project.- 138-138: Updated
github.com/klauspost/cpuid/v2to v2.2.5. Ensure this version is compatible with the project's architecture-specific optimizations.- 156-156: Updated
github.com/pelletier/go-toml/v2to v2.1.0. Verify if the new version affects configuration parsing or introduces new features that could be utilized.- 194-195: Updated
github.com/uptrace/opentelemetry-go-extra/otelutilandgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzapto v0.2.3. Confirm these updates do not disrupt existing telemetry and logging configurations.- 212-214: Updated
go.uber.org/multierrto v1.11.0,go.uber.org/zapto v1.26.0, andgolang.org/x/archto v0.5.0. Check for compatibility issues and performance implications of these updates.services/omnirpc/go.mod (8)
- 70-70: Updated
github.com/bytedance/sonicto v1.10.0. Ensure this version is compatible with the project's JSON handling requirements.- 75-76: Added
github.com/chenzhuoyu/base64xandgithub.aaakk.us.kg/chenzhuoyu/iasm. Verify their necessity and ensure no security vulnerabilities exist for these versions.- 106-106: Updated
github.com/gin-contrib/zapto v0.2.0. Confirm this update integrates well with the project's logging setup andgithub.aaakk.us.kg/gin-gonic/ginversion.- 117-117: Updated
github.com/go-playground/validator/v10to v10.15.3. Review for any breaking changes that could affect the project's validation logic.- 156-156: Updated
github.com/klauspost/cpuid/v2to v2.2.5. Check for compatibility with the project's CPU feature detection and optimization code.- 179-179: Updated
github.com/pelletier/go-toml/v2to v2.1.0. Verify the impact on TOML configuration parsing and if new features can be leveraged.- 215-215: Updated
github.com/uptrace/opentelemetry-go-extra/otelutilto v0.2.3. Ensure this update does not affect the project's telemetry setup.- 235-237: Updated
go.uber.org/multierrto v1.11.0,go.uber.org/zapto v1.26.0, andgolang.org/x/archto v0.5.0. Review for compatibility and performance implications.services/sinner/go.mod (6)
- 102-103: Adding new dependencies
github.com/chenzhuoyu/base64xandgithub.aaakk.us.kg/chenzhuoyu/iasm. Ensure these libraries are necessary for the project and assess their security and maintenance status.- 96-96: Updated
github.com/bytedance/sonictov1.10.0. Verify compatibility with the project's existing codebase and other dependencies.- 140-140: Updated
github.com/gin-contrib/zaptov0.2.0. Check for any breaking changes or deprecated features that might affect the project.- 150-150: Updated
github.com/go-playground/validator/v10tov10.15.3. Confirm that the update does not introduce any breaking changes in validation logic used across the project.- 198-198: Updated
github.com/klauspost/cpuid/v2tov2.2.5. Ensure the update is compatible with the hardware and OS environments the project targets.- 227-227: Updated
github.com/pelletier/go-toml/v2tov2.1.0. Validate that the TOML parsing and serialization work as expected with the new version.agents/go.mod (9)
- 49-49: Added
github.com/chenzhuoyu/iasm v0.9.0as an indirect dependency. Ensure this library's version is compatible with the project's existing dependencies and its security status has been verified.- 73-74: Added
github.com/uptrace/opentelemetry-go-extra/otelutil v0.2.3andgithub.aaakk.us.kg/uptrace/opentelemetry-go-extra/otelzap v0.2.3as indirect dependencies. Confirm their compatibility with the project's version of OpenTelemetry and assess if they introduce any performance implications.- 119-119: Updated
github.com/bytedance/sonictov1.10.0. Verify this version's compatibility with the project and check for any breaking changes or deprecated features that might affect the project.- 125-125: Added
github.com/chenzhuoyu/base64x v0.0.0-20230717121745-296ad89f973das an indirect dependency. Ensure this library's version is compatible with the project's existing dependencies and its security status has been verified.- 153-153: Updated
github.com/gin-contrib/zaptov0.2.0. Confirm this update does not introduce any compatibility issues with the version ofgithub.aaakk.us.kg/gin-gonic/ginused in the project.- 164-164: Updated
github.com/go-playground/validator/v10tov10.15.3. Ensure this update is compatible with the project's usage of the validator library and check for any breaking changes in the validation rules or API.- 207-207: Updated
github.com/klauspost/cpuid/v2tov2.2.5. Verify the compatibility of this update, especially if the project relies on specific CPU feature detection functionality.- 234-234: Updated
github.com/pelletier/go-toml/v2tov2.1.0. Confirm that this version is compatible with the project's TOML parsing and writing requirements, and review the change log for potential breaking changes.- 295-297: Updated
go.uber.org/multierrtov1.11.0,go.uber.org/zaptov1.26.0, andgolang.org/x/archtov0.5.0. Ensure these updates are compatible with the project and do not introduce any breaking changes or performance regressions.core/go.sum (1)
- 534-559: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-955]
The
go.sumfile lists checksums for module dependencies. Without specific context or issues to address (e.g., security vulnerabilities, required updates, or compatibility concerns), there's no actionable change to recommend directly on this file. Dependency management typically involves thego.modfile and the actual project code to assess the need for adding, updating, or removing modules.go.work.sum (11)
- 1039-1041: Confirm that
github.com/bytedance/sonicv1.10.0 is the intended version and check for compatibility with existing code, especially since it's marked as a release candidate (-rc) in the go.mod path but not in the version itself.- 1055-1058: The addition of
github.com/chenzhuoyu/base64xandgithub.aaakk.us.kg/chenzhuoyu/iasmintroduces new dependencies. Verify their usage within the project and ensure they are necessary for the intended enhancements.- 1197-1198: The addition of
github.com/gin-contrib/zapv0.2.0 should be checked for compatibility with the current version ofgithub.aaakk.us.kg/gin-gonic/ginused in the project, to avoid potential middleware conflicts.- 1231-1232: Upgrading
github.com/go-playground/validator/v10to v10.15.3 might introduce breaking changes or new validation tags. Review the changelog for this version and test thoroughly.- 1449-1450: The update to
github.com/klauspost/cpuid/v2v2.2.5 should be verified for compatibility with hardware-specific optimizations in the project. Ensure that no regressions are introduced by this update.- 1454-1455: The addition of
github.com/knz/go-libeditv1.10.1 suggests an update or new feature related to CLI interactions. Confirm that this library is used and compatible with the project's CLI framework.- 1593-1594: Upgrading
github.com/pelletier/go-toml/v2to v2.1.0 requires checking for any changes in TOML parsing behavior that might affect configuration file loading or parsing.- 1780-1781: The addition of
go.uber.org/goleakandgo.uber.org/multierras dependencies should be accompanied by their usage in the project. Verify that these libraries are indeed utilized for their intended purposes, such as leak detection and error aggregation.- 1783-1783: Ensure that the update to
go.uber.org/zapv1.25.0 is compatible with the project's logging configuration and does not introduce any breaking changes.- 1785-1786: The update to
golang.org/x/archv0.5.0 should be verified for compatibility with the project's architecture-specific code, especially if the project relies on low-level system or architecture checks.- 1890-1891: The addition of
nullprogram.com/x/optparsev1.0.0 suggests new CLI parsing functionality. Confirm its integration and test for compatibility with existing CLI commands and flags.
Comment on lines
+1011
to
+1013
| github.com/benbjohnson/clock v1.1.0 h1:Q92kusRqC1XV2MjkWETPvjJVqKetz1OzxZB7mHJLju8= | ||
| github.com/benbjohnson/clock v1.3.0 h1:ip6w0uFQkncKQ979AypyG0ER7mqUSBdKLOgAle/AT8A= | ||
| github.com/benbjohnson/clock v1.3.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZxNJlLklBHA= |
There was a problem hiding this comment.
Ensure that the multiple versions of github.com/benbjohnson/clock (v1.1.0 and v1.3.0) are intentional and not a result of a merge conflict or oversight. Typically, only the highest version should be retained unless there's a specific reason for keeping both.
Comment on lines
+10
to
+14
| type wrappedExperimentalLogger struct { | ||
| // nolint: containedctx | ||
| ctx context.Context | ||
| logger logger.ExperimentalLogger | ||
| } |
There was a problem hiding this comment.
The wrappedExperimentalLogger struct correctly encapsulates a context and an experimental logger. However, consider documenting the purpose of ignoring the lint warning on line 11.
Comment on lines
+16
to
+17
| func (w wrappedExperimentalLogger) Info(msg string, fields ...zap.Field) { | ||
| w.logger.WithOptions(zap.Fields(fields...)).Infof(w.ctx, msg) |
There was a problem hiding this comment.
The Info method implementation is correct but lacks error handling for the logger call.
Comment on lines
+20
to
+21
| func (w wrappedExperimentalLogger) Error(msg string, fields ...zap.Field) { | ||
| w.logger.WithOptions(zap.Fields(fields...)).Errorw(w.ctx, msg) |
There was a problem hiding this comment.
The Error method has a typo in the method call: Errorw should be Errorf to match the method signature.
- w.logger.WithOptions(zap.Fields(fields...)).Errorw(w.ctx, msg)
+ w.logger.WithOptions(zap.Fields(fields...)).Errorf(w.ctx, msg)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.
Suggested change
| func (w wrappedExperimentalLogger) Error(msg string, fields ...zap.Field) { | |
| w.logger.WithOptions(zap.Fields(fields...)).Errorw(w.ctx, msg) | |
| func (w wrappedExperimentalLogger) Error(msg string, fields ...zap.Field) { | |
| w.logger.WithOptions(zap.Fields(fields...)).Errorf(w.ctx, msg) |
| w.logger.WithOptions(zap.Fields(fields...)).Errorw(w.ctx, msg) | ||
| } | ||
|
|
||
| var _ ginzap.ZapLogger = &wrappedExperimentalLogger{} |
There was a problem hiding this comment.
The type assertion on line 24 is correct but consider adding a comment explaining its purpose for future maintainability.
Comment on lines
+24
to
+27
| // MakeWrappedSugaredLogger creates a new wrapped sugar logger. | ||
| func MakeWrappedSugaredLogger(sugaredLogger *otelzap.SugaredLogger) ExperimentalLogger { | ||
| return &wrappedSugarLogger{sugaredLogger} | ||
| } |
There was a problem hiding this comment.
The MakeWrappedSugaredLogger function correctly wraps a sugared logger. Ensure that the purpose of wrapping is clearly documented.
Comment on lines
+34
to
+88
| func (l *wrappedSugarLogger) Debugw(ctx context.Context, msg string, keysAndValues ...interface{}) { | ||
| l.underlyingLogger.DebugwContext(ctx, msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Debugf(ctx context.Context, template string, args ...interface{}) { | ||
| l.underlyingLogger.DebugfContext(ctx, template, args...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Infow(ctx context.Context, msg string, keysAndValues ...interface{}) { | ||
| l.underlyingLogger.InfowContext(ctx, msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Infof(ctx context.Context, template string, args ...interface{}) { | ||
| l.underlyingLogger.InfofContext(ctx, template, args...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Warnw(ctx context.Context, msg string, keysAndValues ...interface{}) { | ||
| l.underlyingLogger.WarnwContext(ctx, msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Warnf(ctx context.Context, template string, args ...interface{}) { | ||
| l.underlyingLogger.WarnfContext(ctx, template, args...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Errorw(ctx context.Context, msg string, keysAndValues ...interface{}) { | ||
| l.underlyingLogger.ErrorwContext(ctx, msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Errorf(ctx context.Context, template string, args ...interface{}) { | ||
| l.underlyingLogger.ErrorfContext(ctx, template, args...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Dpanicf(ctx context.Context, msg string, keysAndValues ...interface{}) { | ||
| l.underlyingLogger.DPanicfContext(ctx, msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Dpanicw(ctx context.Context, template string, args ...interface{}) { | ||
| l.underlyingLogger.DPanicwContext(ctx, template, args...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Fatalf(ctx context.Context, msg string, keysAndValues ...interface{}) { | ||
| l.underlyingLogger.FatalfContext(ctx, msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (l *wrappedSugarLogger) Fatalw(ctx context.Context, template string, args ...interface{}) { | ||
| l.underlyingLogger.FatalwContext(ctx, template, args...) | ||
| } | ||
|
|
||
| // WithOptions returns a new ExperimentalLogger with the given options applied. | ||
| func (l *wrappedSugarLogger) WithOptions(opts ...zap.Option) ExperimentalLogger { | ||
| res := l.underlyingLogger.Desugar().WithOptions(opts...) | ||
| return &wrappedSugarLogger{ | ||
| underlyingLogger: res.Sugar(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The methods in wrappedSugarLogger correctly implement context-aware logging. Ensure that error handling is consistent across all methods.
Comment on lines
+92
to
+152
| // NewNullLogger creates a new null logger. | ||
| func NewNullLogger() ExperimentalLogger { | ||
| logger := MakeZapLogger() | ||
| return &nullLogger{underlyingLogger: logger.Sugar()} | ||
| } | ||
|
|
||
| type nullLogger struct { | ||
| underlyingLogger *zap.SugaredLogger | ||
| } | ||
|
|
||
| func (n nullLogger) Debugw(_ context.Context, msg string, keysAndValues ...interface{}) { | ||
| n.underlyingLogger.Debugw(msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (n nullLogger) Debugf(_ context.Context, template string, args ...interface{}) { | ||
| n.underlyingLogger.Debugf(template, args...) | ||
| } | ||
|
|
||
| func (n nullLogger) Infow(_ context.Context, msg string, keysAndValues ...interface{}) { | ||
| n.underlyingLogger.Infow(msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (n nullLogger) Infof(_ context.Context, template string, args ...interface{}) { | ||
| n.underlyingLogger.Infof(template, args...) | ||
| } | ||
|
|
||
| func (n nullLogger) Warnw(_ context.Context, msg string, keysAndValues ...interface{}) { | ||
| n.underlyingLogger.Warnw(msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (n nullLogger) Warnf(_ context.Context, template string, args ...interface{}) { | ||
| n.underlyingLogger.Warnf(template, args...) | ||
| } | ||
|
|
||
| func (n nullLogger) Errorw(_ context.Context, msg string, keysAndValues ...interface{}) { | ||
| n.underlyingLogger.Errorw(msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (n nullLogger) Errorf(_ context.Context, template string, args ...interface{}) { | ||
| n.underlyingLogger.Errorf(template, args...) | ||
| } | ||
|
|
||
| func (n nullLogger) Dpanicf(_ context.Context, msg string, keysAndValues ...interface{}) { | ||
| n.underlyingLogger.DPanicf(msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (n nullLogger) Dpanicw(_ context.Context, template string, args ...interface{}) { | ||
| n.underlyingLogger.DPanicw(template, args...) | ||
| } | ||
|
|
||
| func (n nullLogger) Fatalf(_ context.Context, msg string, keysAndValues ...interface{}) { | ||
| n.underlyingLogger.Fatalf(msg, keysAndValues...) | ||
| } | ||
|
|
||
| func (n nullLogger) Fatalw(_ context.Context, template string, args ...interface{}) { | ||
| n.underlyingLogger.Fatalw(template, args...) | ||
| } | ||
|
|
||
| func (n nullLogger) WithOptions(opts ...zap.Option) ExperimentalLogger { | ||
| return nullLogger{n.underlyingLogger.WithOptions(opts...)} | ||
| } |
There was a problem hiding this comment.
The nullLogger implementation is correct. Consider adding a brief documentation comment explaining its use case.
Comment on lines
+165
to
+167
| func (b *baseHandler) ExperimentalLogger() experimentalLogger.ExperimentalLogger { | ||
| return b.experimentalLogger | ||
| } |
There was a problem hiding this comment.
The ExperimentalLogger method correctly exposes the experimental logger. Document its intended use cases for clarity.
Comment on lines
+215
to
+236
| betaLogger := experimentalLogger.MakeZapLogger() | ||
| // set the global logger to the beta logger | ||
| otelLogger := otelzap.New(betaLogger) | ||
| sugaredLogger := otelLogger.Sugar() | ||
| // wrapped sugar logger adds another level of depth | ||
| otelzap.WithCallerDepth(1) | ||
|
|
||
| if core.GetEnvBool("ENABLE_EXPERIMENTAL_ZAP_GLOBALS", false) { | ||
| // set the global logger to the beta logger | ||
| undo := otelzap.ReplaceGlobals(otelLogger) | ||
| defer undo() | ||
| } | ||
|
|
||
| // note: meter purposely is not registered until startup. | ||
| return &baseHandler{ | ||
| resource: rsr, | ||
| tp: tracerProvider, | ||
| tracer: tracer, | ||
| name: buildInfo.Name(), | ||
| propagator: propagator, | ||
| handler: promhttp.Handler(), | ||
| resource: rsr, | ||
| tp: tracerProvider, | ||
| tracer: tracer, | ||
| name: buildInfo.Name(), | ||
| propagator: propagator, | ||
| handler: promhttp.Handler(), | ||
| experimentalLogger: experimentalLogger.MakeWrappedSugaredLogger(sugaredLogger), |
There was a problem hiding this comment.
The initialization of the experimental logger and its configuration is correct. Consider documenting the impact of enabling experimental Zap globals for clarity and potential side effects.
trajan0x
force-pushed
the
feat/latest-buildaround
branch
from
98917ca to
805728f
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Commits
Files that changed from the base of the PR and between 7ec0bb9 and 98917ca1b275cb97031a2d973d1d72c77d962841.Files selected for processing (29)
- .github/workflows/go.yml (2 hunks)
- .github/workflows/goreleaser-actions.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- .golangci.yml (1 hunks)
- agents/go.sum (16 hunks)
- contrib/git-changes-action/go.sum (2 hunks)
- contrib/promexporter/go.sum (14 hunks)
- contrib/release-copier-action/go.sum (2 hunks)
- contrib/screener-api/client/client.go (1 hunks)
- contrib/screener-api/db/sql/sqlite/sqlite.go (2 hunks)
- contrib/screener-api/go.sum (20 hunks)
- core/ginhelper/experimental_logger.go (1 hunks)
- ethergo/go.sum (16 hunks)
- ethergo/parser/rpc/rpc.go (1 hunks)
- services/cctp-relayer/go.sum (16 hunks)
- services/explorer/go.sum (14 hunks)
- services/omnirpc/cmd/commands.go (2 hunks)
- services/omnirpc/cmd/flags.go (1 hunks)
- services/omnirpc/go.mod (7 hunks)
- services/omnirpc/go.sum (17 hunks)
- services/omnirpc/modules/confirmedtofinalized/const.go (1 hunks)
- services/omnirpc/modules/confirmedtofinalized/doc.go (1 hunks)
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (1 hunks)
- services/omnirpc/proxy/confirmable.go (2 hunks)
- services/rfq/go.sum (16 hunks)
- services/scribe/go.sum (16 hunks)
- services/sinner/go.sum (16 hunks)
- services/stiprelayer/go.sum (16 hunks)
- tools/go.sum (2 hunks)
Files not summarized due to errors (1)
- contrib/screener-api/go.sum: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review due to trivial changes (1)
- services/omnirpc/modules/confirmedtofinalized/doc.go
Files skipped from review as they are similar to previous changes (18)
- agents/go.sum
- contrib/git-changes-action/go.sum
- contrib/promexporter/go.sum
- contrib/release-copier-action/go.sum
- core/ginhelper/experimental_logger.go
- ethergo/go.sum
- services/cctp-relayer/go.sum
- services/explorer/go.sum
- services/omnirpc/cmd/commands.go
- services/omnirpc/cmd/flags.go
- services/omnirpc/go.mod
- services/omnirpc/go.sum
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go
- services/rfq/go.sum
- services/scribe/go.sum
- services/sinner/go.sum
- services/stiprelayer/go.sum
- tools/go.sum
Additional comments: 9
contrib/screener-api/client/client.go (1)
- 27-27: The change to replace the
clientparameter with_in theOnBeforeRequestfunction is a good practice for indicating unused variables.contrib/screener-api/db/sql/sqlite/sqlite.go (1)
- 37-37: Changing error creation from
fmt.Errorftoerrors.NewinNewSqliteStoreis appropriate since the error message does not require formatting.ethergo/parser/rpc/rpc.go (1)
- 40-40: Adding the
JSONRPCfield with theomitemptytag to theRequeststruct is a good enhancement for flexibility and compatibility with different RPC servers..github/workflows/lint.yml (1)
- 36-36: Updating the
actions/upload-artifactaction fromv2tov4is recommended to take advantage of the latest features and improvements.services/omnirpc/proxy/confirmable.go (1)
- 12-12: Changing the package alias to
ethergoRPCimproves clarity and readability in distinguishing between different RPC-related packages..golangci.yml (1)
- 96-101: Adding comments to explain the reasons for disabling specific linters (
testifylint,spancheck,perfsprint) improves the maintainability and clarity of the linter configuration..github/workflows/goreleaser-actions.yml (1)
- 262-262: Updating the
actions/upload-artifactaction fromv3tov4in the "Push Artifacts (Snapshot)" step aligns with best practices for using up-to-date actions in workflows..github/workflows/go.yml (2)
- 254-254: Updating
actions/upload-artifacttov4is a good practice to ensure the use of the latest features and security patches.- 349-349: Updating
golangci-linttov1.56.1ensures the use of the latest linter features and bug fixes. Verify that all linting rules are compatible with this version.Verification successful
Updating
golangci-linttov1.56.1in the.github/workflows/go.ymlfile does not explicitly involve custom linting rules or configurations based on the provided context. The configuration specifies the version and some general settings like timeout and environment variables, but no custom linting rules are mentioned. Therefore, it can be concluded that the update is focused on using the latest version of the linter without directly affecting custom linting rules.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any custom linting rules that may be affected by the golangci-lint version update. rg --type yaml "golangci-lint" .github/workflows/Length of output: 846
Script:
#!/bin/bash # Inspect the golangci-lint configuration within the go.yml workflow file for custom linting rules or settings. cat .github/workflows/go.yml | grep -A 20 "golangci-lint"Length of output: 1884
Comment on lines
8
to
9
| var latestBlock []byte | ||
| var finalizedBlock []byte |
There was a problem hiding this comment.
Using global variables for latestBlock and finalizedBlock can lead to unpredictable behavior and make the code harder to maintain. Consider encapsulating these within a struct and providing access through methods.
Comment on lines
+14
to
+15
| if err != nil { | ||
| panic(errors.New("could not marshall test from latest block number")) |
There was a problem hiding this comment.
Using panic for error handling in the init function is not recommended for production code. Consider logging the error and handling it gracefully.
Also applies to: 19-20
| "golang.org/x/exp/slices" | ||
| "math/big" | ||
| ) | ||
|
|
||
| func isConfirmable(r rpc2.Request) (bool, error) { | ||
| func isConfirmable(r ethergoRPC.Request) (bool, error) { |
There was a problem hiding this comment.
The TODO comments in the isConfirmable function suggest areas that require further decision-making or implementation. It's important to address these to ensure robust and predictable behavior.
Would you like assistance in resolving these TODO items?
trajan0x
force-pushed
the
feat/latest-buildaround
branch
from
dc2f489 to
b3e2d0c
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
latest-rewriteto automatically convert "latest" block queries to "finalized" for more reliable RPC responses in tests.Bug Fixes
RPCProxystruct documentation for better clarity.Refactor
SimpleProxystructure with comprehensive methods for request handling, including metrics and tracing functionalities.