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

feat: add panic propagation option to gin middleware #1314

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

miguelb
Copy link
Contributor

@miguelb miguelb commented Sep 15, 2022

If gin middleware catches a panic it bypasses other middleware panic handlers. This is an issue in our use case because we have middleware which sends panics to our notification/alert system. The changes in this PR is modeled from other modules which support panicPropagation.

@cla-checker-service
Copy link

cla-checker-service bot commented Sep 15, 2022

💚 CLA has been signed

@apmmachine
Copy link
Contributor

apmmachine commented Sep 15, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-19T13:54:25.576+0000

  • Duration: 59 min 32 sec

Test stats 🧪

Test Results
Failed 0
Passed 8547
Skipped 201
Total 8748

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@miguelb
Copy link
Contributor Author

miguelb commented Sep 15, 2022

I did sign the contribution guidelines. Does this take some time before it gets picked up?

@axw
Copy link
Member

axw commented Sep 19, 2022

@miguelb thanks for opening the PR!

It looks like your commit is not associated with the email you used to sign the CLA. I think what you need to do is configure user.email in git, recreate the commit, and force-push.

@miguelb miguelb force-pushed the apmgin_panic_propagation branch from 5405000 to a37954b Compare September 19, 2022 13:08
@miguelb
Copy link
Contributor Author

miguelb commented Sep 19, 2022

/test

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Nice, thanks :)

@axw
Copy link
Member

axw commented Sep 19, 2022

/test

@axw axw enabled auto-merge (squash) September 19, 2022 13:54
@apmmachine
Copy link
Contributor

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (59/59) 💚
Files 99.346% (152/153) 👍
Classes 96.275% (336/349) 👍
Methods 90.216% (959/1063) 👍
Lines 82.149% (11164/13590) 👎 -0.021
Conditionals 100.0% (0/0) 💚

@axw
Copy link
Member

axw commented Sep 20, 2022

run elasticsearch-ci/docs

@axw axw merged commit 623b328 into elastic:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants