Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
==========================================
+ Coverage 98.18% 98.50% +0.31%
==========================================
Files 6 6
Lines 276 334 +58
==========================================
+ Hits 271 329 +58
Misses 4 4
Partials 1 1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Collaborator
Author
|
One thing that I'd really like input on is this: I'm torn on this. CC @prashantv |
prashantv
reviewed
Oct 22, 2023
internal/stack/stacks_test.go
Outdated
| want: "example.com/foo/bar.baz", | ||
| }, | ||
| { | ||
| name: "created by/in goroutine", // Go 1.21 |
Collaborator
There was a problem hiding this comment.
should we add some testdata of real stack traces from go 1.20 / 1.21 and ensure they parse correctly too?
Collaborator
Author
|
Update:
|
Adds support to the stack parser for reading the full list of functions for a stack trace. This includes the function that created the stack trace; it's the bottom 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.
In Go 1.20, the "created by" lines do not include the "in goroutine" portion.
`Full()` was accidentally dropping the file names from the full traces.
The function tha created a goroutine should not be considered part of its stack. However, we can use that entry to mark the end of a stack trace.
prashantv
approved these changes
Oct 22, 2023
Collaborator
prashantv
left a comment
There was a problem hiding this comment.
LGTM, can also do testdata traces in a follow-up, doesn't have to be part of this PR.
To verify the stacktrace parsing logic, generate real stack traces under the following conditions: - Go 1.21 - Go 1.20 installed with gimme - Go 1.21 with tracebackancestors=10 set The test verifies that the parsed stack traces do not include functions that we did not expect to see in a goroutine's trace.
Collaborator
Author
|
We should set up a codecov configuration that we actually follow. This is just ignored right now.
|
abhinav
added a commit
that referenced
this pull request
Oct 23, 2023
Instead of matching for built-in functions with strings.Contains,
use the parsed stack information to match function names exactly.
Following this change, the only remaining strings.Contains are
to match on the goroutine state:
```
% rg strings.Contains
utils_test.go
84: if strings.Contains(s.State(), "run") {
internal/stack/stacks_test.go
249: if strings.Contains(s.State(), "run") {
```
Resolves #41
Depends on #111
abhinav
added a commit
that referenced
this pull request
Oct 23, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support to the stack parser for reading the full list of functions
for a stack trace.
NOTE:
This includes the function that created the stack trace;
it's the bottom 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 (PR incoming)
and allows implementing an IgnoreAnyFunction option
(similar to #80 that has since stalled).
Depends on #110