Create spans for forward/include by videnkz · Pull Request #557 · elastic/apm-agent-java

videnkz · 2019-04-02T17:46:22Z

closes #384

…transaction

codecov-io · 2019-04-03T12:10:48Z

Codecov Report

Merging #557 into master will decrease coverage by 0.67%.
The diff coverage is 32.5%.

@@             Coverage Diff              @@
##             master     #557      +/-   ##
============================================
- Coverage      63.7%   63.03%   -0.68%     
  Complexity       68       68              
============================================
  Files           189      182       -7     
  Lines          7431     6836     -595     
  Branches        871      786      -85     
============================================
- Hits           4734     4309     -425     
+ Misses         2433     2264     -169     
+ Partials        264      263       -1

Impacted Files	Coverage Δ	Complexity Δ
...ic/apm/agent/servlet/ServletTransactionHelper.java	`83.73% <100%> (+7.51%)`	`0 <0> (ø)`	⬇️
...orwardIncludeRequestDispatcherInstrumentation.java	`25% <25%> (ø)`	`0 <0> (?)`
...gent/servlet/RequestDispatcherInstrumentation.java	`45.45% <45.45%> (ø)`	`0 <0> (?)`
...m/agent/impl/async/SpanInScopeCallableWrapper.java	`62.96% <0%> (-37.04%)`	`0% <0%> (ø)`
...m/agent/impl/async/SpanInScopeRunnableWrapper.java	`64.28% <0%> (-35.72%)`	`0% <0%> (ø)`
.../agent/report/processor/ProcessorEventHandler.java	`44.44% <0%> (-19.2%)`	`0% <0%> (ø)`
...n/java/co/elastic/apm/agent/web/BodyProcessor.java	`69.23% <0%> (-14.11%)`	`0% <0%> (ø)`
...lastic/apm/agent/impl/transaction/Transaction.java	`73.77% <0%> (-11.23%)`	`0% <0%> (ø)`
...bci/methodmatching/TraceMethodInstrumentation.java	`59.57% <0%> (-5.37%)`	`0% <0%> (ø)`
...tic/apm/agent/configuration/CoreConfiguration.java	`96.12% <0%> (-3.88%)`	`0% <0%> (ø)`
... and 51 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 17f6985...0c6d70c. Read the comment docs.

felixbarny

I don't see how this is related to #429 I think this tackles #384, right?

We should also add a test case to the integration tests to be sure that it works on all supported servers.

...vlet-plugin/src/main/java/co/elastic/apm/agent/servlet/RequestDispatcherInstrumentation.java

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

...ent-plugins/apm-servlet-plugin/src/test/java/co/elastic/apm/agent/servlet/ApmFilterTest.java

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

...vlet-plugin/src/main/java/co/elastic/apm/agent/servlet/RequestDispatcherInstrumentation.java

...s/application-server-integration-tests/src/test/java/co/elastic/apm/servlet/WebSphereIT.java

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

videnkz · 2019-05-01T19:03:24Z

@felixbarny please check pull request, i deleted instrumentation on RequestDispatcher.
And added integration tests on forward, include.

felixbarny · 2019-05-02T14:12:44Z

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

+                String pathInfo = "";
+
+                if (FORWARD.equalsIgnoreCase(method)) {
+                    servletPath = (String) httpServletRequest.getAttribute(RequestDispatcher.FORWARD_SERVLET_PATH);


Didn't you say these properties are not set yet? Or are they only set on the real application servers but were not on the unit tests?

I tested with Apache Tomcat Version 8.5.5, and those properties were not there.
They were not on the unit tests, too.
I don't know, if I should instrument RequestDispatcher .

I think there was a confusion here- the attribute RequestDispatcher.FORWARD_SERVLET_PATH provides the servlet path of the original request in the new request created in the new forwarded/included request, but here we are interested in the new request paths. So maybe we can create and activate the span here but set it only when the next service will run simply through HttpServletRequest#getServletPath and HttpServletRequest#getPathInfo.

...n-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java

felixbarny · 2019-05-03T07:08:04Z

Some ideas about alternatives which enable the capturing of the path:

Just instrument javax.servlet.ServletRequest#getRequestDispatcher and return a RequestDispatcherWrapper. Similar to RequestStreamRecordingInstrumentation.
- Pro: easy to implement
- Con: Allocates memory, does not capture ERROR requests
Enhance ServletApiAdvice to create spans for DispatcherTypes FORWARD, INCLUDE and potentially ERROR
- Pro: we can simply read the path from the request.getServletPath() and request.getPathInfo()
- Con: Makes the complex ServletApiAdvice even more complex

Another option is to just omit the path (the current state of the PR).

@kananindzya @eyalkoren WDYT?

eyalkoren

Thanks for your great contribution!
A few comments, mainly about the span naming that seems to include the original path, but we probably prefer using the forwarded/included servlet path and path info.
Also, some notes about start/end and activate/deactivate maintenance.

Without regard, I know you already know our code quite well, but did you see https://www.elastic.co/blog/a-cookbook-for-contributing-a-plugin-to-the-elastic-apm-java-agent? Maybe this can be informative even for you 🙂.

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

eyalkoren · 2019-05-05T13:37:22Z

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

+                if (parent instanceof AbstractSpan) {
+                    AbstractSpan parentSpan = (AbstractSpan) parent;
+                    if (parentSpan.getName().toString().equals(span.getName().toString())) {
+                        return;


This is too late- the span is already created and started, just not activated.
This check needs to be done at the beginning of the method, before span creation and start. Then you will have to avoid object allocation for the string creation, so you could use a ThreadLocal StringBuilder for example, or check whether parent span's name contains the pathInfo and servletPath.
@felixbarny do you think scenarios of multiple forwards/includes for same path and servlet are a valid concern?

Good question. I don't really know. Theoretically, that could happen, although not very likely.

eyalkoren · 2019-05-05T13:49:37Z

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

+
+public class ForwardIncludeRequestDispatcherInstrumentation extends ElasticApmInstrumentation {
+
+    private static final String SPAN_TYPE = "servlet";


@felixbarny the type servlet seems right here, but I wonder if it is a good fit in the overall APM context. All the other span main types are such that we try to keep somewhat aligned between agents and with limited cardinality. I would say sub-type is better to use for Java-specific term, but not sure which major span type is a good fit instead. Any idea?

I think app might be a good type. For breakdown graphs, the transaction's self-time will also be attributed as app and it's also the default type for the @CaptureSpan annotation.

eyalkoren · 2019-05-05T13:51:18Z

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

+                    }
+                }
+
+                span.activate();


If the span is started, it should be activated as well

eyalkoren · 2019-05-05T13:59:38Z

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

+                                        @Advice.Thrown @Nullable Throwable t) {
+            if (span != null) {
+                span.captureException(t)
+                    .deactivate()


This may cause deactivation of non-activated span, which should always be checked against and avoided. I think the code of this method should remain, and the proper way to ensure span activation state correctness is making sure it's either null or non-null-and-activated in beforeExecute

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

eyalkoren · 2019-05-05T15:03:01Z

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java

+                String pathInfo = "";
+
+                if (FORWARD.equalsIgnoreCase(method)) {
+                    servletPath = (String) httpServletRequest.getAttribute(RequestDispatcher.FORWARD_SERVLET_PATH);


I think there was a confusion here- the attribute RequestDispatcher.FORWARD_SERVLET_PATH provides the servlet path of the original request in the new request created in the new forwarded/included request, but here we are interested in the new request paths. So maybe we can create and activate the span here but set it only when the next service will run simply through HttpServletRequest#getServletPath and HttpServletRequest#getPathInfo.

...n-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java

eyalkoren · 2019-05-05T15:49:13Z

@felixbarny I just saw your comment now.

What I thought when reviewing was to create the span as it is created now in this PR with the basic name only, but update its name in the service that is ran for the forwarded/included servlet, either by setting the span as a Request attribute and assume the servlet container will copy it to the new request it generates (needs to be tested) or by looking for the active span and if it is the FORWARD/INCLUDE one update its name (would not work if spans are created in between).

However,

Enhance ServletApiAdvice to create spans for DispatcherTypes FORWARD, INCLUDE and potentially ERROR

may actually be a better option - you mean that we rely on the RequestDispather#FORWARD_XXX/INCLUDE_XXX/ERROR_XXX attributes to decide if and which type of span this is?

Con: Makes the complex ServletApiAdvice even more complex

I don't think so. Seems pretty straightforward.

felixbarny · 2019-05-06T07:51:11Z

you mean that we rely on the RequestDispather#FORWARD_XXX/INCLUDE_XXX/ERROR_XXX attributes to decide if and which type of span this is?

No, I meant relying on javax.servlet.ServletRequest#getDispatcherType.

But that can easily become messy - especially when there's a filter chain which may or may not invoke the target servlet and taking into account that there may be multiple forwards/includes.

eyalkoren · 2019-05-06T08:52:12Z

@kananindzya So let's do this:
Instead of instrumenting RequestDispatcher, we will use our existing doFilter and service instrumentation, ie do everything through ServletApiAdvice.

On method enter and exit, add a @Nullabale @Advice.Local("span") Span span.
On method enter, we will rely on javax.servlet.ServletRequest#getDispatcherType to decide if this is a transaction, or a forward/include/error span. If the DispatcherType is FORWARD, INCLUDE or ERROR, AND tracer.currentTransaction()==null, we will start and activate a span, unless the current span has the same servletPath and pathInfo.
On method exit, if the span is not null, deactivate it and end it.

WDYT?

eyalkoren · 2019-08-12T14:00:03Z

@kananindzya we would like to implement that based on the logic described in the last comment.
Is it OK if I close this PR?

kananindzya added 3 commits April 2, 2019 23:40

added instrumentation for include and forward

e824c81

Merge remote-tracking branch 'upstream/master' into task/429-servlet-…

a712659

…transaction

delete plugin

1f3d1e8

felixbarny mentioned this pull request Apr 3, 2019

Task/429 servlet transaction #503

Closed

felixbarny reviewed Apr 3, 2019

View reviewed changes

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java Show resolved Hide resolved

felixbarny reviewed Apr 3, 2019

View reviewed changes

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java Outdated Show resolved Hide resolved

felixbarny changed the title ~~Task/429 servlet transaction~~ Apr 3, 2019

kananindzya added 2 commits April 8, 2019 15:51

added integration tests for forward and include. Corrected comments

9a5a427

added check on existing span with the same name.

f8ce065

videnkz commented Apr 8, 2019

View reviewed changes

...vlet-plugin/src/main/java/co/elastic/apm/agent/servlet/RequestDispatcherInstrumentation.java Outdated Show resolved Hide resolved

...s/application-server-integration-tests/src/test/java/co/elastic/apm/servlet/WebSphereIT.java Outdated Show resolved Hide resolved

videnkz commented Apr 8, 2019

View reviewed changes

...c/main/java/co/elastic/apm/agent/servlet/ForwardIncludeRequestDispatcherInstrumentation.java Outdated Show resolved Hide resolved

alvarolobato added the [zube]: In Review label Apr 8, 2019

kananindzya added 3 commits April 27, 2019 02:16

task-429 local

8374274

lobcal

ed29dc7

fixed tests. Delete request dispatcher instrumentation

0c6d70c

felixbarny reviewed May 2, 2019

View reviewed changes

...n-server-integration-tests/src/test/java/co/elastic/apm/servlet/tests/ServletApiTestApp.java Show resolved Hide resolved

eyalkoren reviewed May 5, 2019

View reviewed changes

eyalkoren mentioned this pull request Aug 12, 2019

Automatically track spans in Servlet RequestDispatcher forwards/includes #384

Closed

eyalkoren closed this Aug 15, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Aug 15, 2019

lreuven removed the [zube]: Done label Nov 18, 2019

videnkz mentioned this pull request Jun 1, 2020

issue-557 added automatically span creation for DispatcherTypes: FORWARD, INCLUDE, ERROR #1212

Merged

6 tasks


		public class ForwardIncludeRequestDispatcherInstrumentation extends ElasticApmInstrumentation {

		private static final String SPAN_TYPE = "servlet";

Conversation

videnkz commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov-io commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

videnkz commented May 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalkoren May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixbarny commented May 3, 2019

eyalkoren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eyalkoren May 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren commented May 5, 2019

felixbarny commented May 6, 2019

eyalkoren commented May 6, 2019

eyalkoren commented Aug 12, 2019

Labels

6 participants

videnkz commented Apr 2, 2019 •

edited

Loading

codecov-io commented Apr 3, 2019 •

edited

Loading

eyalkoren May 5, 2019 •

edited

Loading

eyalkoren May 5, 2019 •

edited

Loading