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

Update tracing check for whether error has stack #4982

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Jun 1, 2024

Use the pkg/errors and stack package interface to check for a stack. Use the standard library methods to unwrap errors.

Use the pkg/errors and stack package interface to check for a stack. Use
the standard library methods to unwrap errors.

Signed-off-by: Derek McGowan <derek@mcg.dev>
case interface{ Unwrap() []error }:
for _, ue := range e.Unwrap() {
st = Traces(ue)
// Only take first stack
Copy link
Member

Choose a reason for hiding this comment

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

Does this make sense? For nested errors it is expected that there are multiple stacktraces and they are all merged into array here.

Copy link
Member Author

Choose a reason for hiding this comment

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

My worry is that merging the arrays in this case would lead to a stack which doesn't make sense. Essentially this would be the case where you had multiple calls to errors.Wrap from either a loop or multiple goroutines running in parallel, then combined using errors.Join. The stacks would not be part of a single stack. I think this case would be rare and the case where that is used, you could have multiple of the same stack from parallel processes running into the same error case.

If we wanted to support that, then [][]*Stack as the return would make more sense. It doesn't seem like a very relevant edge case to me though.

@tonistiigi tonistiigi merged commit c512ea7 into moby:master Jun 5, 2024
74 checks passed
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.

None yet

2 participants