[SLT-331] feat(omnirpc): omnirpc request tracing #3289
Conversation
WalkthroughThe changes in this pull request enhance the functionality of various structures and methods within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
cf9763f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b364e3d8.sanguine-fe.pages.dev |
| Branch Preview URL: | https://omnirpc-request-tracing.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3289 +/- ##
====================================================
- Coverage 93.56415% 30.71720% -62.84695%
====================================================
Files 94 447 +353
Lines 2455 29866 +27411
Branches 356 82 -274
====================================================
+ Hits 2297 9174 +6877
- Misses 149 19838 +19689
- Partials 9 854 +845
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: 17
🧹 Outside diff range and nitpick comments (8)
services/omnirpc/http/client.go (1)
Line range hint
18-32: LGTM: NewWithMetricsmethod added. Consider updating the TODO comment.The new
WithMetricsmethod is a good addition to theRequestinterface, following the existing builder pattern. It allows for setting metrics handlers, which aligns with the PR objectives for request tracing.Consider updating the TODO comment above the
Requestinterface. It mentions needing support for tracing, which this change might partially address. If tracing is now supported, you may want to remove or update this comment.services/omnirpc/proxy/forward.go (2)
8-10: LGTM! Consider grouping standard library imports.The changes to the import statements look good. Importing
net/httpasgoHTTPhelps avoid potential naming conflicts, and adding thestringspackage is likely necessary for new string operations.Consider grouping standard library imports together at the top, followed by third-party imports. This can improve readability:
import ( goHTTP "net/http" "strings" "github.com/ImVexed/fasturl" // ... other third-party imports )
133-133: Good addition of error return. Consider naming the first return value.The updated method signature with an error return is a good practice for better error handling.
Consider naming the first return value instead of using the blank identifier
_. This can improve code readability and self-documentation:func (f *Forwarder) forwardRequest(parentCtx context.Context, endpoint string) (resp *rawResponse, err error)This way, it's clear what the method is returning even without looking at the implementation.
services/omnirpc/http/resty.go (1)
68-75: Redundant event logging in 'SetContext' methodIn the
SetContextmethod (lines 68-75), after starting the span named "SetContext", an event "SetContext" is added to the span. This may be redundant since the span itself already provides the context of the operation.Consider removing the
span.AddEvent("SetContext")call or adding more meaningful events that provide additional insights.services/omnirpc/http/capture_client.go (1)
114-119: Correct attribute keys inSetHeaderBytesmethod.In the
SetHeaderBytesmethod, the attribute key for the header name is"key", while inSetHeader, it's"SetHeader". For consistency and clarity, consider using"SetHeaderBytes"as the attribute key for the header name.Apply this change for consistency:
_, span := c.Handler.Tracer().Start( c.Context, "SetHeaderBytes", - trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))), + trace.WithAttributes(attribute.String("SetHeaderBytes", common.Bytes2Hex(key))), trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))), )services/omnirpc/proxy/standardize.go (3)
Line range hint
149-149: Consider implementing the TODO for struct pooling.There's a TODO comment suggesting the use of
sync.Poolfor acquiring and releasing structs. Implementing this can optimize memory usage and improve performance by reusing objects instead of allocating new ones frequently.Would you like assistance in implementing this optimization or opening a GitHub issue to track this task?
Line range hint
153-153: Correct the usage oferrgroup.WithContext.The function
errgroup.WithContextreturns an*errgroup.Groupand acontext.Context, but currently, the code assigns the group togroupCtxand discards the context:groupCtx, _ := errgroup.WithContext(ctx)This can be confusing and potentially error-prone. Update the variable names to reflect their purposes and ensure the context is used appropriately:
- groupCtx, _ := errgroup.WithContext(ctx) + group, groupCtx := errgroup.WithContext(ctx)Apply this diff to fix the variable assignment and usage:
- groupCtx, _ := errgroup.WithContext(ctx) + group, groupCtx := errgroup.WithContext(ctx) ... - groupCtx.Go(func() error { + group.Go(func() error { ... - err = groupCtx.Wait() + err = group.Wait()
Line range hint
164-164: Avoid variable shadowing oferrvariable.The declaration
err := json.Unmarshal(req.Params[1], &txFlag)shadows theerrvariable from the outer scope. This can lead to unintended behavior and make error handling confusing. Assign the error to the existingerrvariable instead:- err := json.Unmarshal(req.Params[1], &txFlag) + err = json.Unmarshal(req.Params[1], &txFlag)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- services/omnirpc/http/capture_client.go (2 hunks)
- services/omnirpc/http/client.go (2 hunks)
- services/omnirpc/http/fasthttp.go (5 hunks)
- services/omnirpc/http/resty.go (3 hunks)
- services/omnirpc/proxy/forward.go (3 hunks)
- services/omnirpc/proxy/forwarder.go (3 hunks)
- services/omnirpc/proxy/standardize.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (10)
services/omnirpc/http/client.go (2)
7-7: LGTM: New import for metrics package.The addition of the metrics package import is appropriate for the new
WithMetricsmethod.
31-32: Verify implementation ofWithMetricsin concrete types.The addition of the
WithMetricsmethod to theRequestinterface is non-breaking, but it requires updates to all existing implementations of this interface.Please ensure that all concrete types implementing the
Requestinterface have been updated to include this new method. Run the following script to verify:services/omnirpc/proxy/forward.go (1)
162-162: Good addition of metrics tracking. Please provide more context.The addition of
WithMetrics(f.handler)in the request chain is a good improvement for tracking metrics during the request lifecycle.Could you provide more information about the
WithMetricsmethod? Specifically:
- What kind of metrics does it track?
- How are these metrics used?
- Is there documentation for this method?
To help verify the implementation and usage of
WithMetrics, please run the following script:services/omnirpc/http/resty.go (2)
96-103: Potential issue with 'endpoint' not being setIn the
SetRequestURImethod (lines 96-103), you set theendpointfield without validating the inputuri. If an invalid or emptyuriis provided, it could lead to unexpected behavior whenDo()is called.Please verify that
uriis valid before settingr.endpoint. Consider adding validation or handling for invalid URIs.
117-120: Error wrapping in 'Do' method improves error contextIn the
Domethod (lines 117-120), the error returned byr.Request.Post(r.endpoint)is wrapped with additional context usingfmt.Errorf. This enhances error messages and aids in debugging.The error handling is appropriately enhanced, improving the clarity of error messages.
services/omnirpc/proxy/standardize.go (2)
7-7: Approved: Separation of standard library and external imports.Adding a blank line after standard library imports improves readability by clearly separating them from external imports. This follows Go's convention for grouping imports.
38-38: Approved: Updated linter directive enhances clarity.Changing the linter directive to
//lint:ignore U1000 it's okay.specifies the exact linter warning to suppress (U1000refers to "unused code" in some linters likestaticcheck). This provides better clarity and control over linter warnings.services/omnirpc/proxy/forwarder.go (3)
12-15: Imports Updated to Include Necessary PackagesThe import statements have been updated to include the required packages for the modified code, ensuring that all dependencies are correctly imported.
Also applies to: 20-20
55-56: Addition of Metrics Handler toForwarderStructThe
Forwarderstruct now includes ahandler metrics.Handlerfield, replacing the previoustracerfield. This change aligns with the new approach to metrics handling and tracing.
82-84: Correct Initialization ofhandlerField inAcquireForwarderIn the
AcquireForwardermethod, theForwarderis now initialized with thehandlerfield set tor.handler, ensuring that the metrics handler is properly assigned. This update is consistent with the changes made to theForwarderstruct.
| @@ -19,6 +25,7 @@ func NewRestyClient() Client { | |||
| type restyRequest struct { | |||
| *resty.Request | |||
| endpoint string | |||
| handler metrics.Handler | |||
There was a problem hiding this comment.
Initialize 'handler' to prevent nil pointer dereference
At line 28, the handler field is added to the restyRequest struct but may not be initialized by default. If WithMetrics() is not called before other methods, handler will be nil. This can lead to a nil pointer dereference when methods like SetHeaderBytes, SetBody, SetContext, SetHeader, SetRequestURI, and Do attempt to use r.handler.
Consider ensuring that handler is properly initialized. You could initialize handler with a default metrics.Handler in the NewRequest() method or enforce that WithMetrics() must be called before any other method by adding checks or documentation.
services/omnirpc/http/resty.go
Outdated
| func (r *restyRequest) WithMetrics(metrics metrics.Handler) Request { | ||
| r.handler = metrics | ||
| return r |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the usage of 'WithMetrics' method
The WithMetrics method (lines 123-125) is crucial for initializing the handler. However, its usage is not enforced or explicitly documented, which might lead to handler being nil.
Consider updating the documentation to emphasize that WithMetrics must be called before any other method that uses handler. Alternatively, modify the NewRequest method to require a metrics.Handler parameter, ensuring handler is always initialized.
| _, span := c.Handler.Tracer().Start( | ||
| c.Context, | ||
| "SetBody", | ||
| trace.WithAttributes(attribute.String("SetBody", common.Bytes2Hex(body))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Handle potential nil references for c.Handler.
In the methods SetBody, SetHeader, SetHeaderBytes, SetRequestURI, and Do, there is a possibility of a nil pointer dereference if c.Handler is not set before invoking c.Handler.Tracer(). Ensure that c.Handler is properly initialized before these methods are called, or add nil checks to prevent runtime panics.
Consider adding a nil check at the beginning of each method:
+ if c.Handler == nil {
+ // Handle nil Handler appropriately, e.g., initialize it or return an error.
+ return c
+ }Alternatively, ensure that c.Handler is always set by mandating the use of WithMetrics before these methods are called.
Also applies to: 99-107, 114-122, 129-136, 143-159
Ensure c.Context is initialized before usage in tracing.
In methods like SetBody, SetHeader, SetHeaderBytes, SetRequestURI, and Do, c.Context is used to start tracing spans. If SetContext has not been called prior to these methods, c.Context may be nil, leading to unexpected behavior.
Initialize c.Context with a default context in the CapturedRequest struct:
// During CapturedRequest initialization
CapturedRequest{
Client: c,
StringHeaders: make(map[string]string),
+ Context: context.Background(),
}Alternatively, check for nil and initialize within each method:
+ if c.Context == nil {
+ c.Context = context.Background()
+ }Also applies to: 99-107, 114-122, 129-136, 143-159
| // Metrics is the metrics handler | ||
| Handler metrics.Handler |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce the association of metrics.Handler with CapturedRequest.
To prevent issues with uninitialized c.Handler, consider requiring the metrics.Handler to be set during the creation of CapturedRequest instances.
Modify the NewRequest method to accept a metrics.Handler parameter:
// NewRequest creates a new request.
-func (c *CaptureClient) NewRequest() Request {
+func (c *CaptureClient) NewRequest(handler metrics.Handler) Request {
request := CapturedRequest{
Client: c,
StringHeaders: make(map[string]string),
+ Handler: handler,
+ Context: context.Background(),
}
c.requests = append(c.requests, &request)
return &request
}This ensures that every CapturedRequest has a valid Handler and Context from the beginning.
Also applies to: 161-164
| _, span := c.Handler.Tracer().Start( | ||
| c.Context, | ||
| "SetContext", | ||
| ) | ||
| span.AddEvent("SetContext") | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Use provided context in SetContext for tracing.
In the SetContext method, the tracing span is started using c.Context, which has not been updated yet. This leads to the span being associated with the old context instead of the new one provided as ctx.
Modify the code to use the provided ctx when starting the span:
func (c *CapturedRequest) SetContext(ctx context.Context) Request {
- _, span := c.Handler.Tracer().Start(
- c.Context,
+ _, span := c.Handler.Tracer().Start(
+ ctx,
"SetContext",
)
span.AddEvent("SetContext")
defer func() {
metrics.EndSpan(span)
}()
c.Context = ctx
return c
}📝 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.
| _, span := c.Handler.Tracer().Start( | |
| c.Context, | |
| "SetContext", | |
| ) | |
| span.AddEvent("SetContext") | |
| defer func() { | |
| metrics.EndSpan(span) | |
| }() | |
| _, span := c.Handler.Tracer().Start( | |
| ctx, | |
| "SetContext", | |
| ) | |
| span.AddEvent("SetContext") | |
| defer func() { | |
| metrics.EndSpan(span) | |
| }() |
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetHeader", | ||
| trace.WithAttributes(attribute.String("SetHeader", key)), | ||
| trace.WithAttributes(attribute.String("value", value)), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Potential nil pointer dereference and exposure of sensitive data in SetHeader
In SetHeader, f.handler may be nil, which can cause a nil pointer dereference when invoking f.handler.Tracer(). Moreover, including header keys and values in tracing attributes may expose sensitive information.
Add nil checks for f.handler or ensure it's initialized before use. Be cautious about including header values in tracing attributes, and avoid logging sensitive headers.
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetHeaderBytes", | ||
| trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))), | ||
| trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Potential nil pointer dereference and exposure of sensitive data in SetHeaderBytes
In SetHeaderBytes, f.handler may be nil, leading to a nil pointer dereference when calling f.handler.Tracer(). Additionally, logging header keys and values may expose sensitive data.
Ensure f.handler is not nil before use or add nil checks. Reconsider including header data in tracing attributes if it may contain sensitive information.
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetRequestURI", | ||
| trace.WithAttributes(attribute.String("uri", uri)), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Potential nil pointer dereference in SetRequestURI
In SetRequestURI, f.handler may be nil, leading to a nil pointer dereference when calling f.handler.Tracer().
Ensure that f.handler is initialized before use or add nil checks to prevent panic.
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "Do", | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Potential nil pointer dereference in Do method
In Do, f.handler may be nil, leading to a nil pointer dereference when calling f.handler.Tracer().
Ensure that f.handler is initialized before use or add nil checks.
services/omnirpc/http/fasthttp.go
Outdated
| func (f *fastHTTPRequest) WithMetrics(metrics metrics.Handler) Request { | ||
| f.handler = metrics | ||
| return f | ||
| } |
There was a problem hiding this comment.
Ensure WithMetrics is called before using the request
The method WithMetrics sets the handler for metrics and tracing. Ensure that WithMetrics is called on fastHTTPRequest instances before invoking methods that rely on f.handler to prevent nil pointer dereferences.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (7)
services/omnirpc/http/client.go (2)
31-32: LGTM: New WithMetrics method added.The
WithMetricsmethod is a good addition to theRequestinterface, allowing for metrics handling to be associated with requests. The method signature is consistent with the builder pattern used in other methods of the interface.Consider adding a brief comment to describe the purpose and usage of this method, similar to the other methods in the interface. For example:
// WithMetrics sets the metrics handler for the request WithMetrics(handler metrics.Handler) Request
Tracing Support in Progress
There is an ongoing implementation of tracing support as indicated by PR #3289. However, the TODO comment in
services/omnirpc/http/client.gostill needs to be addressed. Please ensure that the TODO is removed or updated once the tracing feature is fully integrated.🔗 Analysis chain
Line range hint
19-19: Consider implementing tracing support.There's an existing TODO comment about supporting tracing. While the new
WithMetricsmethod improves observability, it doesn't fully address the tracing requirement. Consider implementing tracing support to complement the metrics functionality and provide a more comprehensive observability solution.To check if tracing is implemented elsewhere or if there are any ongoing efforts, you can run the following script:
This script will help identify any existing tracing implementations, ongoing efforts, or related issues/PRs in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for tracing implementation or ongoing efforts # Search for tracing-related code or comments echo "Searching for tracing-related code or comments:" rg --type go -i "trac(e|ing)" -g '!vendor/' # Search for TODO comments related to tracing echo "Searching for TODO comments related to tracing:" rg --type go "TODO.*trac(e|ing)" -g '!vendor/' # Search for any open issues or PRs related to tracing echo "Searching for open issues or PRs related to tracing:" gh issue list --search "tracing in:title,body" gh pr list --search "tracing in:title,body"Length of output: 72657
services/omnirpc/proxy/forward.go (2)
8-10: LGTM with a minor suggestion.The import changes look good. Importing
net/httpasgoHTTPhelps avoid naming conflicts. However, consider grouping standard library imports and third-party imports for better readability.Consider reorganizing imports like this:
import ( "bytes" "context" "crypto/sha256" "fmt" goHTTP "net/http" "strings" "github.com/ImVexed/fasturl" // ... other third-party imports )
Line range hint
169-169: Improved error message formatting.The addition of
goHTTP.StatusText(resp.StatusCode())in the error message is a good improvement. It provides more context about the HTTP status, which is helpful for debugging and logging.Consider using
fmt.Errorfwith%wverb to wrap the error, allowing better error handling up the call stack:return nil, fmt.Errorf("invalid response code: %d (%s): %w", resp.StatusCode(), goHTTP.StatusText(resp.StatusCode()), err)This assumes there's an underlying
errto wrap. If not, the current implementation is fine.services/omnirpc/proxy/standardize.go (3)
7-7: Remove unnecessary blank import lineThe blank import line added here is unnecessary and doesn't follow the typical Go import grouping convention. Consider removing this line to maintain a clean import section.
38-38: Improve explanation for ignored lint warningWhile the linting directive has been made more specific, which is good, the added comment "it's okay." doesn't provide a clear rationale for ignoring the unused code warning. Consider adding a more descriptive explanation, such as why this field is necessary despite being unused.
Example:
//lint:ignore U1000 This field is required for JSON unmarshalling but not used directly in the code.
Line range hint
153-231: Approve changes with a minor suggestion for error handlingThe changes to the
standardizeResponsefunction are well-implemented. The introduction of error groups for concurrent unmarshalling and the additional consistency checks for block-related methods significantly improve the robustness and efficiency of the function.However, there's a minor improvement that could be made in error handling:
Consider wrapping the error returned by
groupCtx.Wait()to provide more context. For example:if err := groupCtx.Wait(); err != nil { return nil, fmt.Errorf("error during concurrent unmarshalling: %w", err) }This change would make debugging easier by providing more context about where the error occurred.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- services/omnirpc/http/capture_client.go (2 hunks)
- services/omnirpc/http/client.go (2 hunks)
- services/omnirpc/http/fasthttp.go (5 hunks)
- services/omnirpc/http/resty.go (3 hunks)
- services/omnirpc/proxy/forward.go (3 hunks)
- services/omnirpc/proxy/forwarder.go (3 hunks)
- services/omnirpc/proxy/standardize.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (11)
services/omnirpc/http/client.go (1)
7-7: LGTM: New import for metrics package.The addition of the
metricspackage import is appropriate for the newWithMetricsmethod in theRequestinterface.services/omnirpc/proxy/forward.go (1)
133-133: Improved tracing and metrics handling.The changes to use
f.handler.Tracer()for starting the tracer and the addition ofWithMetrics(f.handler)improve the centralization of tracing and metrics handling. This is a good practice for maintaining consistency across the application.To ensure the
handleris correctly implemented and used consistently, please run the following verification script:Also applies to: 162-162
services/omnirpc/http/capture_client.go (6)
63-64: Addition of metrics handler toCapturedRequeststructThe addition of the
Handler metrics.Handlerfield to theCapturedRequeststruct allows for better integration of metrics and tracing within the request handling process.
85-92: Potential nil dereference ofHandlerandContextinSetContextThe same issue regarding potential nil dereference of
c.Handlerandc.Contextapplies here as in theSetBodymethod.
99-107: Potential nil dereference ofHandlerandContextinSetHeaderThe same issue regarding potential nil dereference of
c.Handlerandc.Contextapplies here as in previous methods.
114-122: Potential nil dereference ofHandlerandContextinSetHeaderBytesThe same issue regarding potential nil dereference of
c.Handlerandc.Contextapplies here as in previous methods.
129-136: Potential nil dereference ofHandlerandContextinSetRequestURIThe same issue regarding potential nil dereference of
c.Handlerandc.Contextapplies here as in previous methods.
161-164: Addition ofWithMetricsmethod for setting metrics handlerThe
WithMetricsmethod allows theCapturedRequestto set theHandlerfor metrics and tracing. This enhances the observability of the request lifecycle.Ensure that
WithMetricsis called before any method that relies onc.Handler, to prevent nil pointer dereferences.services/omnirpc/proxy/forwarder.go (3)
82-84: Confirm initialization of the newhandlerfieldThe
handlerfield is now being initialized in theAcquireForwardermethod usingr.handler. Verify thatr.handleris properly set and that thehandleris correctly utilized within theForwardermethods for metrics handling.
12-14: Imports add necessary dependenciesThe new imports for
github.com/synapsecns/sanguine/ethergo/parser/rpc,go.opentelemetry.io/otel/attribute,go.opentelemetry.io/otel/trace, andgithub.com/synapsecns/sanguine/core/metricsare appropriate for the added functionality and appear to be necessary.Also applies to: 20-20
55-56: Verify that all references tof.tracerhave been updatedThe
tracerfield has been removed from theForwarderstruct and replaced with thehandlerfield. Ensure that all methods and functions that previously referencedf.tracerhave been updated appropriately, possibly to user.traceror another relevant reference.Run the following script to check for any remaining references to
f.tracerin the codebase:✅ Verification successful
All references to
f.tracerhave been successfully updated.
- No remaining instances of
f.tracerfound in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search the codebase for any references to 'f.tracer'. ast-grep --lang go --pattern $'f.tracer'Length of output: 39
Script:
#!/bin/bash # Description: Use ripgrep to search for any references to 'f.tracer' in the codebase with line numbers. rg -n 'f\.tracer' .Length of output: 21
Script:
#!/bin/bash # Description: Use ripgrep to search for any references to 'tracer' in the codebase with line numbers. rg -n 'tracer' .Length of output: 3485
| @@ -19,6 +25,7 @@ func NewRestyClient() Client { | |||
| type restyRequest struct { | |||
| *resty.Request | |||
| endpoint string | |||
| handler metrics.Handler | |||
There was a problem hiding this comment.
Potential nil pointer dereference of r.handler in tracing methods
The handler field in restyRequest may be nil if WithMetrics is not called before using methods like SetHeaderBytes, SetBody, SetContext, SetHeader, SetRequestURI, and Do. This could lead to a nil pointer dereference when calling r.handler.Tracer().Start().
To prevent this, ensure that handler is initialized before use. You can initialize handler with a default metrics handler when creating a new request.
Example fix by initializing handler in NewRequest:
func (r RestyClient) NewRequest() Request {
return &restyRequest{
Request: r.client.R(),
+ handler: metrics.NoOpHandler(),
}
}Alternatively, add a nil check in each method before starting the trace:
func (r *restyRequest) SetHeaderBytes(key, value []byte) Request {
+ if r.handler == nil {
+ return r
+ }
_, span := r.handler.Tracer().Start(
r.Request.Context(),
"SetHeaderBytes",
trace.WithAttributes(
attribute.String("key", common.Bytes2Hex(key)),
attribute.String("value", common.Bytes2Hex(value)),
))
defer func() {
metrics.EndSpan(span)
}()
r.Request.SetHeader(string(key), string(value))
return r
}Also applies to: 40-49, 55-62, 68-75, 81-90, 96-103, 108-115
services/omnirpc/http/resty.go
Outdated
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "SetContext", | ||
| ) | ||
| span.AddEvent("SetContext") | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Incorrect context used in SetContext method for tracing
In the SetContext method, when starting the tracing span, you're using the old context r.Request.Context() instead of the new context ctx that is being set. This may lead to incorrect tracing data.
To fix this, use the new context ctx when starting the tracing span:
func (r *restyRequest) SetContext(ctx context.Context) Request {
- _, span := r.handler.Tracer().Start(
- r.Request.Context(),
+ _, span := r.handler.Tracer().Start(
+ ctx,
"SetContext",
)
span.AddEvent("SetContext")
defer func() {
metrics.EndSpan(span)
}()
r.Request.SetContext(ctx)
return r
}Committable suggestion was skipped due to low confidence.
| _, span := c.Handler.Tracer().Start( | ||
| c.Context, | ||
| "SetBody", | ||
| trace.WithAttributes(attribute.String("SetBody", common.Bytes2Hex(body))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Potential nil dereference of Handler and Context in SetBody
In the SetBody method, c.Handler and c.Context are used without checking if they are initialized. If either c.Handler or c.Context is nil, this could lead to a panic at runtime.
Consider initializing c.Handler and c.Context with default values when creating a CapturedRequest, or add checks to ensure they are not nil before use.
| _, span := c.Handler.Tracer().Start( | ||
| c.Context, | ||
| "Do", | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() | ||
|
|
||
| resp, err := c.Client.responseFunc(c) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) | ||
|
|
||
| return resp, err | ||
| } |
There was a problem hiding this comment.
Improve error handling and span attributes in Do method
In the Do method, consider the following improvements:
-
Record errors in the span: If
erris notnil, you could record the error in the span usingspan.RecordError(err)before returning. This enhances tracing of errors. -
Optimize span attribute data: Setting the entire response body as a span attribute may not be optimal if the response body is large. Consider summarizing the response or limiting the size of the data added to the span.
You might apply changes like:
if err != nil {
+ span.RecordError(err)
return nil, err
}
-span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body())))
+span.SetAttributes(attribute.String("response_summary", summarizeResponse(resp.Body())))You'll need to implement summarizeResponse to create a summary or limit the size of the response data.
Committable suggestion was skipped due to low confidence.
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetBody", | ||
| trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Potential nil pointer dereference due to uninitialized metrics handler
In the methods SetBody, SetContext, SetHeader, SetHeaderBytes, SetRequestURI, and Do, the code assumes that f.handler is non-nil when calling f.handler.Tracer().Start(). However, f.handler is initialized to nil in AcquireRequest() and may not be set before these methods are called, leading to a possible nil pointer dereference at runtime.
Consider initializing f.handler with a default metrics handler when acquiring the request or adding checks to ensure f.handler is not nil before using it.
Apply this diff to initialize the handler with a default metrics handler:
func (f *fastHTTPClient) AcquireRequest() *fastHTTPRequest {
v := f.reqPool.Get()
if v == nil {
return &fastHTTPRequest{
&fasthttp.Request{},
f,
- nil,
+ context.Background(),
+ metrics.DefaultHandler(), // Initialize with a default metrics handler
}
}
// Rest of the code
}Alternatively, add a nil check in each method before using f.handler:
func (f *fastHTTPRequest) SetBody(body []byte) Request {
+ if f.handler == nil {
+ // Handle the nil handler case, possibly initialize or skip tracing
+ return f
+ }
_, span := f.handler.Tracer().Start(
f.context,
"SetBody",
trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))),
)
defer func() {
metrics.EndSpan(span)
}()
f.Request.SetBodyRaw(body)
return f
}Also applies to: 191-198, 204-212, 218-226, 232-239, 245-251
services/omnirpc/http/fasthttp.go
Outdated
| trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))), | ||
| ) |
There was a problem hiding this comment.
Potential PII exposure by including request body in tracing attributes
The SetBody method includes the request body in tracing attributes:
trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))),If the request body contains sensitive information, converting it to hex and adding it to tracing can expose PII or confidential data.
Consider removing the body from tracing attributes or sanitizing it to prevent sensitive data leakage.
Apply this diff to exclude the request body from tracing attributes:
_, span := f.handler.Tracer().Start(
f.context,
"SetBody",
- trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))),
)
services/omnirpc/http/fasthttp.go
Outdated
| trace.WithAttributes(attribute.String("SetHeader", key)), | ||
| trace.WithAttributes(attribute.String("value", value)), |
There was a problem hiding this comment.
Potential PII exposure by including header values in tracing attributes
In the SetHeader method, header keys and values are added to tracing attributes:
trace.WithAttributes(attribute.String("SetHeader", key)),
trace.WithAttributes(attribute.String("value", value)),Headers may contain sensitive information, such as authentication tokens or personal data.
Consider excluding sensitive headers or masking values before adding them to tracing to avoid exposing PII.
Apply this diff to prevent sensitive header values from being included:
_, span := f.handler.Tracer().Start(
f.context,
"SetHeader",
trace.WithAttributes(attribute.String("SetHeader", key)),
- trace.WithAttributes(attribute.String("value", value)),
+ // Optionally include a placeholder or exclude sensitive values
)
services/omnirpc/http/fasthttp.go
Outdated
| trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))), | ||
| trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))), |
There was a problem hiding this comment.
Potential PII exposure by including header byte values in tracing attributes
Similarly, the SetHeaderBytes method includes header keys and values:
trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))),
trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))),Including raw header bytes can expose sensitive data.
Consider removing or sanitizing these attributes to protect sensitive information.
Apply this diff to avoid including raw header bytes:
_, span := f.handler.Tracer().Start(
f.context,
"SetHeaderBytes",
- trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))),
- trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))),
+ // Avoid including raw header bytes to prevent data exposure
)
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetContext", | ||
| ) | ||
| span.AddEvent("SetContext") | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Starting span with outdated context in SetContext method
In the SetContext method, the span is started with f.context, but f.context is updated to ctx afterward:
_, span := f.handler.Tracer().Start(
f.context, // Old context
"SetContext",
)
...
f.context = ctx // New contextThis means the span is associated with the old context, which may not be intended.
Consider starting the span with the new context ctx to ensure accurate tracing.
Apply this diff to use the new context:
func (f *fastHTTPRequest) SetContext(ctx context.Context) Request {
- _, span := f.handler.Tracer().Start(
- f.context,
+ _, span := f.handler.Tracer().Start(
+ ctx,
"SetContext",
)
span.AddEvent("SetContext")
defer func() {
metrics.EndSpan(span)
}()
f.context = ctx
return f
}
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "Do", | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Ensure context cancellation is respected in Do method
While the Do method checks for context cancellation after receiving the response:
select {
case <-f.context.Done():
return nil, fmt.Errorf("could not get context: %w", f.context.Err())
default:
return newRawResponse(realResponse, resp.StatusCode()), nil
}This check occurs after the request has completed, which might not prevent unnecessary work if the context is canceled during the request.
Consider integrating context cancellation support with the underlying HTTP client or implementing a periodic check during long-running requests if possible.
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (10)
services/omnirpc/http/client.go (2)
31-32: Approve newWithMetricsmethod with suggestions.The addition of the
WithMetricsmethod is a good step towards implementing request tracing. However, consider the following suggestions:
- Enhance the comment to provide more context, e.g., "WithMetrics associates a metrics handler with the request for performance tracking and monitoring."
- Consider grouping related methods together in the interface for better readability. For instance, you might want to place this method near
SetContextor other request configuration methods.
Tracing Support Still Needed in
client.goThe TODO comment regarding tracing support in
services/omnirpc/http/client.gois still present and there is no evidence of actual tracing implementation in this file. Please implement the necessary tracing functionality or update/remove the TODO comment if tracing support has been addressed elsewhere.🔗 Analysis chain
Line range hint
18-19: Review and update the TODO comment about tracing support.The TODO comment about supporting tracing seems partially addressed by the new
WithMetricsmethod. However, it's unclear if full tracing support has been implemented. Please review this comment and either:
- Remove it if tracing is now fully supported.
- Update it to clarify what aspects of tracing still need to be implemented.
- If tracing is distinct from metrics in this context, consider adding a separate
WithTracingmethod.To help determine the current state of tracing support, you can run the following command to search for tracing-related code:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tracing-related code in the project rg --type go 'trac(e|ing)' -g '!*test*'Length of output: 52968
services/omnirpc/proxy/forward.go (2)
Line range hint
170-172: LGTM! Consider using HTTP status code constants.The use of
goHTTP.StatusText()is an improvement, providing standardized HTTP status messages. This enhances code readability and maintainability.Consider using HTTP status code constants for the range check to improve readability:
-if resp.StatusCode() < 200 || resp.StatusCode() > 400 { +if resp.StatusCode() < goHTTP.StatusOK || resp.StatusCode() >= goHTTP.StatusBadRequest { return nil, fmt.Errorf("invalid response code: %d (%s)", resp.StatusCode(), goHTTP.StatusText(resp.StatusCode())) }
Missing Implementation of
WithMetricsMethod inforward.goThe
WithMetricsmethod is called inservices/omnirpc/proxy/forward.go, but its implementation is not found in the file. This omission can lead to compilation errors or unintended behavior.Please implement the
WithMetricsmethod inforward.goor ensure that it is correctly referenced from another appropriate location.🔗 Analysis chain
Line range hint
133-162: LGTM! Verify the implementation ofWithMetrics.The changes to the tracer initialization and the addition of metrics tracking are improvements to the observability of the system. The use of
f.handler.Tracer()suggests a more centralized approach to tracing, which is a good practice.To ensure the
WithMetricsmethod is correctly implemented, please run the following script:
To resolve the syntax error and verify the
WithMetricsmethod implementation, please run the following script:
To verify the implementation of the
WithMetricsmethod inservices/omnirpc/proxy/forward.go, please run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of WithMetrics method # Test: Search for the WithMetrics method definition ast-grep --lang go --pattern 'func ($_) WithMetrics($_) $_'Length of output: 938
Script:
#!/bin/bash # Description: Verify the implementation of WithMetrics method # Corrected Test: Search for the WithMetrics method definition ast-grep --lang go --pattern 'func ($_) WithMetrics($_) $_'Length of output: 844
Script:
#!/bin/bash # Description: Check if WithMetrics method is implemented in forward.go ast-grep --lang go --pattern 'func ($_) WithMetrics($_) $_' services/omnirpc/proxy/forward.goLength of output: 95
services/omnirpc/proxy/standardize.go (2)
7-7: Consider removing the blank import line.The added blank import line doesn't seem to serve a clear purpose. If it's intended to separate import groups, consider using comments instead to improve readability.
38-38: Provide a more descriptive reason for ignoring the linter warning.While the linting directive has been updated to be more specific, the comment "it's okay" doesn't provide a clear explanation for why this unused code is acceptable. Consider adding a more descriptive reason, such as explaining why this field is necessary despite being unused, or if it's used in other parts of the codebase not visible in this file.
Example:
//lint:ignore U1000 This field is used for serialization/deserialization purposes in other parts of the codebase.services/omnirpc/http/resty.go (1)
Line range hint
28-125: Potential nil pointer dereference due to uninitializedhandlerfieldThe
handlerfield inrestyRequestis not initialized inNewRequest, and methods likeSetHeaderBytes,SetBody,SetContext,SetHeader,SetRequestURI, andDoassumer.handleris non-nil. Ifhandleris nil, callingr.handler.Tracer()will result in a nil pointer dereference, causing a runtime panic. To prevent this, ensure thathandleris properly initialized before use or add nil checks before dereferencingr.handler.Apply this diff to add nil checks in the methods (example shown for
SetHeaderBytes):func (r *restyRequest) SetHeaderBytes(key, value []byte) Request { + if r.handler != nil { _, span := r.handler.Tracer().Start( r.Request.Context(), "SetHeaderBytes", trace.WithAttributes( attribute.String("key", common.Bytes2Hex(key)), attribute.String("value", common.Bytes2Hex(value)), )) defer func() { metrics.EndSpan(span) }() + } r.Request.SetHeader(string(key), string(value)) return r }Repeat similar nil checks in other methods where
r.handleris used.services/omnirpc/http/capture_client.go (2)
63-64: Update comment to match field name 'Handler'The comment above the
Handlerfield says// Metrics is the metrics handler, but the field is namedHandler. For clarity and consistency, the comment should refer toHandler.Apply this diff:
- // Metrics is the metrics handler + // Handler is the metrics handler
99-107: Combine multiple attributes in 'trace.WithAttributes' callsWhen starting spans in
SetHeader,SetHeaderBytes, andSetRequestURI, you can combine multiple attributes into a singletrace.WithAttributes()call for better readability and efficiency.Apply this diff to the
SetHeadermethod:_, span := c.Handler.Tracer().Start( c.Context, "SetHeader", - trace.WithAttributes(attribute.String("SetHeader", key)), - trace.WithAttributes(attribute.String("value", value)), + trace.WithAttributes( + attribute.String("SetHeader", key), + attribute.String("value", value), + ), )Similarly, update the
SetHeaderBytesandSetRequestURImethods:
- For
SetHeaderBytes:_, span := c.Handler.Tracer().Start( c.Context, "SetHeaderBytes", - trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))), - trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))), + trace.WithAttributes( + attribute.String("key", common.Bytes2Hex(key)), + attribute.String("value", common.Bytes2Hex(value)), + ), )
- For
SetRequestURI:_, span := c.Handler.Tracer().Start( c.Context, "SetRequestURI", - trace.WithAttributes(attribute.String("uri", uri)), + trace.WithAttributes( + attribute.String("uri", uri), + ), )Also applies to: 114-122, 129-136
services/omnirpc/http/fasthttp.go (1)
284-287: Fluent interface inconsistency inWithMetricsThe
WithMetricsmethod returns aRequest, allowing method chaining. However, if the user forgets to callWithMetricsbefore other methods, it may lead to a runtime panic due tof.handlerbeingnil.Suggestion:
Consider returning the concrete type
*fastHTTPRequestinstead of the interfaceRequestto maintain consistency. Additionally, document the necessity of callingWithMetricsbefore other methods, or enforce initialization in the constructor.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- services/omnirpc/http/capture_client.go (2 hunks)
- services/omnirpc/http/client.go (2 hunks)
- services/omnirpc/http/fasthttp.go (5 hunks)
- services/omnirpc/http/resty.go (3 hunks)
- services/omnirpc/proxy/forward.go (3 hunks)
- services/omnirpc/proxy/forwarder.go (3 hunks)
- services/omnirpc/proxy/standardize.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
services/omnirpc/http/client.go (1)
6-7: LGTM: New import for metrics package.The addition of the metrics package import is appropriate for the new
WithMetricsmethod.services/omnirpc/proxy/forward.go (1)
8-10: LGTM! Verify usage of re-imported packages.The addition of
net/http(aliased asgoHTTP) andstringspackages is appropriate. As these were re-imported, please ensure they are utilized effectively in the updated code.To confirm the usage of these packages, run the following script:
#!/bin/bash # Description: Verify the usage of re-imported packages # Test: Search for usage of goHTTP and strings rg --type go 'goHTTP\.' services/omnirpc/proxy/forward.go rg --type go 'strings\.' services/omnirpc/proxy/forward.goservices/omnirpc/proxy/standardize.go (1)
Line range hint
1-241: Verify alignment with PR objectives.The PR objectives mention introducing features related to omnirpc request tracing. However, the changes observed in this file are minor and don't seem to directly address this objective. Could you please clarify how these changes contribute to the stated goal of implementing request tracing for omnirpc?
services/omnirpc/http/resty.go (1)
123-125: TheWithMetricsmethod is well implementedThe
WithMetricsmethod allows assigning ametrics.Handlerto therestyRequest, enabling metrics tracking for the request lifecycle. This enhances the observability of requests.services/omnirpc/proxy/forwarder.go (1)
12-15: Approved addition of necessary importsThe added imports for
rpc,attribute,trace, andmetrics(lines 12-15 and 20) are appropriate and essential for the tracing and metrics functionality introduced in the code.Also applies to: 20-20
services/omnirpc/http/resty.go
Outdated
| func (r *restyRequest) Do() (_ Response, err error) { | ||
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "Do", | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
| resp, err := r.Request.Post(r.endpoint) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get response from %s: %w", r.endpoint, err) | ||
| } | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
Ensure r.endpoint is set before making the request
In the Do method, if r.endpoint is not set, calling r.Request.Post(r.endpoint) might fail or send a request to an unintended URL. It's important to check if r.endpoint is not empty before making the request and handle the error appropriately.
Apply this diff to add a check for r.endpoint:
func (r *restyRequest) Do() (_ Response, err error) {
_, span := r.handler.Tracer().Start(
r.Request.Context(),
"Do",
)
defer func() {
metrics.EndSpanWithErr(span, err)
}()
+ if r.endpoint == "" {
+ return nil, fmt.Errorf("endpoint is not set")
+ }
resp, err := r.Request.Post(r.endpoint)
if err != nil {
return nil, fmt.Errorf("could not get response from %s: %w", r.endpoint, err)
}
return resp, 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 (r *restyRequest) Do() (_ Response, err error) { | |
| _, span := r.handler.Tracer().Start( | |
| r.Request.Context(), | |
| "Do", | |
| ) | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| resp, err := r.Request.Post(r.endpoint) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get response from %s: %w", r.endpoint, err) | |
| } | |
| return resp, nil | |
| } | |
| func (r *restyRequest) Do() (_ Response, err error) { | |
| _, span := r.handler.Tracer().Start( | |
| r.Request.Context(), | |
| "Do", | |
| ) | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| if r.endpoint == "" { | |
| return nil, fmt.Errorf("endpoint is not set") | |
| } | |
| resp, err := r.Request.Post(r.endpoint) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get response from %s: %w", r.endpoint, err) | |
| } | |
| return resp, nil | |
| } |
| _, span := c.Handler.Tracer().Start( | ||
| c.Context, | ||
| "SetBody", | ||
| trace.WithAttributes(attribute.String("SetBody", common.Bytes2Hex(body))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Ensure 'Handler' is not nil before usage to prevent nil pointer dereference
In the methods SetBody, SetContext, SetHeader, SetHeaderBytes, SetRequestURI, and Do, the code calls c.Handler.Tracer() without checking if Handler is initialized. If Handler is nil, this will cause a nil pointer dereference at runtime.
Consider initializing Handler in NewRequest() or adding nil checks before using it. For example, initialize Handler with a no-op metrics handler:
func (c *CaptureClient) NewRequest() Request {
request := CapturedRequest{
Client: c,
StringHeaders: make(map[string]string),
+ Handler: metrics.NoopHandler(),
}
c.requests = append(c.requests, &request)
return &request
}Alternatively, add nil checks before using Handler:
if c.Handler != nil {
// Proceed to start the span
} else {
// Handle the case where Handler is nil, possibly by initializing it or skipping tracing
}Also applies to: 85-92, 99-107, 114-122, 129-136, 143-149
| _, span := c.Handler.Tracer().Start( | ||
| c.Context, | ||
| "SetContext", | ||
| ) | ||
| span.AddEvent("SetContext") | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Use provided context in 'SetContext' method
In the SetContext method, when starting a new span, you're currently using c.Context before it's set. Since SetContext is intended to update c.Context, you should use the provided ctx parameter when starting the span.
Apply this diff to use the correct context:
func (c *CapturedRequest) SetContext(ctx context.Context) Request {
- _, span := c.Handler.Tracer().Start(
- c.Context,
+ _, span := c.Handler.Tracer().Start(
+ ctx,
"SetContext",
)
span.AddEvent("SetContext")
defer func() {
metrics.EndSpan(span)
}()
c.Context = ctx
return c
}📝 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.
| _, span := c.Handler.Tracer().Start( | |
| c.Context, | |
| "SetContext", | |
| ) | |
| span.AddEvent("SetContext") | |
| defer func() { | |
| metrics.EndSpan(span) | |
| }() | |
| _, span := c.Handler.Tracer().Start( | |
| ctx, | |
| "SetContext", | |
| ) | |
| span.AddEvent("SetContext") | |
| defer func() { | |
| metrics.EndSpan(span) | |
| }() |
| span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) | ||
|
|
There was a problem hiding this comment.
Handle potential nil response body in 'Do' method
In the Do method, after calling responseFunc, you set an attribute with resp.Body(). If resp is nil or resp.Body() returns nil, this could cause a panic. Consider checking if resp and resp.Body() are not nil before setting the attribute.
Apply this diff:
- span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body())))
+ if resp != nil && resp.Body() != nil {
+ span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body())))
+ }📝 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.
| span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) | |
| if resp != nil && resp.Body() != nil { | |
| span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) | |
| } |
| // handler is the metrics handler | ||
| handler metrics.Handler |
There was a problem hiding this comment.
Unused handler field in Forwarder struct
The handler field is added to the Forwarder struct (line 56) and initialized in AcquireForwarder (lines 82-84), but it is not used anywhere within the Forwarder methods. Introducing unused fields can lead to confusion and unnecessary code complexity.
Consider either removing the handler field if it's unnecessary or implementing its usage within the Forwarder methods to handle metrics appropriately.
Also applies to: 82-84
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetBody", | ||
| trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Ensure handler is non-nil before starting tracing spans
In methods SetBody, SetContext, SetHeader, SetHeaderBytes, SetRequestURI, and Do, you're starting tracing spans using f.handler.Tracer(), but there's no check to ensure that f.handler is not nil. If handler is nil, this will result in a runtime panic.
Suggestion:
Add a check to ensure f.handler is not nil before starting spans. For example:
if f.handler != nil {
ctx, span := f.handler.Tracer().Start(...)
// ... rest of the code
}Alternatively, enforce that WithMetrics must be called before these methods, and document this requirement clearly.
Also applies to: 191-198, 204-212, 218-226, 232-239, 245-251
Avoid logging entire request bodies in tracing attributes
In SetBody, you're adding the entire request body to the tracing attributes:
trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))),This may lead to performance issues and potential exposure of sensitive data if the body is large or contains confidential information.
Suggestion:
Consider limiting the amount of data recorded in the attributes or avoid logging the body altogether. If necessary, log a hashed or truncated version of the body to prevent sensitive data exposure.
trace.WithAttributes(attribute.String("body_hash", hashFunc(body))),Replace hashFunc with an appropriate hashing function.
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetHeader", | ||
| trace.WithAttributes(attribute.String("SetHeader", key)), | ||
| trace.WithAttributes(attribute.String("value", value)), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Avoid logging sensitive header information in tracing attributes
In SetHeader and SetHeaderBytes, you're adding header keys and values to the tracing attributes:
trace.WithAttributes(attribute.String("SetHeader", key)),
trace.WithAttributes(attribute.String("value", value)),Headers may contain sensitive information like authentication tokens, which should not be logged.
Suggestion:
Exclude sensitive headers from tracing, or avoid logging header values altogether. If you need to log headers, consider logging only non-sensitive keys.
trace.WithAttributes(attribute.String("header_key", key)),Be cautious about which headers are included to comply with security best practices.
Also applies to: 218-226
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "SetContext", | ||
| ) | ||
| span.AddEvent("SetContext") | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Incorrect context usage in SetContext
In the SetContext method, you're starting a span with the old f.context before assigning the new context ctx:
_, span := f.handler.Tracer().Start(
f.context,
"SetContext",
)
...
f.context = ctxThis means the span is started with the previous context rather than the new one, which may not be the intended behavior.
Suggestion:
Start the span using the new context ctx:
_, span := f.handler.Tracer().Start(
ctx,
"SetContext",
)
...
f.context = ctxThis ensures that the tracing is correctly associated with the updated context.
services/omnirpc/http/fasthttp.go
Outdated
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "Do", | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure context cancellation is respected in Do method
In the Do method, after making the request, you check for context cancellation:
select {
case <-f.context.Done():
return nil, fmt.Errorf("could not get context: %w", f.context.Err())
default:
return newRawResponse(realResponse, resp.StatusCode()), nil
}However, if the context is canceled during the request, the request will still complete, potentially wasting resources.
Suggestion:
Consider integrating context cancellation with the request execution. Since fasthttp doesn't support context cancellation natively, you might implement a workaround using a custom Dial function or manage timeouts carefully to respect context cancellations.
🛠️ Refactor suggestion
Hardcoded timeout in Do method
In the Do method, you're using a hardcoded timeout value for the request:
err := hostClient.DoTimeout(f.Request, resp, time.Second*30)This may not be flexible for different use cases where varying timeouts are required.
Suggestion:
Allow the timeout duration to be configurable, either through a method parameter or a configurable field within the client or request. This provides flexibility for different scenarios.
For example:
func (f *fastHTTPRequest) Do(timeout time.Duration) (Response, error) {
// ...
err := hostClient.DoTimeout(f.Request, resp, timeout)
// ...
}
services/omnirpc/http/fasthttp.go
Outdated
| func (f *fastHTTPRequest) WithMetrics(metrics metrics.Handler) Request { | ||
| f.handler = metrics | ||
| return f | ||
| } |
There was a problem hiding this comment.
Method WithMetrics should handle nil input
In WithMetrics, you assign the provided metrics.Handler to f.handler without checking if it's nil:
func (f *fastHTTPRequest) WithMetrics(metrics metrics.Handler) Request {
f.handler = metrics
return f
}If nil is passed inadvertently, subsequent method calls may result in a nil pointer dereference.
Suggestion:
Add a check or validation to ensure that metrics is not nil. Alternatively, document that metrics must not be nil when calling this method.
if metrics == nil {
// Handle the nil case, possibly return an error or use a default handler
}
golangisfun123
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
services/omnirpc/http/capture_client.go (3)
99-107: LGTM with suggestions: Added tracing to SetHeader method.The tracing functionality has been correctly implemented in the
SetHeadermethod. However, please consider the following points:
- Combine multiple
WithAttributescalls into a single call for better efficiency.- Ensure that
HandlerandContextare not nil before use to prevent potential nil pointer dereferences.Consider refactoring the
WithAttributescalls as follows:_, span := c.Handler.Tracer().Start( c.Context, "SetHeader", - trace.WithAttributes(attribute.String("SetHeader", key)), - trace.WithAttributes(attribute.String("value", value)), + trace.WithAttributes( + attribute.String("SetHeader", key), + attribute.String("value", value), + ), )
114-122: LGTM with suggestions: Added tracing to SetHeaderBytes method.The tracing functionality has been correctly implemented in the
SetHeaderBytesmethod. However, please consider the following points:
- Combine multiple
WithAttributescalls into a single call for better efficiency, similar to the suggestion for theSetHeadermethod.- Ensure that
HandlerandContextare not nil before use to prevent potential nil pointer dereferences.Consider refactoring the
WithAttributescalls as follows:_, span := c.Handler.Tracer().Start( c.Context, "SetHeaderBytes", - trace.WithAttributes(attribute.String("key", common.Bytes2Hex(key))), - trace.WithAttributes(attribute.String("value", common.Bytes2Hex(value))), + trace.WithAttributes( + attribute.String("key", common.Bytes2Hex(key)), + attribute.String("value", common.Bytes2Hex(value)), + ), )
143-159: LGTM with suggestions: Added tracing to Do method.The tracing functionality has been correctly implemented in the
Domethod. However, please consider the following improvements:
- Ensure that
HandlerandContextare not nil before use to prevent potential nil pointer dereferences.- Check if
respandresp.Body()are not nil before setting the response attribute.- Consider recording the error in the span if an error occurs.
Consider refactoring the error handling and attribute setting as follows:
resp, err := c.Client.responseFunc(c) if err != nil { + span.RecordError(err) return nil, err } -span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) +if resp != nil && resp.Body() != nil { + span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/omnirpc/http/capture_client.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
services/omnirpc/http/capture_client.go (6)
6-10: LGTM: New imports for metrics and tracing.The added imports are appropriate for the new tracing and metrics functionality being introduced.
63-64: LGTM: Added metrics Handler to CapturedRequest struct.The addition of the
Handler metrics.Handlerfield is appropriate for integrating metrics functionality into theCapturedRequeststruct.
71-78: LGTM: Added tracing to SetBody method.The tracing functionality has been correctly implemented in the
SetBodymethod. However, please note the potential issue with nilHandlerorContextas mentioned in the previous review comments.
161-164: LGTM: Added WithMetrics method.The
WithMetricsmethod has been correctly implemented to set the metrics handler for theCapturedRequest. This addition complements the new tracing functionality and allows for easy integration of metrics handling.
Line range hint
1-164: Overall LGTM with suggestions for improvementThe changes introduce valuable tracing and metrics functionality to the
CapturedRequeststructure and its methods. The implementation is generally correct and enhances the observability of the system. However, there are a few recurring issues that should be addressed:
- Ensure proper nil checks for
HandlerandContextbefore use in all methods to prevent potential nil pointer dereferences.- Combine multiple
WithAttributescalls into single calls for better efficiency in methods likeSetHeaderandSetHeaderBytes.- Improve error handling in the
Domethod by recording errors in the span and checking for nil responses before setting attributes.Addressing these points will further improve the robustness and efficiency of the implementation.
To verify the nil checks implementation, you can run the following script:
This script will help identify methods where nil checks for
HandlerandContexthave been implemented.
85-92: LGTM: Added tracing to SetContext method.The tracing functionality has been correctly implemented in the
SetContextmethod, and the span is now started with the new context as suggested. However, please ensure thatHandleris not nil before using it to prevent potential nil pointer dereferences.To verify the
Handlerinitialization, you can run the following script:
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (12)
services/omnirpc/http/capture_client_test.go (1)
Line range hint
14-57: Consider enhancing test coverage for metrics functionality.While the test has been updated to include the new metrics handler, it doesn't explicitly verify the interaction with this handler. To improve test coverage, consider adding assertions or a separate test case that checks if the metrics handler is being used correctly by the
CaptureClient.Would you like assistance in drafting an additional test case or assertions for the metrics functionality?
services/omnirpc/http/resty.go (2)
29-36: LGTM with suggestion: restyRequest struct and NewRequest method updatedThe addition of the
handler metrics.Handlerfield to the restyRequest struct and its initialization in the NewRequest method are appropriate. However, consider adding a nil check for the handler field in methods that use it to prevent potential nil pointer dereferences.Example:
if r.handler != nil { // Use r.handler }
110-122: LGTM with suggestion: Do method updated with tracing and improved error handlingThe Do method has been correctly updated with tracing functionality and improved error handling. However, consider adding a check for an empty endpoint before making the request to prevent potential issues.
Suggestion:
if r.endpoint == "" { return nil, fmt.Errorf("endpoint is not set") }services/omnirpc/proxy/server.go (1)
51-52: LGTM: Improved client initialization with metrics handlerThe modification to include the
handlerin the HTTP client initialization centralizes tracing and metrics handling, which is a good practice. This change aligns well with the PR's objective of implementing request tracing.Consider adding a brief comment explaining the role of the
handlerin the client initialization, for improved code readability:return &RPCProxy{ chainManager: chainmanager.NewChainManagerFromConfig(config, handler), refreshInterval: time.Second * time.Duration(config.RefreshInterval), port: config.Port, + // Initialize client with metrics handler for request tracing client: omniHTTP.NewClient(handler, omniHTTP.ClientTypeFromString(config.ClientType)), handler: handler, }services/omnirpc/http/capture_client.go (3)
73-80: LGTM with suggestion: Added tracing to SetBody method.The tracing functionality added to the
SetBodymethod is well-implemented. However, consider adding a nil check forc.Contextto prevent potential panics if the context hasn't been set:ctx := context.Background() if c.Context != nil { ctx = c.Context } _, span := c.Handler.Tracer().Start( ctx, "SetBody", trace.WithAttributes(attribute.String("SetBody", common.Bytes2Hex(body))), )This change ensures that a valid context is always used, even if
c.Contexthasn't been initialized.
101-138: LGTM with suggestions: Added tracing to SetHeader, SetHeaderBytes, and SetRequestURI methods.The tracing functionality added to these methods is generally well-implemented. However, there are two suggestions for improvement:
Add a nil check for
c.Contextin all three methods, similar to the suggestion forSetBody.In
SetHeaderandSetHeaderBytes, combine multipleWithAttributescalls into a single call:_, span := c.Handler.Tracer().Start( c.Context, "SetHeader", trace.WithAttributes( attribute.String("SetHeader", key), attribute.String("value", value), ), )This ensures that all attributes are correctly added to the span.
145-160: LGTM with suggestions: Added tracing to Do method.The tracing functionality added to the
Domethod is well-implemented. However, there are two suggestions for improvement:
Add a nil check for
c.Context, similar to the suggestion for other methods.Add a nil check before setting the response attribute:
if resp != nil && resp.Body() != nil { span.SetAttributes(attribute.String("response", common.Bytes2Hex(resp.Body()))) }This prevents potential panics if the response or its body is nil.
Additionally, consider recording any errors in the span before returning:
if err != nil { span.RecordError(err) return nil, err }This provides more comprehensive error tracing.
services/omnirpc/modules/harmonyproxy/harmonyproxy.go (5)
Line range hint
95-100: Avoid exposing internal error details to clientsReturning detailed error messages to clients can reveal internal implementation details and sensitive information. Consider sending a generic error message to the client while logging the detailed error for debugging purposes.
Apply this diff to address the issue:
if err != nil { _ = c.Error(err) - c.JSON(http.StatusBadGateway, gin.H{ - "error": err.Error(), - }) + c.JSON(http.StatusBadGateway, gin.H{ + "error": "An internal server error occurred", + }) }Ensure that the detailed error
erris logged appropriately on the server side.
Line range hint
103-104: Avoid logging full request body in tracing attributesSetting the entire request body as a span attribute can lead to performance overhead and potential exposure of sensitive information. Consider logging only essential metadata.
Apply this diff to address the issue:
- span.SetAttributes(attribute.String("body", string(rawBody))) + span.SetAttributes(attribute.Int("request.size", len(rawBody)))- span.SetAttributes(attribute.String("original-body", string(rawBody))) + // Omit or sanitize the request body before loggingAlso applies to: 131-131
Line range hint
202-203: Limit logging of request and response bodies in tracing eventsIncluding full request and response bodies in tracing events can expose sensitive data and affect performance. Log summaries or sizes instead.
Apply this diff to address the issue:
- span.AddEvent("http.request", trace.WithAttributes(attribute.String("body", string(body)))) + span.AddEvent("http.request", trace.WithAttributes(attribute.Int("request.size", len(body))))- span.AddEvent("http.response", trace.WithAttributes(attribute.String("body", string(respBody)))) + span.AddEvent("http.response", trace.WithAttributes(attribute.Int("response.size", len(respBody))))Also applies to: 218-219
Line range hint
246-318: RefactorgetHarmonyReceiptVerifyto reduce complexityThe function
getHarmonyReceiptVerifyhas high cyclomatic complexity, which can make maintenance and testing more difficult. Consider breaking the function into smaller helper functions to improve readability and maintainability.
Line range hint
326-414: RefactorgetLogsHarmonyVerifyto reduce complexitySimilar to
getHarmonyReceiptVerify, thegetLogsHarmonyVerifyfunction is complex. Refactoring it into smaller, focused functions will enhance code clarity and ease future modifications.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
- services/omnirpc/http/capture_client.go (3 hunks)
- services/omnirpc/http/capture_client_test.go (2 hunks)
- services/omnirpc/http/client.go (2 hunks)
- services/omnirpc/http/client_test.go (2 hunks)
- services/omnirpc/http/fasthttp.go (6 hunks)
- services/omnirpc/http/resty.go (1 hunks)
- services/omnirpc/http/suite_test.go (2 hunks)
- services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (2 hunks)
- services/omnirpc/modules/harmonyproxy/harmonyproxy.go (1 hunks)
- services/omnirpc/modules/receiptsbackup/receiptsbackup.go (2 hunks)
- services/omnirpc/proxy/forward.go (2 hunks)
- services/omnirpc/proxy/forward_test.go (3 hunks)
- services/omnirpc/proxy/forwarder.go (4 hunks)
- services/omnirpc/proxy/server.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/omnirpc/http/client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/omnirpc/proxy/forward.go
🧰 Additional context used
🔇 Additional comments (34)
services/omnirpc/http/capture_client_test.go (2)
9-9: LGTM: New import added correctly.The addition of the metrics package import is necessary for the changes in the test and follows proper Go import conventions.
18-25: LGTM:NewCaptureClientupdated correctly with metrics handler.The
NewCaptureClientfunction call has been properly updated to include the newmetrics.NewNullHandler()parameter. The multi-line formatting also improves readability. These changes align with the updated method signature and maintain the existing test logic.services/omnirpc/http/resty.go (4)
5-11: LGTM: Import statements are correctly updatedThe new import statements are appropriate for the added functionality of metrics and tracing.
16-17: LGTM: RestyClient struct updated correctlyThe addition of the
handler metrics.Handlerfield to the RestyClient struct is appropriate for implementing metrics functionality.
22-23: LGTM: NewRestyClient function updated correctlyThe NewRestyClient function has been properly updated to accept a metrics.Handler parameter and initialize the handler field of the RestyClient struct.
70-77: LGTM: SetContext method implemented correctlyThe SetContext method correctly uses the new context for starting the span and implements tracing functionality appropriately.
services/omnirpc/proxy/server.go (1)
6-10: LGTM: New imports addedThe addition of "net/http", "strconv", and "sync" imports suggests enhancements in HTTP handling, string conversions, and concurrency management respectively. These changes align well with the PR's objective of implementing request tracing and metrics handling.
services/omnirpc/http/capture_client.go (6)
6-10: LGTM: New imports for tracing and metrics.The added imports for Ethereum common, metrics, and OpenTelemetry are appropriate for the new tracing and metrics functionality introduced in this file.
17-17: LGTM: Added metrics handler to CaptureClient.The addition of the
handler metrics.Handlerfield to theCaptureClientstruct is appropriate for supporting the new metrics functionality.
24-25: LGTM: Updated NewCaptureClient to include metrics handler.The
NewCaptureClientfunction has been correctly updated to accept ametrics.Handlerparameter and initialize thehandlerfield in the returnedCaptureClient. This change is consistent with the new metrics functionality.
38-38: LGTM: Initialized Handler in NewRequest.The
Handlerfield ofCapturedRequestis now correctly initialized withc.handlerin theNewRequestmethod. This ensures that each new request has access to the metrics handler.
65-66: LGTM: Added metrics handler to CapturedRequest.The addition of the
Handler metrics.Handlerfield to theCapturedRequeststruct is appropriate for supporting the new metrics functionality at the request level.
87-94: LGTM: Added tracing to SetContext method.The tracing functionality added to the
SetContextmethod is well-implemented. The span is correctly created using the new context, an event is added, and the span is properly ended using a deferred function.services/omnirpc/modules/receiptsbackup/receiptsbackup.go (3)
12-13: LGTM: Import statement reorganization.The relocation of the
mixinspackage import improves code organization without affecting functionality.
Line range hint
1-214: Summary: Minimal changes with positive impact on metrics handling.The changes in this file are minimal and focused:
- Reorganization of an import statement.
- Modification of the HTTP client initialization to include a metrics handler.
These changes improve the code organization and enhance the metrics tracking capabilities without introducing any apparent issues or inconsistencies. The overall structure and logic of the package remain intact.
61-61: Approve change and verify usage: HTTP client initialization with metrics handler.The modification to pass the
handlertoomniHTTP.NewRestyClient()is a good improvement for metrics tracking. This change aligns with the updated function signature mentioned in the summary.To ensure this change doesn't introduce any issues, please verify the usage of
NewRestyClientacross the codebase:✅ Verification successful
Change Verified: All NewRestyClient usages include handler parameter.
All instances of
omniHTTP.NewRestyClient()across the codebase have been updated to include thehandlerparameter, ensuring consistent metrics tracking.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other usages of NewRestyClient that might need updating # Test: Search for NewRestyClient usage rg --type go 'NewRestyClient\(' --context 3Length of output: 2827
services/omnirpc/proxy/forward_test.go (3)
8-12: LGTM: Import statements added correctly.The new import statements for "net/http", "net/http/httptest", "net/url", and "strconv" are correctly added and necessary for the changes made in the test functions.
88-88: LGTM: Client initialization updated correctly.The
SetClientmethod call has been correctly updated to includep.metricsas an argument when creating a new client withomniHTTP.NewClient. This change aligns with the updated method signature.To ensure consistency across the codebase, please run the following script to verify the
NewClientmethod signature:✅ Verification successful
LGTM: Client initialization updated correctly.
TheSetClientmethod call has been correctly updated to includep.metricsas an argument when creating a new client withomniHTTP.NewClient. This change aligns with the updated method signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the signature of NewClient method in omniHTTP package # Test: Search for NewClient method definition rg --type go -A 5 'func NewClient\('Length of output: 4775
148-155: LGTM: CaptureClient initialization updated correctly.The
NewCaptureClientmethod call has been correctly updated to includep.metricsas the first argument. This change aligns with the updated method signature.To ensure consistency across the codebase, please run the following script to verify the
NewCaptureClientmethod signature:✅ Verification successful
Verified: CaptureClient initialization aligns with method signature.
The
NewCaptureClientmethod signature matches the provided arguments, and the initialization in the test has been updated correctly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the signature of NewCaptureClient method in omniHTTP package # Test: Search for NewCaptureClient method definition rg --type go -A 5 'func NewCaptureClient\('Length of output: 612
services/omnirpc/http/suite_test.go (1)
42-55: Proper initialization of metrics handler inSetupSuiteGood job initializing the metrics handler conditionally based on the CI environment in
SetupSuite(). This ensures that metrics are only collected when appropriate during testing, which enhances test performance and avoids unnecessary overhead in CI.services/omnirpc/http/client.go (1)
7-7: Import ofmetricspackage is appropriate.The addition of the
metricspackage import is necessary for the newmetrics.Handlerparameter.services/omnirpc/modules/confirmedtofinalized/finalizedproxy.go (2)
9-13: Necessary imports added.The added imports (
io,math/big,net/http, andtime) are required for the new functionalities and enhancements in the code, such as request handling, time-based operations, and working with big integers.
62-62: Client initialization includes metrics handler.Updating the client initialization to
omniHTTP.NewRestyClient(handler)allows the HTTP client to integrate with the metrics handler. This ensures that metrics and tracing information are properly captured during HTTP requests.services/omnirpc/http/fasthttp.go (8)
172-172: Ensure 'handler' in 'fastHTTPRequest' is used safelyThe
handler metrics.Handlerfield added tofastHTTPRequestis critical for tracing. Verify that all usages off.handlerwithinfastHTTPRequestmethods handle the possibility ofnilvalues to prevent runtime panics.
182-189: Potential nil pointer dereference and sensitive data exposure in 'SetBody'The issues previously identified regarding a potential
nilpointer dereference due to uninitializedf.handler, and possible exposure of sensitive data by including the request body in tracing attributes, still exist.
196-203: Potential nil pointer dereference and context usage in 'SetContext'The previous concerns about
f.handlerbeingniland starting a span with the old context are still applicable in theSetContextmethod.
209-217: Potential nil pointer dereference and sensitive data exposure in 'SetHeader'The earlier issues regarding
f.handlerpossibly beingniland the inclusion of sensitive header information in tracing attributes remain in theSetHeadermethod.
223-231: Potential nil pointer dereference and sensitive data exposure in 'SetHeaderBytes'The concerns about a possible
nilpointer dereference withf.handlerand exposure of sensitive data by logging raw header bytes still exist inSetHeaderBytes.
237-244: Potential nil pointer dereference in 'SetRequestURI'The issue of
f.handlerpotentially beingnil, leading to anilpointer dereference when starting a span, remains in theSetRequestURImethod.
250-256: Potential nil pointer dereference and context cancellation in 'Do' methodThe
Domethod may still suffer from anilpointer dereference iff.handlerisnil. Additionally, previous suggestions about respecting context cancellations during the request are still applicable.
51-64: Verify that 'handler' passed to 'NewFastHTTPClient' is not nilThe
NewFastHTTPClientfunction now accepts ametrics.Handlerparameter and assigns it to thehandlerfield. Ensure that all calls toNewFastHTTPClientprovide a non-nilhandlerto prevent runtimenilpointer dereferences.Run the following script to find all invocations of
NewFastHTTPClientand check ifhandleris properly passed:services/omnirpc/proxy/forwarder.go (2)
12-15: Approved: Addition of necessary importsThe added imports for
rpc,attribute, andtraceare appropriate for the new tracing functionality and are correctly included.
20-20: Approved: Import of metrics packageThe import of
github.com/synapsecns/sanguine/core/metricsis necessary for handling metrics and is properly added.services/omnirpc/modules/harmonyproxy/harmonyproxy.go (1)
57-57: Initialization with handler looks goodThe HTTP client is now correctly initialized with the metrics handler, ensuring that metrics are properly captured.
services/omnirpc/http/resty.go
Outdated
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "SetHeaderBytes", | ||
| trace.WithAttributes( | ||
| attribute.String("key", common.Bytes2Hex(key)), | ||
| attribute.String("value", common.Bytes2Hex(value)), | ||
| )) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Security concern: Potential exposure of sensitive information in trace attributes
While the tracing functionality is implemented correctly, logging header values as trace attributes might expose sensitive information. Consider removing or sanitizing potentially sensitive data before adding it to trace attributes.
Suggestion:
attribute.String("key", string(key)),
attribute.String("value", "[REDACTED]"),
services/omnirpc/http/resty.go
Outdated
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "SetBody", | ||
| trace.WithAttributes(attribute.String("body", common.Bytes2Hex(body))), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Security concern: Potential exposure of sensitive information in trace attributes
While the tracing functionality is implemented correctly, logging the entire request body as a trace attribute might expose sensitive information. Consider removing the body from trace attributes or implementing a safe way to log only non-sensitive parts of the body.
Suggestion:
// Remove or sanitize body logging
trace.WithAttributes(attribute.String("body_length", fmt.Sprintf("%d", len(body)))),
services/omnirpc/http/resty.go
Outdated
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "SetHeader", | ||
| trace.WithAttributes( | ||
| attribute.String("SetHeader", key), | ||
| attribute.String("value", value), | ||
| )) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Security concern: Potential exposure of sensitive information in SetHeader method
The SetHeader method logs the header key and value as trace attributes, which might expose sensitive information. Consider removing or sanitizing potentially sensitive data before adding it to trace attributes.
Suggestion:
attribute.String("header_key", key),
attribute.String("header_value", "[REDACTED]"),
services/omnirpc/http/resty.go
Outdated
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "SetRequestURI", | ||
| trace.WithAttributes(attribute.String("uri", uri)), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Security concern: Potential exposure of sensitive information in SetRequestURI method
The SetRequestURI method logs the entire URI as a trace attribute, which might contain sensitive information in query parameters. Consider sanitizing the URI before logging it.
Suggestion:
sanitizedURI := sanitizeURI(uri) // Implement sanitizeURI function
attribute.String("uri", sanitizedURI),| c.clients = append(c.clients, http.NewClient(metrics.NewNullHandler(), clientType)) | ||
| } |
There was a problem hiding this comment.
Use the initialized metrics handler when creating clients
In SetupTest(), the clients are being initialized with metrics.NewNullHandler(), while the metrics handler is set up and stored in c.metrics during SetupSuite(). To ensure consistency and that the clients use the appropriate metrics handler based on the test environment, consider using c.metrics when initializing the clients.
Apply this diff to use the metrics handler stored in c.metrics:
39- c.clients = append(c.clients, http.NewClient(metrics.NewNullHandler(), clientType))
39+ c.clients = append(c.clients, http.NewClient(c.metrics, clientType))
40 }📝 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.
| c.clients = append(c.clients, http.NewClient(metrics.NewNullHandler(), clientType)) | |
| } | |
| c.clients = append(c.clients, http.NewClient(c.metrics, clientType)) | |
| } |
| default: | ||
| return NewRestyClient() | ||
| return NewRestyClient(handler) |
There was a problem hiding this comment.
Handle unknown ClientType explicitly to prevent unexpected behavior.
In the default case of the switch statement, the function returns NewRestyClient(handler) when an unknown ClientType is provided. This could lead to unintended behavior if an unrecognized ClientType is used. Consider explicitly handling unknown ClientType values by returning an error or defaulting to a safe, well-documented client type, and possibly logging a warning to alert developers.
| func NewClient(handler metrics.Handler, clientType ClientType) Client { | ||
| switch clientType { | ||
| case FastHTTP: | ||
| return NewFastHTTPClient() | ||
| return NewFastHTTPClient(handler) | ||
| case Resty: | ||
| return NewRestyClient() | ||
| return NewRestyClient(handler) | ||
| default: | ||
| return NewRestyClient() | ||
| return NewRestyClient(handler) |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update NewClient Call Sites to Include the handler Parameter
Several instances of NewClient are missing the new handler parameter. Please update these call sites to include handler metrics.Handler to ensure proper functionality and prevent potential issues.
-
ethergo/client/capture_client.go:ethClient := ethclient.NewClient(c)w3Client := w3.NewClient(c)
-
ethergo/backends/geth/geth.go:rawClient := ethclient.NewClient(rpcClient)w3Client := w3.NewClient(rpcClient)
-
ethergo/chain/client/client.go:c.Client = ethclient.NewClient(c.rpcClient)c.w3Client = w3.NewClient(c.rpcClient)ethClient := ethclient.NewClient(rpcClient)w3Client := w3.NewClient(rpcClient)ethClient := ethclient.NewClient(rpcClient)w3Client := w3.NewClient(rpcClient)
-
core/metrics/otlp.go:return otlptracehttp.NewClient(to.httpOptions...), nilreturn otlptracegrpc.NewClient(to.grpcOptions...), nil
-
contrib/git-changes-action/detector/git/ref.go:client := github.NewClient(tc)
Please ensure that each NewClient invocation includes the required handler parameter.
🔗 Analysis chain
Ensure all call sites of NewClient are updated with the new handler parameter.
The NewClient function signature now includes handler metrics.Handler. Please verify that all calls to NewClient throughout the codebase are updated to pass the new handler argument to prevent compilation errors or unexpected behavior.
Run the following script to find all usages of NewClient:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all call sites of NewClient that need to be updated.
# Search for usage of 'NewClient(' in Go files
rg --type go 'NewClient\('
Length of output: 5031
Script:
#!/bin/bash
# Description: Find all call sites of NewClient that do not include the handler parameter.
# Search for 'NewClient(' in Go files with fewer than expected arguments
rg --type go 'NewClient\s*\(\s*(?!handler\s*,).*?\)'
Length of output: 312
Script:
#!/bin/bash
# Description: Find all call sites of NewClient that include only one argument.
# Search for 'NewClient(' in Go files with only one argument
rg --type go 'NewClient\s*\(\s*[^,()]+\s*\)'
Length of output: 990
| @@ -26,6 +31,7 @@ | |||
| // no longer needed. This allows Request recycling, reduces GC pressure | |||
| // and usually improves performance. | |||
| reqPool sync.Pool | |||
| handler metrics.Handler | |||
There was a problem hiding this comment.
💡 Codebase verification
'handler' Field Not Properly Initialized in 'fastHTTPClient'
The handler field in fastHTTPClient is not being initialized in the NewFastHTTPClient function, which could lead to potential nil pointer dereferences.
- File: services/omnirpc/http/fasthttp.go
🔗 Analysis chain
Ensure 'handler' field is properly initialized in 'fastHTTPClient'
The addition of the handler metrics.Handler field to the fastHTTPClient struct is beneficial for metrics and tracing. However, ensure that handler is properly initialized to prevent potential nil pointer dereferences when it is used.
Run the following script to check where NewFastHTTPClient is called and verify that handler is properly initialized:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to NewFastHTTPClient and check initialization of 'handler'
# Search for NewFastHTTPClient usage
rg --type go 'NewFastHTTPClient\((.*)\)' -A 2 -B 2
Length of output: 875
services/omnirpc/proxy/forwarder.go
Outdated
| @@ -95,7 +97,7 @@ func (r *RPCProxy) ReleaseForwarder(f *Forwarder) { | |||
| // Forward forwards the rpc request to the servers and makes assertions around confirmation thresholds. | |||
| // required confirmations can be used to override the required confirmations count. | |||
| func (r *RPCProxy) Forward(c *gin.Context, chainID uint32, requiredConfirmationsOverride *uint16) { | |||
| ctx, span := r.tracer.Start(c, "rpcRequest", | |||
| ctx, span := r.handler.Tracer().Start(c, "rpcRequest", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using forwarder.handler instead of r.handler for consistency
In the Forward method, you are using r.handler.Tracer().Start(...). Since the handler is already stored in the Forwarder struct (as forwarder.handler), consider using forwarder.handler.Tracer().Start(...) instead. This enhances consistency by utilizing the handler field within the Forwarder.
| } | ||
|
|
||
| var _ Client = &CaptureClient{} | ||
|
|
||
| // SetBody stores the body for testing. | ||
| func (c *CapturedRequest) SetBody(body []byte) Request { | ||
| _, span := c.Handler.Tracer().Start( |
There was a problem hiding this comment.
i wouldn't do this here. I'd have a single trace around when Do() si called and set the attributes there (http.body) specifically
| c.Context = ctx | ||
| return c | ||
| } | ||
|
|
||
| // SetHeader sets the header for testing. | ||
| func (c *CapturedRequest) SetHeader(key, value string) Request { | ||
| _, span := c.Handler.Tracer().Start( |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
services/omnirpc/http/capture_client.go (2)
66-67: LGTM with suggestion: Added metrics handler to CapturedRequest.The new
Handlerfield of typemetrics.Handleris a good addition to theCapturedRequeststruct. It allows for metrics handling within individual requests.Consider updating the comment to be more specific:
- // Metrics is the metrics handler + // Handler is the metrics handler for tracing and monitoring request operations
104-127: LGTM with suggestion: Added comprehensive tracing to Do method.The tracing implementation in the
Domethod is well-structured and covers important aspects of the request-response cycle. Good use ofdeferfor ending the span and proper error handling.Consider limiting the size of the response body added to the span attributes to avoid performance issues with large responses:
- attribute.String("response", common.Bytes2Hex(resp.Body())), + attribute.String("response", truncateHexString(common.Bytes2Hex(resp.Body()), maxResponseLength)),You'll need to implement the
truncateHexStringfunction and definemaxResponseLengthappropriately.services/omnirpc/http/fasthttp.go (1)
Line range hint
1-255: Overall improvements in observability, consider performance impactThe changes introduce valuable metrics and tracing functionality to the HTTP client, enhancing its observability. The new
handlerfields and parameters are consistently implemented across the relevant structs and functions.However, there are a few points to consider:
- Ensure proper initialization of the
handlerfield to prevent nil pointer dereferences.- Be cautious about including sensitive data in tracing spans.
- The added tracing might have an impact on performance, especially for high-throughput scenarios.
Consider conducting performance testing to measure the impact of the added tracing on request latency and throughput. This will help in making informed decisions about the trade-off between observability and performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/omnirpc/http/capture_client.go (4 hunks)
- services/omnirpc/http/fasthttp.go (6 hunks)
- services/omnirpc/http/resty.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
services/omnirpc/http/resty.go (5)
5-10: LGTM: Import statements updated correctlyThe new import statements are correctly added to support the tracing and metrics functionality. All imports appear to be used in the code.
15-16: LGTM: RestyClient struct updated with metrics handlerThe
RestyClientstruct has been correctly updated with ahandlerfield of typemetrics.Handlerto support the new metrics functionality.
21-22: LGTM: NewRestyClient function updated correctlyThe
NewRestyClientfunction has been properly updated to accept ametrics.Handlerparameter and initialize theRestyClientstruct with both the resty client and the metrics handler.
34-36: LGTM: NewRequest method updated to initialize handlerThe
NewRequestmethod has been correctly updated to initialize thehandlerfield of therestyRequeststruct with thehandlerfrom theRestyClient. This change addresses the previous concern about potential nil pointer dereferences.
28-28: LGTM with a note: restyRequest struct updated with metrics handlerThe
restyRequeststruct has been correctly updated with ahandlerfield of typemetrics.Handlerto support the new metrics functionality. However, ensure that this field is always properly initialized to prevent potential nil pointer dereferences, as mentioned in a previous review comment.To verify the proper initialization of the
handlerfield, run the following script:services/omnirpc/http/capture_client.go (4)
7-11: LGTM: New imports for tracing and metrics.The added imports are necessary for the new tracing and metrics functionality. They are correctly formatted and placed.
18-18: LGTM: Added metrics handler to CaptureClient.The new
handlerfield of typemetrics.Handleris a good addition to theCaptureClientstruct. It allows for metrics handling within the client.
25-26: LGTM: Updated NewCaptureClient to include metrics handler.The
NewCaptureClientfunction has been correctly updated to accept ametrics.Handlerparameter and initialize thehandlerfield in the returnedCaptureClient. This change is consistent with the struct modification.
39-39: LGTM: Initialized Handler in NewRequest method.The
Handlerfield of theCapturedRequestis now correctly initialized with thehandlerfrom theCaptureClient. This ensures that each new request has access to the metrics handler.services/omnirpc/http/fasthttp.go (3)
6-8: LGTM: Import statements updated correctlyThe new import statements are appropriate for the added functionality related to metrics and tracing. They include necessary packages for synchronization, time operations, metrics handling, and OpenTelemetry tracing.
Also applies to: 12-12, 14-15
171-171: LGTM: Newhandlerfield added, ensure proper initializationThe addition of the
handler metrics.Handlerfield to thefastHTTPRequeststruct is appropriate for the new metrics and tracing functionality. However, ensure thathandleris properly initialized when creating newfastHTTPRequestinstances to prevent potential nil pointer dereferences.Run the following script to check all places where
fastHTTPRequestis created and verify thathandleris properly initialized:#!/bin/bash # Description: Find all instantiations of fastHTTPRequest and check initialization of 'handler' # Search for fastHTTPRequest instantiations rg --type go 'fastHTTPRequest\{' -A 5 -B 2
33-33: LGTM: Newhandlerfield added, but ensure proper initializationThe addition of the
handler metrics.Handlerfield to thefastHTTPClientstruct is appropriate for the new metrics and tracing functionality. However, ensure thathandleris properly initialized to prevent potential nil pointer dereferences when it is used.Run the following script to check where
NewFastHTTPClientis called and verify thathandleris properly initialized:✅ Verification successful
Verified:
handlerfield is properly initializedThe
handlerfield in thefastHTTPRequeststruct is consistently initialized withf.handlerin all instances ofNewFastHTTPClient. This ensures that the handler is nevernil, preventing potential nil pointer dereferences.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to NewFastHTTPClient and check initialization of 'handler' # Search for NewFastHTTPClient usage rg --type go 'NewFastHTTPClient\((.*)\)' -A 2 -B 2Length of output: 472
| func (r *restyRequest) Do() (_ Response, err error) { | ||
| _, span := r.handler.Tracer().Start( | ||
| r.Request.Context(), | ||
| "Do", | ||
| trace.WithAttributes( | ||
| attribute.String("uri", r.endpoint), | ||
| attribute.String("headers", fmt.Sprintf("%v", r.Request.Header)), | ||
| attribute.String("body", r.Request.Body.(string)), | ||
| ), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
| resp, err := r.Request.Post(r.endpoint) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get response from %s: %w", r.endpoint, err) | ||
| } | ||
| return resp, nil |
There was a problem hiding this comment.
💡 Codebase verification
Security Issues Detected in Do Method
The Do method in services/omnirpc/http/resty.go logs sensitive information such as uri, headers, and body using trace.WithAttributes. This practice can expose sensitive data and pose security risks.
Issues:
- Sensitive Data Logging: Logging
headersandbodycan leak confidential information. - Endpoint Exposure: Including the
uriin error messages may reveal sensitive endpoint details.
Recommendations:
-
Sanitize or Remove Sensitive Information from Trace Attributes:
trace.WithAttributes( attribute.String("uri", sanitizeURI(r.endpoint)), attribute.String("headers", "[REDACTED]"), attribute.String("body_length", fmt.Sprintf("%d", len(r.Request.Body.(string)))), ),
-
Sanitize the Endpoint in Error Messages:
return nil, fmt.Errorf("could not get response from [REDACTED]: %w", err)
-
Validate Endpoint Before Making Requests:
if r.endpoint == "" { return nil, fmt.Errorf("endpoint is not set") }
🔗 Analysis chain
LGTM with security concerns: Do method updated with tracing and improved error handling
The Do method has been updated with tracing functionality and improved error handling, which are good improvements. However, there are some security concerns to address:
-
Logging sensitive information: The method is logging headers and the entire request body as trace attributes. This could potentially expose sensitive data.
-
Error message: The error message includes the endpoint, which might contain sensitive information.
Consider the following improvements:
-
Sanitize or remove sensitive information from trace attributes:
trace.WithAttributes( attribute.String("uri", sanitizeURI(r.endpoint)), attribute.String("headers", "[REDACTED]"), attribute.String("body_length", fmt.Sprintf("%d", len(r.Request.Body.(string)))), ),
-
Sanitize the endpoint in the error message:
return nil, fmt.Errorf("could not get response from [REDACTED]: %w", err)
-
Consider adding a check for
r.endpointbeing empty before making the request.
To verify the usage of sensitive information in tracing, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential logging of sensitive information in tracing
# Test: Search for trace.WithAttributes calls and check for potential sensitive data
rg --type go -e 'trace\.WithAttributes' -A 5 | grep -E '(headers|body|uri)'
Length of output: 1915
Script:
#!/bin/bash
# Description: Identify instances of trace.WithAttributes that log headers, body, or uri in Go files.
# Search for 'trace.WithAttributes' in Go files and filter for 'headers', 'body', or 'uri'
rg --type go 'trace\.WithAttributes' | grep -E '(headers|body|uri)'
Length of output: 983
| Request: &fasthttp.Request{}, | ||
| client: f, | ||
| context: nil, | ||
| handler: f.handler, |
There was a problem hiding this comment.
Initialize context with a default value and verify handler initialization
The initialization of the handler field looks correct. However, initializing context to nil might lead to nil pointer dereferences if not properly handled in subsequent operations.
Consider initializing context with a default value, such as context.Background(). Also, ensure that f.handler is properly initialized before being used here.
Apply this diff to initialize context with a default value:
return &fastHTTPRequest{
Request: &fasthttp.Request{},
client: f,
- context: nil,
+ context: context.Background(),
handler: f.handler,
}📝 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.
| Request: &fasthttp.Request{}, | |
| client: f, | |
| context: nil, | |
| handler: f.handler, | |
| Request: &fasthttp.Request{}, | |
| client: f, | |
| context: context.Background(), | |
| handler: f.handler, |
| func NewFastHTTPClient(handler metrics.Handler) Client { | ||
| return &fastHTTPClient{ | ||
| clients: xsync.NewMapOf[FastClient](), | ||
| defaultClient: &fasthttp.Client{ | ||
| NoDefaultUserAgentHeader: true, | ||
| Dial: dialer.Dial, | ||
| DialDualStack: false, | ||
| ReadTimeout: time.Second * 30, | ||
| WriteTimeout: time.Second * 30, | ||
| DisableHeaderNamesNormalizing: true, | ||
| DisablePathNormalizing: true, | ||
| }, | ||
| handler: handler, | ||
| } |
There was a problem hiding this comment.
Add nil check for handler parameter
The function signature update and initialization of the handler field are correct. However, consider adding a nil check for the handler parameter to prevent potential issues if a nil handler is passed.
Apply this diff to add a nil check:
func NewFastHTTPClient(handler metrics.Handler) Client {
+ if handler == nil {
+ handler = metrics.DefaultHandler() // Replace with appropriate default handler
+ }
return &fastHTTPClient{
clients: xsync.NewMapOf[FastClient](),
defaultClient: &fasthttp.Client{
NoDefaultUserAgentHeader: true,
Dial: dialer.Dial,
DialDualStack: false,
ReadTimeout: time.Second * 30,
WriteTimeout: time.Second * 30,
DisableHeaderNamesNormalizing: true,
DisablePathNormalizing: true,
},
handler: handler,
}
}Committable suggestion was skipped due to low confidence.
| _, span := f.handler.Tracer().Start( | ||
| f.context, | ||
| "Do", | ||
| trace.WithAttributes( | ||
| attribute.String("uri", f.Request.URI().String()), | ||
| attribute.String("headers", fmt.Sprintf("%v", f.Request.Header.String())), | ||
| attribute.String("body", string(f.Request.Body())), | ||
| ), | ||
| ) | ||
| defer func() { | ||
| metrics.EndSpan(span) | ||
| }() |
There was a problem hiding this comment.
Limit or sanitize body content in span attributes
The addition of tracing spans enhances observability, which is good. However, including the entire request body in the span attributes might lead to performance issues for large payloads and potential exposure of sensitive data.
Consider limiting the amount of body content included in the span attributes or implement a sanitization mechanism to prevent sensitive data exposure.
Apply this diff to limit the body content:
_, span := f.handler.Tracer().Start(
f.context,
"Do",
trace.WithAttributes(
attribute.String("uri", f.Request.URI().String()),
attribute.String("headers", fmt.Sprintf("%v", f.Request.Header.String())),
- attribute.String("body", string(f.Request.Body())),
+ attribute.String("body_length", fmt.Sprintf("%d", len(f.Request.Body()))),
),
)
defer func() {
metrics.EndSpan(span)
}()📝 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.
| _, span := f.handler.Tracer().Start( | |
| f.context, | |
| "Do", | |
| trace.WithAttributes( | |
| attribute.String("uri", f.Request.URI().String()), | |
| attribute.String("headers", fmt.Sprintf("%v", f.Request.Header.String())), | |
| attribute.String("body", string(f.Request.Body())), | |
| ), | |
| ) | |
| defer func() { | |
| metrics.EndSpan(span) | |
| }() | |
| _, span := f.handler.Tracer().Start( | |
| f.context, | |
| "Do", | |
| trace.WithAttributes( | |
| attribute.String("uri", f.Request.URI().String()), | |
| attribute.String("headers", fmt.Sprintf("%v", f.Request.Header.String())), | |
| attribute.String("body_length", fmt.Sprintf("%d", len(f.Request.Body()))), | |
| ), | |
| ) | |
| defer func() { | |
| metrics.EndSpan(span) | |
| }() |
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
WithMetricsmethod to associate requests with metrics handling.Bug Fixes
ActiveQuoteRequestStatusconstants.Chores