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

Error handling of Servlet Async API #399

Merged
merged 4 commits into from
Jan 7, 2019

Conversation

eyalkoren
Copy link
Contributor

Status code and error reporting
Pool ApmAsynListener objects
Extend integration tests to capturing errors and JSP transactions

@eyalkoren eyalkoren force-pushed the async-servlet-error-handling branch from 379e09e to 81dff7a Compare December 24, 2018 12:31
@eyalkoren eyalkoren self-assigned this Dec 24, 2018
@eyalkoren
Copy link
Contributor Author

Jenkins test this please

@eyalkoren eyalkoren force-pushed the async-servlet-error-handling branch 2 times, most recently from 241c217 to cfda77a Compare December 25, 2018 08:55
@codecov-io
Copy link

codecov-io commented Dec 25, 2018

Codecov Report

Merging #399 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #399      +/-   ##
============================================
- Coverage     72.26%   72.24%   -0.02%     
+ Complexity     1339     1315      -24     
============================================
  Files           145      143       -2     
  Lines          5257     5063     -194     
  Branches        550      518      -32     
============================================
- Hits           3799     3658     -141     
+ Misses         1222     1169      -53     
  Partials        236      236
Impacted Files Coverage Δ Complexity Δ
...lastic/apm/agent/servlet/AsyncInstrumentation.java 68.57% <ø> (ø) 3 <0> (ø) ⬇️
...tic/apm/agent/servlet/helper/ApmAsyncListener.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...t/servlet/helper/AsyncContextAdviceHelperImpl.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...astic/apm/agent/impl/transaction/TraceContext.java 92.21% <0%> (-0.52%) 56% <0%> (-11%)
...java/co/elastic/apm/agent/bci/ElasticApmAgent.java 73.04% <0%> (-0.33%) 22% <0%> (-2%)
...webmvc/DispatcherServletRenderInstrumentation.java
...bci/bytebuddy/FailSafeDeclaredMethodsCompiler.java
.../agent/plugin/api/AbstractSpanInstrumentation.java 53.19% <0%> (+1.15%) 3% <0%> (-3%) ⬇️
...agent/plugin/api/ElasticApmApiInstrumentation.java 52% <0%> (+12%) 3% <0%> (ø) ⬇️

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 f1611f9...8c6bc1e. Read the comment docs.


int maxPooledElements = tracer.getConfigurationRegistry().getConfig(ReporterConfiguration.class).getMaxQueueSize() * 2;
asyncListenerObjectPool = QueueBasedObjectPool.ofRecyclable(
AtomicQueueFactory.<ApmAsyncListener>newQueue(createBoundedMpmc(maxPooledElements)),
Copy link
Member

Choose a reason for hiding this comment

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

The number of required ApmAsyncListener instances is independent of the size of the disruptor. It's probably dependant on the number of container threads. The default for Tomcat is 200 IIRC. I'm not sure whether there is an extra thread pool for the handling of async requests but I suppose there is not.

Maybe hard-code to 256?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right it is not the right configuration to rely on. We can start with hard code and see if it bites us.

@eyalkoren eyalkoren force-pushed the async-servlet-error-handling branch from 2143199 to 8c6bc1e Compare January 7, 2019 16:07
@eyalkoren eyalkoren merged commit 0238613 into elastic:master Jan 7, 2019
@eyalkoren eyalkoren deleted the async-servlet-error-handling branch January 9, 2019 09:56
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.

4 participants