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.

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

Allow checking for functions anywhere in the stack #80

Closed
wants to merge 3 commits into from

Conversation

thomshutt
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented Sep 5, 2022

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #80 (bd1f0a0) into master (5c9bf00) will increase coverage by 1.36%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   94.92%   96.29%   +1.36%     
==========================================
  Files           5        5              
  Lines         138      243     +105     
==========================================
+ Hits          131      234     +103     
- Misses          4        6       +2     
  Partials        3        3              
Impacted Files Coverage Δ
internal/stack/stacks.go 89.61% <92.85%> (+4.19%) ⬆️
options.go 99.03% <100.00%> (-0.97%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@thomshutt
Copy link
Author

thomshutt commented Oct 11, 2022

👋 This has been open for a while now; if there's anything you'd like me to change then let me know. Similarly if it's not functionality that you want to add then I can close the PR.

@pohly
Copy link

pohly commented Feb 3, 2023

This would be useful for a more precise suppression of a goroutine in Kubernetes which is currently reported as being in time.Sleep.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

I'm +1 on adding this functionality, but don't think it should be done using strings.Contains on the raw stack.

I'd prefer if we updated the parser to understand the function names specifically, and allow matching on the function name only.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Thanks for updating the parsing logic, left a couple of comments on the parser + tests.

}

// AllFunctions returns the names of all functions on the stack.
func (s Stack) AllFunctions() []string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you update the tests in stacks_test.go to verify the behaviour of AllFunctions as well?

Comment on lines +106 to 108
if f := parseFunc(line); f != "" {
curStack.functions = append(curStack.functions, f)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks like it's trying to parse every line, should we instead parse only the lines known to be functions?

I think the format is:

<function>
<tab><file:line>
<function>
<tab><file:line>
...

Comment on lines +141 to +143
if idx := strings.LastIndex(line, "created by"); idx >= 0 {
return strings.TrimPrefix(line, "created by ")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

since it's not part of the stack, may be better to skip parsing these till it's required

@brandur
Copy link

brandur commented Jun 23, 2023

@thomshutt Any chance you're still around to address those code changes? I was just about to file an issue over lacking this functionality before I found this PR because I needed it too.

abhinav added a commit that referenced this pull request Oct 21, 2023
Adds a new IgnoreAnyFunction option to ignore stacks
that have the provided function anywhere in the stack,
not just the top.

To test this better, the helper blockedG.run function
was split into two.

Supersedes #80
abhinav added a commit that referenced this pull request Oct 22, 2023
Adds a new IgnoreAnyFunction option to ignore stacks
that have the provided function anywhere in the stack,
not just the top.

To test this better, the helper blockedG.run function
was split into two.

Supersedes #80
abhinav added a commit that referenced this pull request Oct 22, 2023
Adds a new IgnoreAnyFunction option to ignore stacks
that have the provided function anywhere in the stack,
not just the top.

To test this better, the helper blockedG.run function
was split into two.

Supersedes #80
abhinav added a commit that referenced this pull request Oct 23, 2023
Adds support to the stack parser for reading the full list of functions
for a stack trace.

NOTE:
The function that created the goroutine
is NOT considered part of the stack.

We don't maintain the order of the functions
since that's not something we need at this time.
The functions are all placed in a set.

This unblocks #41 and allows implementing an
IgnoreAnyFunction option (similar to the stalled #80 PR).

Depends on #110
abhinav added a commit that referenced this pull request Oct 23, 2023
Adds a new IgnoreAnyFunction option to ignore stacks
that have the provided function anywhere in the stack,
not just the top.

To test this better, the helper blockedG.run function
was split into two.

Supersedes #80
abhinav added a commit that referenced this pull request Oct 23, 2023
Adds a new IgnoreAnyFunction option to ignore stacks
that have the provided function anywhere in the stack,
not just the top.

To test this better, the helper blockedG.run function
was split into two.

Supersedes #80
abhinav added a commit that referenced this pull request Oct 23, 2023
Adds a new IgnoreAnyFunction option to ignore stacks
that have the provided function anywhere in the stack,
not just the top.

To test this better, the helper blockedG.run function
was split into two.

Supersedes #80
Depends on #111
@abhinav
Copy link
Collaborator

abhinav commented Oct 23, 2023

Superseded by #113
Closing

@abhinav abhinav closed this Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants