-
Notifications
You must be signed in to change notification settings - Fork 810
[tmpnet] Add optional stack traces to errors originating from tmpnet #4262
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
Conversation
b2c6261 to
a355fb0
Compare
| switch { | ||
| case errors.Is(err, ErrUnrecoverableNodeHealthCheck): | ||
| return fmt.Errorf("%w for node %q", err, n.NodeID) | ||
| return stacktrace.Errorf("node %q saw unrecoverable health check: %w", n.NodeID, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to reorder the args to ensure the error was last so that the stacktrace could be added to it.
a355fb0 to
68051ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds optional stacktraces to errors originating from tmpnet by introducing a new stacktrace package and integrating it throughout the tmpnet codebase. When STACK_TRACE_ERRORS=1 is set, errors will include stacktraces collected at the deepest point in the call chain for debugging assistance.
Key changes:
- Introduces a new
tests/fixture/stacktracepackage withNew,Errorf, andWrapfunctions - Replaces standard error functions (
fmt.Errorf,errors.New,errreturns) with stacktrace equivalents throughout tmpnet - Updates documentation to explain the stacktrace feature
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/fixture/stacktrace/stacktrace.go | New stacktrace package implementing error wrapping with stack traces |
| tests/fixture/tmpnet/README.md | Documents the new stack trace error feature |
| Multiple tmpnet files | Replaces error handling with stacktrace equivalents |
Comments suppressed due to low confidence (1)
tests/fixture/tmpnet/start_kind_cluster.go:1
- [nitpick] This TODO comment is unrelated to the stacktrace changes and appears to have been accidentally included in the diff. Consider removing it or addressing it in a separate PR.
// Copyright (C) 2019-2025, Ava Labs, Inc. All rights reserved.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Setting STACK_TRACE_ERROR=1 will configure tmpnet to include stacktraces with errors it originates. This is intended to aid in debugging by indicating not just the location of the failure but also chain of callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapping the error in stacktrace is nice 👍 but don't you sort of get the same result if you fmt the error before returning it? ex.
return nil, stacktrace.Wrap(err)vs
return nil, fmt.Errorf("failed to do something %w", err)
It's is similar, but you just get a long error string and have to reconstruct the call stack by searching for error messages (e.g |
| var stackTraceErrors bool | ||
|
|
||
| func init() { | ||
| if os.Getenv("STACK_TRACE_ERRORS") == "1" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if tmpnet has other env vars it supports, but should we prefix them with something like TMPNET_ or TMPNET_DEBUG_?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally avoided prefixing with TMPNET because it's not a tmpnet-specific thing. tmpnet is only the first adopter, but there's no reason for this library to be restricted to it (hence not putting it under tests/fixture/tmpnet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I think this env var is ideal, just that I think it's good enough for now.
| } | ||
| } | ||
|
|
||
| return StackTraceError{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to export this type? It seems like we only use this type as the error interface downstream anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to err on the side of exporting when it comes to library functionality in support of testing to minimize friction with downstream repos like coreth and subnet-evm.
Co-authored-by: rodrigo <77309055+RodrigoVillar@users.noreply.github.com> Signed-off-by: maru <maru.newby@avalabs.org>
…4262) Signed-off-by: maru <maru.newby@avalabs.org> Co-authored-by: rodrigo <77309055+RodrigoVillar@users.noreply.github.com>
Why this should be merged
Setting
STACK_TRACE_ERRORS=1will configure tmpnet to include stack traces with errors it originates. This is intended to aid in debugging by indicating not just the location of the failure but also the chain of callers.Inspired by: golang/go#63358
How this works
How this was tested
Need to be documented in RELEASES.md?
N/A