GO-3747 stacktrace logger improvements#1383
Conversation
requilence
commented
Jul 10, 2024
requilence
commented
- send most actual stacktrace when request is finished
- double check if it really contains the method goroutine
WalkthroughThis update introduces enhanced monitoring and debugging capabilities. Key changes include the ability to track and log long-running gRPC unary requests using atomic operations and predefined time intervals, a new function to compress byte slices, and an updated method to handle stack traces more efficiently. Additionally, a new event type for logging long method executions is implemented. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- metrics/interceptors.go (2 hunks)
- util/debug/debug.go (2 hunks)
Additional context used
GitHub Check: lint
util/debug/debug.go
[failure] 35-35:
Error return value ofgz.Writeis not checked (errcheck)
Additional comments not posted (4)
util/debug/debug.go (1)
29-30: LGTM!The
StackCompactfunction now usesCompressBytesto compress the stack trace.metrics/interceptors.go (3)
10-10: LGTM!The import of
go.uber.org/atomicis necessary for the newatomic.Stringvariable.
24-26: LGTM!The variables
UnaryTraceCollectionsandUnaryWarningInProgressIndexare correctly declared and initialized.
71-106: LGTM! But verify the function usage in the codebase.The
SharedLongMethodsInterceptorfunction includes the new logic for monitoring and logging long-running gRPC unary requests usingatomic.String. The changes are approved.However, ensure that the function is used correctly in the codebase.
Verification successful
Verify the context around the usage of
SharedLongMethodsInterceptor.The
SharedLongMethodsInterceptorfunction is used inmetrics/interceptors.goandclientlibrary/service/lib.go. Ensure that these usages are consistent with the new logic introduced.
The function
SharedLongMethodsInterceptoris used correctly in the codebase.The usage of
SharedLongMethodsInterceptorinmetrics/interceptors.goandclientlibrary/service/lib.gois consistent with the new logic introduced. Both usages are correctly integrating the new monitoring and logging functionality.
metrics/interceptors.go: Used withinLongMethodsInterceptor.clientlibrary/service/lib.go: Used withinregisterClientCommandsHandler.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `SharedLongMethodsInterceptor` in the codebase. # Test: Search for the function usage. Expect: Correct usage of the new logic. rg --type go -A 5 $'SharedLongMethodsInterceptor'Length of output: 1121
Script:
#!/bin/bash # Description: Analyze the context around the usage of `SharedLongMethodsInterceptor`. # Extract and display the relevant context in `metrics/interceptors.go`. rg --type go -A 10 -B 10 $'SharedLongMethodsInterceptor' metrics/interceptors.go # Extract and display the relevant context in `clientlibrary/service/lib.go`. rg --type go -A 10 -B 10 $'SharedLongMethodsInterceptor' clientlibrary/service/lib.goLength of output: 1987
| func CompressBytes(s []byte) string { | ||
| var buf bytes.Buffer | ||
| gz := gzip.NewWriter(&buf) | ||
| _, _ = gz.Write(Stack(allGoroutines)) | ||
| _, _ = gz.Write(s) | ||
| _ = gz.Close() |
There was a problem hiding this comment.
Check the error return value of gz.Write.
The error return value of gz.Write is not checked. This could lead to unnoticed errors during the compression process.
- _, _ = gz.Write(s)
+ if _, err := gz.Write(s); err != nil {
+ return ""
+ }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 CompressBytes(s []byte) string { | |
| var buf bytes.Buffer | |
| gz := gzip.NewWriter(&buf) | |
| _, _ = gz.Write(Stack(allGoroutines)) | |
| _, _ = gz.Write(s) | |
| _ = gz.Close() | |
| func CompressBytes(s []byte) string { | |
| var buf bytes.Buffer | |
| gz := gzip.NewWriter(&buf) | |
| if _, err := gz.Write(s); err != nil { | |
| return "" | |
| } | |
| _ = gz.Close() |
Tools
GitHub Check: lint
[failure] 35-35:
Error return value ofgz.Writeis not checked (errcheck)
|
|
||
| var ( | ||
| // every duration will be added to the previous ones | ||
| UnaryTraceCollections = []time.Duration{time.Second * 3, time.Second * 7, time.Second * 10, time.Second * 30, time.Second * 50} // 3, 10, 20, 50, 100 |
There was a problem hiding this comment.
misleading comment
// 3, 10, 20, 50, 100
this is not true
There was a problem hiding this comment.
@fat-fellow because it runs after each other, the durations are cumulative
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
…ug-clickhouse GO-3747 Improve stacktrace debug clickhouse
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- metrics/events.go (1 hunks)
- metrics/interceptors.go (2 hunks)
- util/debug/debug.go (3 hunks)
- util/debug/debug_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- metrics/interceptors.go
Additional context used
golangci-lint
util/debug/debug.go
[error] 43-43: wrapperFunc: use strings.ReplaceAll method in
strings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1)(gocritic)
GitHub Check: lint
util/debug/debug.go
[failure] 43-43:
wrapperFunc: use strings.ReplaceAll method instrings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1)(gocritic)
[failure] 74-74:
Error return value ofgz.Writeis not checked (errcheck)
Additional comments not posted (2)
util/debug/debug_test.go (1)
1-14: LGTM!The new test function
Testcorrectly verifies the parsing of goroutine stack traces usingrequire.Equal.metrics/events.go (1)
403-422: LGTM!The new type
LongMethodEventand its associated methodsGetBackendandMarshalFastJsonare correctly implemented, following the same pattern as other event types in the file.
| _, _ = gz.Write(s) | ||
| _ = gz.Close() |
There was a problem hiding this comment.
Check the error return value of gz.Write.
Ignoring the error return value of gz.Write could lead to unnoticed errors during the compression process.
- _, _ = gz.Write(s)
+ if _, err := gz.Write(s); err != nil {
+ return ""
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: lint
[failure] 74-74:
Error return value ofgz.Writeis not checked (errcheck)
| lineCount = 0 | ||
| } else { | ||
| if lineCount < 3 { | ||
| sb.WriteString(strings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1)) |
There was a problem hiding this comment.
Use strings.ReplaceAll for better readability and performance.
The strings.ReplaceAll method is more concise and avoids the need for specifying the -1 parameter.
- sb.WriteString(strings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1))
+ sb.WriteString(strings.ReplaceAll(addressPattern.ReplaceAllString(line, ""), "\t", ""))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.
| sb.WriteString(strings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1)) | |
| sb.WriteString(strings.ReplaceAll(addressPattern.ReplaceAllString(line, ""), "\t", "")) |
Tools
golangci-lint
[error] 43-43: wrapperFunc: use strings.ReplaceAll method in
strings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1)(gocritic)
GitHub Check: lint
[failure] 43-43:
wrapperFunc: use strings.ReplaceAll method instrings.Replace(addressPattern.ReplaceAllString(line, ""), "\t", "", -1)(gocritic)
2 participants
