Improve RFQ propogation #1973
Improve RFQ propogation #1973
Conversation
WalkthroughThe recent updates focus on enhancing traceability, error handling, and testing within the RFQ service. Key modifications include the introduction of OpenTelemetry propagators, middleware for improved server functionality, refined context and error handling in quote generation and submission, adjustments in inventory management error handling, and the addition of a new linting tool for better code quality assurance. An integration test for ETH to ETH transactions has also been introduced, emphasizing the project's commitment to robustness and reliability. 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 (
|
trajan0x
force-pushed
the
fix/propogator-rfq-api
branch
from
3c9b245 to
a4658d8
Compare
![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: 0
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 7e30b1e and 3c9b245ff08c4f5586517123b9561af049b12f07.Files selected for processing (1)
- services/rfq/api/client/client.go (2 hunks)
Additional comments: 13
services/rfq/api/client/client.go (13)
- 83-83: The
otelresty.TraceClientcall correctly adds theWithPropagatorsoption to enhance tracing. Ensure thatmetrics.Propagator()is properly configured to return a compatible propagator for OpenTelemetry.- 100-100: The
otelresty.TraceClientcall inNewUnauthenticaedClientcorrectly includes theWithPropagatorsoption. Verify thatmetricHandler.Propagator()returns the expected propagator instance for tracing.- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The package declaration and file comments are clear and relevant to the file's purpose.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [7-14]
Imports are correctly organized and relevant to the functionality provided by this client file. No unused imports are detected.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [16-29]
Interface definitions for
AuthenticatedClientandUnauthenticatedClientare clear and well-documented. Ensure that all methods are implemented as required.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [31-37]
The
unauthenticatedClientstruct and itsrestymethod are correctly implemented. This method provides encapsulated access to theresty.Client.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [39-45]
The
clientImplstruct correctly embeds theUnauthenticatedClientinterface and holds a*resty.Client. This design supports both authenticated and unauthenticated client functionalities.
- 80-87: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [47-84]
In
NewAuthenticatedClient, the error handling, request signing, and tracing setup are correctly implemented. Pay attention to the TODO comment about GET requests not requiring authentication, which might need further action or removal based on current API requirements.
- 97-103: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [86-100]
NewUnauthenticaedClientfunction correctly initializes a newresty.Client, sets the base URL, and adds a request ID header to each request. The tracing enhancement is appropriately applied.
- 97-103: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [102-118]
The
PutQuotemethod correctly performs a PUT request to the RFQ API. The TODO comment about handling the response should be addressed or removed if no action is deemed necessary.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [120-138]
GetAllQuotesmethod correctly retrieves all quotes from the RFQ API, with proper error handling and response parsing.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [140-160]
GetSpecificQuotemethod correctly retrieves a specific quote based on the provided request parameters, with appropriate error handling and response parsing.
- 80-87: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [162-182]
GetQuoteByRelayerAddressmethod correctly retrieves quotes by relayer address, with proper error handling and response parsing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1973 +/- ##
===================================================
- Coverage 48.80073% 47.76756% -1.03317%
===================================================
Files 370 371 +1
Lines 27225 26966 -259
Branches 83 83
===================================================
- Hits 13286 12881 -405
- Misses 12554 12748 +194
+ Partials 1385 1337 -48
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
trajan0x
force-pushed
the
fix/propogator-rfq-api
branch
from
49f1873 to
85f95fe
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- services/rfq/api/client/client.go (2 hunks)
- services/rfq/api/rest/server.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/api/client/client.go
Additional comments: 3
services/rfq/api/rest/server.go (1)
- 91-91: Ensure that the middleware added for tracing (
r.handler.Gin()) properly handles all scenarios, including error states and non-200 responses, to ensure comprehensive trace coverage.services/rfq/relayer/quoter/quoter.go (2)
- 248-252: The use of
deferfor ending the span ingenerateQuotesis correct and ensures that the span is ended properly even if an error occurs or the function returns early. This is a good practice for consistent trace handling.- 248-252: Verify that all error messages returned from
generateQuotesare clear and provide enough context for debugging. Avoid exposing internal implementation details or sensitive information in error messages.
| func (m *Manager) generateQuotes(parentCtx context.Context, chainID int, address common.Address, balance *big.Int) (_ []model.PutQuoteRequest, err error) { | ||
| ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "isProfitableQuote", trace.WithAttributes(attribute.Int(metrics.ChainID, chainID))) | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() |
There was a problem hiding this comment.
Consider adding more detailed trace attributes before returning errors or at critical decision points within generateQuotes to enhance traceability and debugging capabilities.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- services/rfq/relayer/inventory/manager.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/quoter/quoter.go
Additional comments: 1
services/rfq/relayer/inventory/manager.go (1)
- 243-245: The change in the
deferfunction signature fromfunc(err error)tofunc()removes the direct handling of errors within thedeferstatement. Verify the intention behind this change, especially considering the impact on error handling and tracing. Ensure that this modification aligns with the desired error reporting and tracing strategy.
This reverts commit 62ce2cb.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .golangci.yml (1 hunks)
- services/rfq/e2e/rfq_test.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/quoter/quoter.go
Additional comments: 2
.golangci.yml (1)
- 96-96: Added
testifylintto the linters list. Ensure that existing tests comply withtestifylintrules to avoid a surge in linting errors.services/rfq/e2e/rfq_test.go (1)
- 202-202: Conditional skip based on an environment variable (
CI) is used inTestETHtoETH. Ensure that this approach aligns with the project's testing strategy, especially for CI environments.
services/rfq/e2e/rfq_test.go
Outdated
| @@ -199,6 +199,7 @@ func (i *IntegrationSuite) TestUSDCtoUSDC() { | |||
| }) | |||
| } | |||
|
|
|||
| // nolint: cyclop | |||
There was a problem hiding this comment.
The use of nolint: cyclop disables cyclomatic complexity checks for TestETHtoETH. Consider refactoring to reduce complexity or justify the need for complexity in this test.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- services/rfq/relayer/inventory/manager.go (1 hunks)
- services/rfq/relayer/quoter/quoter.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/inventory/manager.go
- services/rfq/relayer/quoter/quoter.go
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
| func (m *Manager) SubmitAllQuotes(parentCtx context.Context) (err error) { | ||
| timeCtx, cancel := context.WithTimeout(parentCtx, time.Minute*2) | ||
|
|
||
| ctx, span := m.metricsHandler.Tracer().Start(timeCtx, "submitQuotes") | ||
| defer func() { | ||
| logger.Error(err) | ||
| metrics.EndSpanWithErr(span, err) | ||
| cancel() |
There was a problem hiding this comment.
The introduction of a new time context (timeCtx) in the SubmitAllQuotes function is a good practice for managing long-running operations. However, logging the error directly within the defer function (logger.Error(err)) might not be ideal as it could log nil errors or errors from outside the scope of this function due to the named return value pattern. Consider logging errors where they occur to ensure clarity and accuracy in error reporting.
| func (m *Manager) prepareAndSubmitQuotes(parentCtx context.Context, inv map[int]map[common.Address]*big.Int) (err error) { | ||
| ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "prepareAndSubmitQuotes") | ||
| defer func() { | ||
| logger.Error(err) | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() |
There was a problem hiding this comment.
In the prepareAndSubmitQuotes function, the same issue with error logging within the defer function is observed. This could lead to logging nil or unrelated errors. It's recommended to handle and log errors closer to where they occur, which can provide more context and make the logs more meaningful.
| // nolint: nestif, cyclop | ||
| func (m *Manager) generateQuotes(parentCtx context.Context, chainID int, address common.Address, balance *big.Int) (_ []model.PutQuoteRequest, err error) { | ||
| ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "generateQuotes", trace.WithAttributes(attribute.Int(metrics.ChainID, chainID))) | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() |
There was a problem hiding this comment.
The generateQuotes function correctly sets up a new context and span for tracing. However, the comment // nolint: nestif, cyclop indicates a potential complexity issue within the function. While disabling lint warnings might be necessary in some cases, it's generally a good practice to refactor complex functions to reduce their cognitive load and improve maintainability. Consider breaking down this function into smaller, more manageable pieces if possible.
| defer func() { | ||
| logger.Error(err) |
There was a problem hiding this comment.
Logging errors directly within a defer function can lead to misleading error logs, especially when using named return values. It's possible for the err variable to be nil or to reflect an error from a different scope. Consider moving error logging to the specific error handling blocks where the context of the error is clear.
| func (m *Manager) prepareAndSubmitQuotes(parentCtx context.Context, inv map[int]map[common.Address]*big.Int) (err error) { | ||
| ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "prepareAndSubmitQuotes") | ||
| defer func() { | ||
| logger.Error(err) |
There was a problem hiding this comment.
Similar to the previous comment, logging within a defer block in the prepareAndSubmitQuotes function can lead to inaccurate error logging. It's advisable to log errors closer to their source to ensure the logs accurately reflect the operation's outcome and context.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Description
Fix tracingbetween req-relayer and rfq-api
Summary by CodeRabbit
New Features
Refactor
Chores
testifylintfor improved code quality checks.