trace [SLT-330] #3264
trace [SLT-330] #3264
Conversation
[goreleaser]
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3264 +/- ##
===================================================
+ Coverage 24.97972% 30.68453% +5.70480%
===================================================
Files 340 447 +107
Lines 22190 29787 +7597
Branches 82 82
===================================================
+ Hits 5543 9140 +3597
- Misses 16081 19797 +3716
- Partials 566 850 +284
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
0b2dc8e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4cda0d15.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-track-issue.sanguine-fe.pages.dev |
WalkthroughThe changes in this pull request introduce a new constant 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
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- services/omnirpc/http/client.go (1 hunks)
- services/omnirpc/modules/receiptsbackup/receiptsbackup.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/omnirpc/http/client.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/omnirpc/modules/receiptsbackup/receiptsbackup.go
[warning] 140-143: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L140-L143
Added lines #L140 - L143 were not covered by tests
[warning] 150-151: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L150-L151
Added lines #L150 - L151 were not covered by tests
[warning] 202-203: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L202-L203
Added lines #L202 - L203 were not covered by tests
🔇 Additional comments (2)
services/omnirpc/modules/receiptsbackup/receiptsbackup.go (2)
150-151:⚠️ Potential issueAvoid logging request bodies to prevent potential PII leakage.
Logging the request body in the tracing event may expose sensitive user data. Consider sanitizing the request body before logging or avoid logging the entire body to protect sensitive information.
Please verify that the request body does not contain sensitive information before logging.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 150-151: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L150-L151
Added lines #L150 - L151 were not covered by tests
202-203:⚠️ Potential issuePrevent potential PII leakage by avoiding logging full response bodies.
Logging the response body in the tracing event may expose sensitive or confidential information. It's recommended to avoid logging full response bodies or to sanitize any sensitive data before logging.
Please ensure that the response body does not contain sensitive information before logging.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-203: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L202-L203
Added lines #L202 - L203 were not covered by tests
| ctx, span := r.handler.Tracer().Start(ctx, "proxyrequest") | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() |
There was a problem hiding this comment.
Increase test coverage for the new tracing code in processRequest.
The tracing code added in lines 140-143 is not currently covered by tests, as indicated by the static analysis tools. Adding unit tests will help ensure that the tracing functionality works as expected and improve the reliability of the code.
Would you like assistance in writing unit tests for this code?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 140-143: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L140-L143
Added lines #L140 - L143 were not covered by tests
| span.AddEvent("request marshaled", trace.WithAttributes(attribute.String("body", string(body)))) | ||
|
|
There was a problem hiding this comment.
Improve test coverage for the added tracing event.
The new tracing event in lines 150-151 is not covered by tests. Adding tests can ensure that this functionality is working correctly and help detect any issues early.
Would you like help in creating tests for this code?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 150-151: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L150-L151
Added lines #L150 - L151 were not covered by tests
| span.AddEvent("response returned", trace.WithAttributes(attribute.String("body", string(resp.Body())))) | ||
|
|
There was a problem hiding this comment.
Add tests for the new tracing event to improve test coverage.
The tracing event added in lines 202-203 is not covered by tests, according to the static analysis tools. Please consider adding unit tests to cover this new functionality and enhance code reliability.
Do you need assistance in writing tests for this code?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-203: services/omnirpc/modules/receiptsbackup/receiptsbackup.go#L202-L203
Added lines #L202 - L203 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ethergo/util/attributes.go (1)
Line range hint
38-56: Consider performance impact of including raw transaction data.While adding the raw transaction data enhances tracing capabilities, it may have performance implications for large transactions. This could potentially increase memory usage and network traffic when these attributes are transmitted.
Consider implementing a configuration option to enable/disable the inclusion of raw transaction data based on the specific use case or environment requirements. This would allow for flexibility in balancing between detailed tracing and performance.
Example implementation:
var IncludeRawTransactionData bool func TxToAttributes(transaction *types.Transaction) []attribute.KeyValue { // ... existing code ... if IncludeRawTransactionData { bin, err := transaction.MarshalBinary() if err != nil { bin = []byte(fmt.Sprintf("could not be marshalled: %v", err)) } attributes = append(attributes, attribute.String(txRawAttr, common.Bytes2Hex(bin))) } // ... rest of the function ... }This approach allows users to control when to include the raw transaction data, optimizing for their specific needs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- ethergo/util/attributes.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (1.22.x)
ethergo/util/attributes.go
[failure] 56-56:
syntax error: unexpected ) in composite literal; possibly missing comma or }
🔇 Additional comments (2)
ethergo/util/attributes.go (2)
25-25: LGTM: New constant for raw transaction attribute.The new constant
txRawAttris well-named and consistent with the existing naming convention. It provides a clear identifier for the raw transaction data attribute.
38-41: LGTM: Transaction marshalling with error handling.The new code block effectively marshals the transaction to binary format and includes proper error handling. This ensures that even if marshalling fails, a meaningful error message is captured instead of causing a runtime error.
ethergo/util/attributes.go
Outdated
| @@ -46,9 +53,10 @@ | |||
| // nolint: gosec | |||
| attribute.Int64(gasLimitAttr, int64(transaction.Gas())), | |||
| attribute.String(chainIDAttr, BigPtrToString(transaction.ChainId())), | |||
| attribute.String(txRawAttr, common.Bytes2Hex(bin))), | |||
There was a problem hiding this comment.
Fix syntax error in attribute addition.
There's a syntax error in the new attribute addition. The closing parenthesis is misplaced.
Please apply the following fix:
- attribute.String(txRawAttr, common.Bytes2Hex(bin))),
+ attribute.String(txRawAttr, common.Bytes2Hex(bin)),📝 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.
| attribute.String(txRawAttr, common.Bytes2Hex(bin))), | |
| attribute.String(txRawAttr, common.Bytes2Hex(bin)), |
🧰 Tools
🪛 GitHub Check: Build (1.22.x)
[failure] 56-56:
syntax error: unexpected ) in composite literal; possibly missing comma or }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ethergo/util/attributes.go (2)
38-41: LGTM with a minor suggestion: Enhanced transaction attributes with raw data.The addition of raw transaction data to the attributes is a valuable enhancement for tracing. The implementation is correct and follows good practices.
A minor suggestion for improvement:
Consider using a more specific error message when marshalling fails. For example:
- bin = []byte(fmt.Sprintf("could not be marshalled: %v", err)) + bin = []byte(fmt.Sprintf("failed to marshal transaction: %v", err))This provides more context about what operation failed.
Also applies to: 56-56
🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 40-40:
marshalledis a misspelling ofmarshaled(misspell)
40-40: Correct spelling for consistency: "marshalled" to "marshaled".To maintain consistency with American English spelling used throughout the codebase, please update the spelling of "marshalled" to "marshaled".
Apply the following change:
- bin = []byte(fmt.Sprintf("could not be marshalled: %v", err)) + bin = []byte(fmt.Sprintf("could not be marshaled: %v", err))🧰 Tools
🪛 GitHub Check: Lint (ethergo)
[failure] 40-40:
marshalledis a misspelling ofmarshaled(misspell)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- ethergo/util/attributes.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint (ethergo)
ethergo/util/attributes.go
[failure] 40-40:
marshalledis a misspelling ofmarshaled(misspell)
🔇 Additional comments (1)
ethergo/util/attributes.go (1)
25-25: LGTM: New constant for raw transaction attribute.The new constant
txRawAttris well-named and consistent with the existing naming convention. It provides a clear identifier for the raw transaction data in the attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ethergo/util/export_test.go (1)
75-76: LGTM! Consider adding a descriptive comment.The addition of
TxRawAttris consistent with the existing pattern of exporting constants for testing purposes.Consider adding a descriptive comment for
TxRawAttr, similar to the other constants in this group. For example:+ // TxRawAttr exports txRawAttr for testing. TxRawAttr = txRawAttrThis would improve consistency and provide clarity on the constant's purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- ethergo/util/attributes.go (3 hunks)
- ethergo/util/attributes_test.go (1 hunks)
- ethergo/util/export_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ethergo/util/attributes.go
🧰 Additional context used
🔇 Additional comments (1)
ethergo/util/attributes_test.go (1)
112-115: Excellent addition to test coverage!The new test case effectively verifies that the raw transaction data is correctly captured in the
TxRawAttrattribute. This addition enhances the overall test coverage and ensures the integrity of the transaction attribute mapping functionality.The implementation is clean and follows good practices:
- Proper error handling with
u.Require().NoError(err).- Appropriate use of
MarshalBinary()andcommon.Bytes2Hex()for data conversion.- Direct comparison of the marshaled data with the
TxRawAttrvalue.This change will help maintain the reliability of the transaction attribute handling in the future.
[goreleaser]
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
Documentation
Requestinterface.