-
Notifications
You must be signed in to change notification settings - Fork 44
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
Annotate panics with provider metadata and origin #2871
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2871 +/- ##
==========================================
+ Coverage 67.50% 67.73% +0.22%
==========================================
Files 327 328 +1
Lines 41743 42088 +345
==========================================
+ Hits 28180 28508 +328
- Misses 11977 11992 +15
- Partials 1586 1588 +2 ☔ View full report in Codecov by Sentry. |
panic_recovering_provider.go has actually 100% coverage. I don't know what to make of the codecov report. |
The codecov report is partial, since only some of the tests have run due to the CI failure |
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.
LGTM. Do we need to add the interceptor to the muxer in case it panics?
That's a good idea. In case we are building a muxed provider it's better to install muxer once at the outer layer instead of installing it in the inner layers. I went on a few tangents here trying to reconcile the problem of a missing logging sink but unfortunately it doesn't seem like my experiments are converging. Let me try to think it over and maybe relax the code's insistence on a non-nil logging sink.. |
crosstestsimpl "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/internal/tests/cross-tests/impl" | ||
) | ||
|
||
type T = crosstestsimpl.T | ||
|
||
type testLogSink struct{ t T } |
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.
Moved this code to make it more available.
Installed in muxer too, PTAL. |
Recover panics in the bridged providers and emit them as engine error events with sufficient annotations about which provider, provider version, resource and method was responsible for the panic.
Fixes #2844
While the bridged providers should never panic, some WIP semantic discrepancy between TF and Pulumi still occasionally results in panics. These changes accomplish two purposes. First, it is easier for maintainers to immediately narrow down a panic to a resource or function involved to attempt a repro. Second, it is easier to scan for panics in internal logs and attribute them to providers unambiguously.
With these changes, an injected panic in pulumi-random looks as follows. It is unfortunately double-stated but the new error message correctly attributes it to the provider, resource and method: