Fix the filter order in starter #766

dhaval24 · 2018-10-25T17:09:48Z

This fixes the order of Filter injection using SpringBoot starter. It allows to capture the correct response code for failed requests.

It turns out that : Ordered.HIGHEST_PRECEDENCE = Integer.MIN_VALUE and we were adding 10 to it which reduced the priority eventually ;)

@grlima @littleaj could you please review this change. It is small and there is request to make hot fix for this.

Deployed the changes in the test app and here is the distributed transaction.

dhaval24 · 2018-10-25T17:14:24Z

.../microsoft/applicationinsights/autoconfigure/ApplicationInsightsWebMvcAutoConfiguration.java

@@ -69,7 +69,7 @@ public FilterRegistrationBean webRequestTrackingFilterRegistrationBean(WebReques
        FilterRegistrationBean registration = new FilterRegistrationBean();
        registration.setFilter(webRequestTrackingFilter);
        registration.addUrlPatterns("/*");
-        registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 10);
+        registration.setOrder(Ordered.HIGHEST_PRECEDENCE);


Ordered.HIGHEST_PRECEDENCE = Integer.MIN_VALUE. Adding 10 to it eventually reduced our filter priority :(

Does having Highest Precedence mess up other scenarios? It seems @gavlyukovskiy had concerns with doing this when he first introduced the starter in #518
Specifically, he mentioned it could prevent users or the framework itself from running code before ours. Is it a problem? Can we double check?

@grlima For reference : https://www.tuturself.com/posts/view?menuId=3&postId=1141
If any other filter runs before our Filter, it would just not enable us to capture the right timing information, the correct response code etc. It is critical to have our filter as the first filter to get the correct information. For example - this filter was running after our Filter : https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/OncePerRequestFilter.java
after which the response code in the response object changed.

It may be ok to order it as highest as long as it can be changed by the user. @dhaval24 , is this possible?

@littleaj currently there is no such mechanism in starter, but it would be fairly easy to bring this if needed. However, in case of a conflict it is upto the Spring to decide which order it wants to execute the filters. Also there are multiple filters in Spring with highest precedence. It is fine for them to execute in relative order among themselves.

The important point here is that our filter executes above any other filter whose priority is less that HIGHEST_ORDER. @grlima this should also explain your concern.

Initially I did it to let user the ability to put something before this filter. For instance we had logging filter that we needed to run earlier whatever happens in other libraries.
I think +2 would be okay, at least users have some room to breathe.

littleaj · 2018-10-25T18:02:04Z

@dhaval24, have you tested this with spring error/exception handlers?

dhaval24 · 2018-10-25T18:16:48Z

@dhaval24, have you tested this with spring error/exception handlers?

@littleaj can you elaborate a bit on this.

dhaval24 · 2018-10-25T18:29:43Z

@grlima and @littleaj ok, I did some more research: https://docs.spring.io/spring-boot/docs/current-SNAPSHOT/reference/htmlsingle/#boot-features-embedded-container-servlets-filters-listeners

It says to avoid HIGHEST_PRECEDENCE. But I think +10 is making it too late in the chain. May +2 so that it just comes after the metrics filter should be okay.

dhaval24 · 2018-10-25T20:31:35Z

Spring’s WebMVC Metrics filter : https://github.com/spring-projects/spring-boot/blob/5a4220c77383741205203b615d40de82b890e153/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java#L166 is responsible to set the status code for the request ultimately.

The priority of this filter is : Ordered.HIGHEST_PRECEDENCE + 1
Based on SpringBoot docs here: https://docs.spring.io/spring-boot/docs/current-SNAPSHOT/reference/htmlsingle/#boot-features-embedded-container-servlets-filters-listeners

Therefore, if we would not have the Ordered.HIGHEST_PRECEDENCE, it would be impossible to capture the correct response code from the Filter Based Method we have today. We need to be first in the filter chain in one way or the other.

dhaval24 · 2018-10-25T21:32:48Z

Spring’s WebMVC Metrics filter : https://github.com/spring-projects/spring-boot/blob/5a4220c77383741205203b615d40de82b890e153/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilter.java#L166 is responsible to set the status code for the request ultimately.

The priority of this filter is : Ordered.HIGHEST_PRECEDENCE + 1
Based on SpringBoot docs here: https://docs.spring.io/spring-boot/docs/current-SNAPSHOT/reference/htmlsingle/#boot-features-embedded-container-servlets-filters-listeners

Therefore, if we would not have the Ordered.HIGHEST_PRECEDENCE, it would be impossible to capture the correct response code from the Filter Based Method we have today. We need to be first in the filter chain in one way or the other.

@gavlyukovskiy please consider the above!! and let me know your thoughts

littleaj · 2018-10-25T21:37:41Z

@dhaval24 ErrorPageFilter can also set the status

dhaval24 · 2018-10-25T21:39:36Z

@dhaval24 ErrorPageFilter can also set the status

Sure @littleaj that makes sense. It's precedence is also Ordered.HIGHEST_PRECEDENCE + 1 (as from SpringBoot docs). Therefore I think we would need to have the filter at HIGHEST_PRECEDENCE if we want to stick with the filter based solution.

littleaj · 2018-10-26T18:04:41Z

@dhaval24 regarding error handlers, in j2ee you can specify error pages to handle certain exceptions. These pages can change the result code sent back to the user (which don't make it back to the filter). I was wondering if Spring has the same issue. Spring lets you define error handlers which do the same thing.

littleaj · 2018-10-26T18:08:23Z

@dhaval24 What happens if you define more filters with HIGHEST_PRECEDENCE? Is the order determinate by some other factor?

gavlyukovskiy · 2018-10-29T05:11:12Z

@littleaj order of beans with order value is non deterministic, though there is interface PriorityOrdered which is used in exceptional cases and those beans have higher precedence over the beans that don't implement it.

littleaj

Looks good. Just need a passing build.

dhaval24 · 2018-10-29T23:22:38Z

Thanks @littleaj , I queued a new build.

dhaval24 · 2018-10-29T23:37:08Z

https://dev.azure.com/mseng/AppInsights/_build/results?buildId=7619743&view=logs
The build has passed.

…ert the version number update for core sdk

dhaval24 · 2018-11-02T00:42:29Z

@grlima and @littleaj I also verified the correlation and it works good with this change too.

Here is a node app communicating with java app. I think this change is good to go.

fixing the filter order in starter

dee705d

dhaval24 requested review from littleaj and grlima October 25, 2018 17:09

dhaval24 commented Oct 25, 2018

View reviewed changes

dhaval24 added 2 commits October 27, 2018 20:35

trying to add smoke test for 500 capturing

64fc213

some more tests

41d03cf

littleaj reviewed Oct 29, 2018

View reviewed changes

add autoconfigure before for Azure metrics springboot starter and rev…

d8c8fe3

…ert the version number update for core sdk

Polish

cfc0c1b

grlima approved these changes Nov 2, 2018

View reviewed changes

littleaj approved these changes Nov 2, 2018

View reviewed changes

dhaval24 merged commit f01a266 into master Nov 2, 2018

dhaval24 deleted the Starter/FixFilterOrder branch November 9, 2018 20:09

dhaval24 mentioned this pull request Jan 13, 2019

add support for opencensus http servlet plugin in spring cloud framework census-instrumentation/opencensus-java#1650

Merged

dhaval24 mentioned this pull request Jan 30, 2019

Application Insights showing 200 for failed requests #810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the filter order in starter #766

Fix the filter order in starter #766

dhaval24 commented Oct 25, 2018

dhaval24 Oct 25, 2018

grlima Oct 25, 2018

dhaval24 Oct 25, 2018

littleaj Oct 25, 2018

dhaval24 Oct 25, 2018

gavlyukovskiy Oct 25, 2018

littleaj commented Oct 25, 2018 •

edited

Loading

dhaval24 commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

littleaj commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

littleaj commented Oct 26, 2018

littleaj commented Oct 26, 2018

gavlyukovskiy commented Oct 29, 2018

littleaj left a comment

dhaval24 commented Oct 29, 2018

dhaval24 commented Oct 29, 2018 •

edited

Loading

dhaval24 commented Nov 2, 2018

Fix the filter order in starter #766

Fix the filter order in starter #766

Conversation

dhaval24 commented Oct 25, 2018

dhaval24 Oct 25, 2018

Choose a reason for hiding this comment

grlima Oct 25, 2018

Choose a reason for hiding this comment

dhaval24 Oct 25, 2018

Choose a reason for hiding this comment

littleaj Oct 25, 2018

Choose a reason for hiding this comment

dhaval24 Oct 25, 2018

Choose a reason for hiding this comment

gavlyukovskiy Oct 25, 2018

Choose a reason for hiding this comment

littleaj commented Oct 25, 2018 • edited Loading

dhaval24 commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

littleaj commented Oct 25, 2018

dhaval24 commented Oct 25, 2018

littleaj commented Oct 26, 2018

littleaj commented Oct 26, 2018

gavlyukovskiy commented Oct 29, 2018

littleaj left a comment

Choose a reason for hiding this comment

dhaval24 commented Oct 29, 2018

dhaval24 commented Oct 29, 2018 • edited Loading

dhaval24 commented Nov 2, 2018

littleaj commented Oct 25, 2018 •

edited

Loading

dhaval24 commented Oct 29, 2018 •

edited

Loading