[SLT-331] feat(omnirpc): omnirpc request tracing #3289
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,29 +2,37 @@ | |
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/go-resty/resty/v2" | ||
| "github.com/synapsecns/sanguine/core/metrics" | ||
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/trace" | ||
| ) | ||
|
|
||
| // RestyClient is a resty client for making requests to the http client. | ||
| type RestyClient struct { | ||
| client *resty.Client | ||
| handler metrics.Handler | ||
| } | ||
|
|
||
| // NewRestyClient creates a resty client. | ||
| // while much slower than fasthttp, this client requests context cancellation. | ||
| func NewRestyClient(handler metrics.Handler) Client { | ||
| return &RestyClient{client: resty.New(), handler: handler} | ||
| } | ||
|
|
||
| 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 Consider ensuring that There was a problem hiding this comment. Potential nil pointer dereference of The To prevent this, ensure that Example fix by initializing 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 |
||
| } | ||
|
|
||
| // NewRequest create a new request. | ||
| func (r RestyClient) NewRequest() Request { | ||
| return &restyRequest{ | ||
| Request: r.client.R(), | ||
| handler: r.handler, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -54,9 +62,24 @@ | |
| return r | ||
| } | ||
|
|
||
| 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 | ||
|
Comment on lines
+65
to
+82
There was a problem hiding this comment. 💡 Codebase verification Security Issues Detected in Do Method The Issues:
Recommendations:
🔗 Analysis chainLGTM with security concerns: Do method updated with tracing and improved error handling The
Consider the following improvements:
To verify the usage of sensitive information in tracing, run the following script: 🏁 Scripts executedThe 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 |
||
| } | ||
|
|
||
| var _ Client = &RestyClient{} | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,17 +1,25 @@ | ||||||||||
| package http_test | ||||||||||
|
|
||||||||||
| import ( | ||||||||||
| "testing" | ||||||||||
|
|
||||||||||
| "github.com/brianvoe/gofakeit/v6" | ||||||||||
| "github.com/stretchr/testify/suite" | ||||||||||
| "github.com/synapsecns/sanguine/core" | ||||||||||
| "github.com/synapsecns/sanguine/core/config" | ||||||||||
| "github.com/synapsecns/sanguine/core/metrics" | ||||||||||
| "github.com/synapsecns/sanguine/core/metrics/localmetrics" | ||||||||||
| "github.com/synapsecns/sanguine/core/testsuite" | ||||||||||
| "github.com/synapsecns/sanguine/services/omnirpc/http" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| var buildInfo = config.NewBuildInfo(config.DefaultVersion, config.DefaultCommit, "omnirpc", config.DefaultDate) | ||||||||||
|
|
||||||||||
| // clientSuite defines the basic test suite. | ||||||||||
| type HTTPSuite struct { | ||||||||||
| *testsuite.TestSuite | ||||||||||
| clients []http.Client | ||||||||||
| metrics metrics.Handler | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // NewTestSuite creates a new test suite and performs some basic checks afterward. | ||||||||||
|
|
@@ -27,8 +35,24 @@ func (c *HTTPSuite) SetupTest() { | |||||||||
| c.TestSuite.SetupTest() | ||||||||||
|
|
||||||||||
| for _, clientType := range http.AllClientTypes { | ||||||||||
| c.clients = append(c.clients, http.NewClient(metrics.NewNullHandler(), clientType)) | ||||||||||
| } | ||||||||||
|
Comment on lines
+38
to
+39
There was a problem hiding this comment. Use the initialized metrics handler when creating clients In Apply this diff to use the metrics handler stored in 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
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| func (c *HTTPSuite) SetupSuite() { | ||||||||||
| c.TestSuite.SetupSuite() | ||||||||||
| // don't use metrics on ci for integration tests | ||||||||||
| isCI := core.GetEnvBool("CI", false) | ||||||||||
| useMetrics := !isCI | ||||||||||
| metricsHandler := metrics.Null | ||||||||||
|
|
||||||||||
| if useMetrics { | ||||||||||
| localmetrics.SetupTestJaeger(c.GetSuiteContext(), c.T()) | ||||||||||
| metricsHandler = metrics.Jaeger | ||||||||||
| } | ||||||||||
| var err error | ||||||||||
| c.metrics, err = metrics.NewByType(c.GetSuiteContext(), buildInfo, metricsHandler) | ||||||||||
| c.Require().NoError(err) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func TestCommonSuite(t *testing.T) { | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enforce the association of
metrics.HandlerwithCapturedRequest.To prevent issues with uninitialized
c.Handler, consider requiring themetrics.Handlerto be set during the creation ofCapturedRequestinstances.Modify the
NewRequestmethod to accept ametrics.Handlerparameter:This ensures that every
CapturedRequesthas a validHandlerandContextfrom the beginning.Also applies to: 161-164