Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gracefully handle elided frames #120

Merged
merged 6 commits into from
Apr 13, 2024
+36 −2
Merged

gracefully handle elided frames #120

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1650242
gracefully handle elided frames
drshriveer Mar 27, 2024
a96f254
make elided frames matcher more strict
drshriveer Mar 27, 2024
810b534
respond to PR feedback
drshriveer Apr 12, 2024
c192d3a
comment fix
drshriveer Apr 12, 2024
71a9df0
another comment tweak for clarity
drshriveer Apr 12, 2024
effe6af
Verify large stack has elided frames
prashantv Apr 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions internal/stack/stacks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,17 @@ import (
"errors"
"fmt"
"io"
"regexp"
"runtime"
"strconv"
"strings"
)

const _defaultBufferSize = 64 * 1024 // 64 KiB

// elidedFramesMatcher matches lines that indicate frames were elided; these cannot be parsed as functions.
var elidedFramesMatcher = regexp.MustCompile(`\.\.\.\d+ frames elided\.\.\.`)

// Stack represents a single Goroutine's stack.
type Stack struct {
id int
Expand Down Expand Up @@ -158,6 +162,13 @@ func (p *stackParser) parseStack(line string) (Stack, error) {
// Just skip it.
continue
}
if elidedFramesMatcher.MatchString(line) {
prashantv marked this conversation as resolved.
Show resolved Hide resolved
// e.g. ...23 frames elided...
// This indicates frames were elided from the stack trace,
// attempting to parse them via parseFuncName will fail resulting in a panic
// and a relatively useless output. Gracefully handle this.
continue
}

funcName, creator, err := parseFuncName(line)
if err != nil {
Expand Down
17 changes: 17 additions & 0 deletions internal/stack/stacks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,23 @@ func TestParseStack(t *testing.T) {
"example.com/foo/bar.baz",
},
},
{
name: "elided frames",
give: joinLines(
"goroutine 1 [running]:",
"example.com/foo/bar.baz()",
" example.com/foo/bar.go:123",
"...3 frames elided...",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to this fixed data test case, can we add or update a test that creates a goroutine with a deep stack that leads to elided frames, and verify that? That way, we'll have coverage that will detect if the elided message has changed.

This is a real concern, when the eliding logic was originally added, it used "stack frames omitted". Given that it has changed, we should have a test that will detect changes by relying on the real runtime logic.

"created by example.com/foo/bar.qux",
" example.com/foo/bar.go:456",
),
id: 1,
state: "running",
firstFunc: "example.com/foo/bar.baz",
funcs: []string{
"example.com/foo/bar.baz",
},
},
}

for _, tt := range tests {
Expand Down
Loading