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

module/...: fix panic handling in web instrumentations #273

Merged
merged 2 commits into from
Oct 24, 2018

Conversation

axw
Copy link
Member

@axw axw commented Oct 24, 2018

Introduce a basic HTTP test suite for each of
the web framework instrumentation modules to
run, to test functionality that is expected
to be common across each of them.

For now there are just a few tests for some
edge cases:

  • handler does not explicitly write a response
  • handler panics before writing a response
  • handler panics after writing a response

This uncovered a handful of bugs in the various
web instrumentation modules which have now
been fixed.

We exposed apmhttp.setContext as SetContext,
using it in apmrestful to set context. This allows us
to set the framework for errors originating from
go-restful handlers.

Fixes #272

Introduce a basic HTTP test suite for each of
the web framework instrumentation modules to
run, to test functionality that is expected
to be common across each of them.

For now there are just a few tests for some
edge cases:

 - handler does not explicitly write a response
 - handler panics before writing a response
 - handler panics after writing a response
@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #273 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   81.18%   81.46%   +0.28%     
==========================================
  Files          98       99       +1     
  Lines        5622     5692      +70     
==========================================
+ Hits         4564     4637      +73     
+ Misses        837      833       -4     
- Partials      221      222       +1
Impacted Files Coverage Δ
module/apmhttp/handler.go 76.27% <100%> (+6.27%) ⬆️
module/apmhttprouter/handler.go 62.5% <100%> (+4.16%) ⬆️
transport/transporttest/recorder.go 81.25% <100%> (+1.25%) ⬆️
module/apmecho/middleware.go 85.48% <100%> (+0.23%) ⬆️
apmtest/httpsuite.go 100% <100%> (ø)
module/apmrestful/filter.go 88.88% <100%> (+1.08%) ⬆️
module/apmgin/middleware.go 86.3% <100%> (ø) ⬆️
module/apmhttp/recovery.go 86.66% <100%> (ø) ⬆️
tracer.go 83.78% <0%> (-0.46%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b426f8...cbfe017. Read the comment docs.

Update all of the web framework instrumentation modules
to run the new apmtest.HTTPTestSuite, and fix bugs that
fell out.

Expose apmhttp.setContext as SetContext, using it in
apmrestful to set context. This allows us to set the
framework for errors originating from go-restful handlers.
@axw axw merged commit bf46820 into elastic:master Oct 24, 2018
@axw axw deleted the apmhttp-panic-response branch October 24, 2018 02:44
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.

apmhttp: response behaviour of panicking handlers is changed
2 participants