feat: The trace of log can be clicked as a link #735
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
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
CodeRabbit Configuration 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: 2
🧹 Outside diff range and nitpick comments (1)
log/formatter/general_test.go (1)
221-224: Consider adding more test cases for robustnessWhile the current test cases verify the basic functionality, consider adding:
- Test cases for malformed stack traces (missing line numbers, colons, or brackets)
- Test cases that explicitly verify the clickable nature of the links (format matches IDE expectations)
Here's a suggested test case to add:
tests := []struct { name string setup func() assert func() }{ + { + name: "Malformed stack trace", + setup: func() { + stackTraces = map[string]any{ + "root": map[string]any{ + "message": "error", + "stack": []string{ + "malformed/path/no/line/number [method]", + "malformed/path/no/brackets:123", + "just/a/path", + }, + }, + } + }, + assert: func() { + traces, err := general.formatStackTraces(stackTraces) + s.NotNil(err) + s.Contains(err.Error(), "invalid stack trace format") + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
log/formatter/general.go(1 hunks)log/formatter/general_test.go(2 hunks)
🔇 Additional comments (3)
log/formatter/general.go (1)
125-134: Verify the stack trace format across different environments
The current parsing logic makes assumptions about the stack trace format. We should verify that this works consistently across different environments and error types.
✅ Verification successful
Let me gather more information about the stack trace formatting implementation and its tests.
Let me check the actual implementation of the formatStackTraces function.
Stack trace parsing implementation is well-tested and handles format variations
The implementation is safe and well-structured for the following reasons:
- The code has comprehensive test coverage with test cases for both nil and structured stack traces
- The
StackTracestruct properly defines the expected format - The parsing logic has fallback handling: if the expected format (with two colons) isn't found, it prints the raw stack trace line
- The test suite includes a realistic example that matches the production format:
/dummy/examples/logging/example.go:143 [main.main]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stack trace generation patterns in the codebase
rg -A 5 'runtime\.Stack|debug\.Stack|stacktrace|stack trace'
# Look for existing stack trace format tests
rg -A 10 'TestFormatStackTraces|test.*stack.*trace' --type go
Length of output: 3670
Script:
#!/bin/bash
# Check the stack trace formatting test implementation
cat log/formatter/general_test.go
# Look for actual stack trace format implementation
ast-grep --pattern 'func $_ formatStackTraces($_) {
$$$
}'
Length of output: 6703
Script:
#!/bin/bash
# Look for the formatStackTraces implementation in the general.go file
cat log/formatter/general.go
Length of output: 3576
log/formatter/general_test.go (2)
211-212: LGTM! Wrap trace format is consistent
The wrap trace maintains the same clickable format as the main stack traces, ensuring consistency throughout the error chain.
203-206: LGTM! Stack trace format is IDE-friendly
The new stack trace format filepath:line [method] is well-structured and follows the common IDE-clickable format. This change aligns with the PR objective of making log traces clickable as links.
Let's verify the format consistency across the codebase:
✅ Verification successful
Let me verify the stack trace format consistency with a more targeted search:
Let me try a different search pattern to verify the stack trace format:
Let me check the entire test file context around these stack traces:
Stack trace format is consistent and well-structured
The stack trace format consistently follows the pattern filepath:line [method] throughout the test file, both in the input data and expected output assertions. All stack traces maintain this IDE-friendly format that makes them clickable:
- Input format:
/dummy/examples/logging/example.go:143 [main.main] - Expected format:
/dummy/examples/logging/example.go:143 [main.main]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any inconsistent stack trace formats in test files
rg -g '**/*_test.go' '(?<!:)\d+\s+\[' # Find traces without the filepath:line format
rg -g '**/*_test.go' '\][^\]]' # Find traces where method is not properly bracketed
Length of output: 145864
Script:
#!/bin/bash
# Search for stack trace patterns in test files
rg --pcre2 'filepath:\d+\s+\[[\w\.]+\]' log/formatter/
Length of output: 56
Script:
#!/bin/bash
# Search for stack trace patterns in test files
rg "\"stack\".*\[" log/formatter/
Length of output: 229
Script:
#!/bin/bash
# Get the full test context
rg -A 10 '"stack".*\[' log/formatter/general_test.go
Length of output: 991
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #735 +/- ##
==========================================
+ Coverage 69.38% 69.40% +0.01%
==========================================
Files 210 210
Lines 17030 17040 +10
==========================================
+ Hits 11817 11827 +10
Misses 4556 4556
Partials 657 657 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
log/formatter/general.go (1)
132-143: Add test cases for the new formatStackTrace functionTo ensure reliability and catch edge cases, we should add unit tests for the new function. Test cases should cover:
- Valid stack traces with file:line and method
- Invalid formats
- Edge cases (empty string, missing parts)
Would you like me to help generate comprehensive test cases for this function? This would help fulfill the PR objective's testing requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
log/formatter/general.go(1 hunks)
🔇 Additional comments (2)
log/formatter/general.go (2)
125-125: LGTM! Good separation of concerns.
The extraction of formatting logic into a separate function improves testability and code organization.
132-143:
Implementation needs revision to make traces clickable
The current implementation has several issues:
- The format doesn't follow IDE/terminal patterns for clickable files
- String manipulation is fragile without proper validation
- The implementation doesn't achieve the PR objective of making traces clickable
Consider this revised implementation that follows standard IDE patterns:
func formatStackTrace(stackStr string) string {
- lastColon := strings.LastIndex(stackStr, ":")
- if lastColon > 0 && lastColon < len(stackStr)-1 {
- secondLastColon := strings.LastIndex(stackStr[:lastColon], ":")
- if secondLastColon > 0 {
- fileLine := stackStr[secondLastColon+1:]
- method := stackStr[:secondLastColon]
- return fmt.Sprintf("\t%s [%s]\n", fileLine, method)
- }
- }
- return fmt.Sprintf("\t%s\n", stackStr)
+ // Split into file:line and method
+ parts := strings.Split(stackStr, " ")
+ if len(parts) < 2 {
+ return fmt.Sprintf("\t%s\n", stackStr)
+ }
+
+ // Validate file location format (path:line)
+ fileLocation := parts[0]
+ if !strings.Contains(fileLocation, ":") {
+ return fmt.Sprintf("\t%s\n", stackStr)
+ }
+
+ // Format as "file:line method" to enable IDE/terminal click-to-file
+ method := strings.Join(parts[1:], " ")
+ return fmt.Sprintf("\t%s %s\n", fileLocation, method)
}This revision:
- Preserves the
file:lineformat that IDEs/terminals recognize as clickable - Adds proper validation of the input format
- Maintains a cleaner separation between file location and method name
Let's verify if the stack trace format matches common IDE patterns:
#!/bin/bash
# Check if the stack trace format matches common IDE patterns
# Example: should match patterns like "/path/to/file.go:123"
# Search for current stack trace formats in the codebase
rg -A 2 'file\.go:\d+'
# Search for any existing clickable file patterns
ast-grep --pattern 'fmt.Sprintf("\t%s %s\n", $file, $_)'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
log/formatter/general_test.go (1)
241-300: LGTM! Comprehensive test coverage with room for enhancement.The test cases thoroughly cover various scenarios using a well-structured table-driven approach. Consider adding these additional test cases to improve coverage:
tests := []struct { name string input string expected string }{ // ... existing test cases ... + { + name: "Malformed path with spaces", + input: "main.handler:/path with spaces/file.go:42", + expected: "\t/path with spaces/file.go:42 [main.handler]\n", + }, + { + name: "Path with special characters", + input: "handler:/path/with-#special@chars/file.go:42", + expected: "\t/path/with-#special@chars/file.go:42 [handler]\n", + }, + { + name: "Windows-style path", + input: "main.handler:C:\\path\\to\\file.go:42", + expected: "\tC:\\path\\to\\file.go:42 [main.handler]\n", + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
log/formatter/general_test.go(3 hunks)
🔇 Additional comments (2)
log/formatter/general_test.go (2)
203-206: LGTM! Test data updated to reflect new clickable format.
The stack trace format changes consistently follow the new pattern of filepath:line [method], which aligns with making traces clickable as links.
Also applies to: 211-212, 221-224
241-300: Verify format compatibility with common development tools.
While the new format positions file paths for clickability, we should verify that it works as expected with common IDEs and tools. Could you:
- List the development tools/IDEs this format has been tested with
- Add a comment in the test file documenting the supported tools
📑 Description
Closes goravel/goravel#397
Summary by CodeRabbit
New Features
Bug Fixes
formatStackTracesmethod.✅ Checks