opentracing-contrib / java-spring-web

OpenTracing Spring Web instrumentation
Apache License 2.0
107 stars 59 forks source link

SkipPattern for Management Endpoints doesn't work with context-path #113

Closed ancore closed 5 years ago

ancore commented 5 years ago

The skip pattern for actuator endpoints contains the context-path, but the TracingFilter tries to match the skip pattern without context-path.

The following is added to the configured default skip pattern: /user/api/management/(health|health/.*|info|info/.*)

The method TracingFilter#isTraced removes the context-path from the request URI: httpServletRequest.getRequestURI().substring(httpServletRequest.getContextPath().length())

Maybe I get something wrong, but I suspect that this filter never works when Spring has a context-path configured.

pavolloffay commented 5 years ago

cc) @larchanjo @ask4gilles could you please have a look. You worked on the actuator part.

Does the context path affect the actuator endpoints?

I think we should remove the context path from the actuator skip pattern and add e2e test with configured context-path.

Would you be interested in submitting a PR?

ancore commented 5 years ago

I'm trying to understand the unit tests and found this while debugging SkipEndPointsWithContextPathAndBasePathTest#testTraceHealthCareEndpoint:

istTraced

ancore commented 5 years ago

The TestRestTemplate expands the URL automatically by context-path (RestTemplate#execute). When you prefix your URL to call for tests with the context-path private static final String HEALTHCARE = CONTEXT_PATH + "/healthcare"; it will be there twice and therefore can be easily removed once and still match the pattern.

I think, the unit test is wrong and hides the actual error I mentioned in my initial post.

I would not like to submit a PR because it affects a lot of unit test methods and I can't estimate the impact.

ask4gilles commented 5 years ago

@pavolloffay @ancore I'll have a look at this tomorrow.

ask4gilles commented 5 years ago

@ancore In order to gain time, can you please provide a minimal project sample to reproduce your issue? Thanks!

ancore commented 5 years ago

You will see the effect when you remove CONTEXT_PATH + from private static final String HEALTHCARE = CONTEXT_PATH + "/healthcare"

ask4gilles commented 5 years ago

@ancore What's "the effect"? If I removed it, I still see it traced, which is normal. What's your issue exactly? An actuator endpoint traced which should not or the other way around? When you say:

The following is added to the configured default skip pattern: /user/api/management/(health|health/.|info|info/.)

Did you add this configuration yourself or is it the result that you see? It would be easier to have a simple example with your configuration. Thanks

ancore commented 5 years ago

Sorry, it is not happening in HEALTHCARE as I mentioned before.

Module: io.opentracing.contrib:opentracing-spring-web-starter:2.1.3-SNAPSHOT Class: SkipEndPointsWithContextPathAndBasePathTest

Let's focus one one test:

private static final String INFO_ACTUATOR = CONTEXT_PATH + ACTUATOR_PATH + INFO_PATH;

@Test
public void testSkipInfoEndpoint() {
   invokeEndpoint(INFO_ACTUATOR);
   assertNoSpans();
}

Debugging: Breakpoint at line 271 in io.opentracing.contrib.web.servlet.filter.TracingFilter

Inspection of httpServletRequest.getRequestURI shows /myapp two times. When using a TestRestTemplate, the configured context-path is added automatically (RestTemplate line 669). We must not add it to the test URL (INFO_ACTUATOR).

So, adding CONTEXT_PATH to the test URLs is wrong in these unit tests. The test fails (rightly so) if you setup the test URL correctly:

private static final String INFO_ACTUATOR = ACTUATOR_PATH + INFO_PATH;

The following happens:

The problem is the skip pattern, it should not contain the context-path.

ask4gilles commented 5 years ago

@ancore Thanks, makes sense. See attached PR.