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
Merged

gracefully handle elided frames #120

Show file tree
Hide file tree
Changes from 5 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
7 changes: 7 additions & 0 deletions internal/stack/stacks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,13 @@ func (p *stackParser) parseStack(line string) (Stack, error) {
// Just skip it.
continue
}
if strings.HasPrefix(line, "...") && strings.HasSuffix(line, " frames elided...") {
// 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
29 changes: 27 additions & 2 deletions internal/stack/stacks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
package stack

import (
"bytes"
"os"
"path/filepath"
"runtime"
Expand Down Expand Up @@ -136,8 +137,8 @@ func TestCurrentCreatedBy(t *testing.T) {

func TestAllLargeStack(t *testing.T) {
const (
stackDepth = 100
numGoroutines = 100
stackDepth = 101
numGoroutines = 101
)

var started sync.WaitGroup
Expand All @@ -163,6 +164,13 @@ func TestAllLargeStack(t *testing.T) {
t.Fatalf("Expected larger stack buffer")
}

// Also test the stack parser here to ensure it handles elided frames gracefully.
// We want to check this here, so that if the format of the "elided frames" message changes, we catch it.
// At the time of writing this test, with a stack depth of 101, we get 2 elided frames:
// "...2 frames elided...".
_, err := newStackParser(bytes.NewReader(buf)).Parse()
require.NoError(t, err)
prashantv marked this conversation as resolved.
Show resolved Hide resolved

// Start enough goroutines so we exceed the default buffer size.
close(done)
}
Expand Down Expand Up @@ -263,6 +271,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