Skip to content

Print panic output on Error log level #292

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

Merged
merged 4 commits into from
Mar 4, 2025
Merged

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Nov 10, 2023

First of all, thanks for creating this awesome library! We at Mattermost use it a lot.

If a plugin panics, the panic message goes to stderr of the plugin. Because the log lines don't have any levelled prefix like [ERROR], the panic message would end up in the Debug level. That makes it hard to spot, why a plugin panicked.

The (hacky) solution is to check if any unparsed lines start with panic:. If that is the case, this line and all unparsed following lines go to the Error level. Most likely, these will be the last log lines anyway before the process dies.

I tried adding a test to cover, but that would require spinning up another process (that then panics) to cover this PR.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 10, 2023

CLA assistant check
All committers have signed the CLA.

@hanzei
Copy link
Contributor Author

hanzei commented Dec 6, 2023

@tomhjp Would you mind giving this PR a review?

@hanzei
Copy link
Contributor Author

hanzei commented Sep 7, 2024

Anything I can help with to get a review going?

@streamer45
Copy link

@VioletHynes Would your team have some bandwidth to review this one?

@VioletHynes
Copy link
Contributor

Hi there -- this isn't my team's area but I'll try and forward this to the team that owns this. Thanks :)

@streamer45
Copy link

Thanks, appreciate it!

Co-authored-by: Robert <17119716+robmonte@users.noreply.github.com>
@hanzei
Copy link
Contributor Author

hanzei commented Dec 17, 2024

@robmonte Gentle reminder to review the PR 😄

@hanzei
Copy link
Contributor Author

hanzei commented Feb 18, 2025

@robmonte Just wanted to check the process on the PR. Is there anything I can do to speed up the process?

@robmonte
Copy link
Member

robmonte commented Mar 1, 2025

@hanzei Sorry for the delay since my last comment. At first a concern with this change was that, as you said, it's a little hacky to be based on string prefixes. An alternate solution pull request was opened that you can see here: #319

Providing a PanicHandler might be a "more correct" solution long term and so we were looking into that a bit, however given the widespread use of go-plugin it definitely makes us more cautious with changes. I'm not familiar enough with gRPC as a whole to give that PR a proper review despite it seeming straightforward.

However since the entire log level checker is already based on that same string logic, we think it makes sense to go ahead with this smaller, simpler change for now.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Approving the PR after reviewing the changes.

@ghost ghost mentioned this pull request Mar 4, 2025
@robmonte robmonte merged commit a8be8f5 into hashicorp:main Mar 4, 2025
1 check passed
@hanzei hanzei deleted the print-panic branch March 26, 2025 10:31
@hanzei
Copy link
Contributor Author

hanzei commented Mar 26, 2025

Awesome, thanks for merging the PR! Looking forward to the next release 🚀

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.

5 participants