Skip to content

Fix the filter order in starter #766

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.

Sign up for GitHub

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

Merged
dhaval24 merged 5 commits into from
Nov 2, 2018
Merged

Fix the filter order in starter #766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dee705d
fixing the filter order in starter
dhaval24 Oct 25, 2018
64fc213
trying to add smoke test for 500 capturing
dhaval24 Oct 28, 2018
41d03cf
some more tests
dhaval24 Oct 28, 2018
d8c8fe3
add autoconfigure before for Azure metrics springboot starter and rev…
dhaval24 Nov 2, 2018
cfc0c1b
Polish
dhaval24 Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azure-application-insights-spring-boot-starter/gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
spring.boot.starter.version-number=1.1.0-BETA
spring.boot.starter.version-number=1.1.1-BETA
3 changes: 2 additions & 1 deletion ...soft/applicationinsights/autoconfigure/ApplicationInsightsTelemetryAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@
ApplicationInsightsWebModuleConfiguration.class
})
@AutoConfigureBefore(name = {
"io.micrometer.spring.autoconfigure.export.azuremonitor.AzureMonitorMetricsExportAutoConfiguration"
"io.micrometer.spring.autoconfigure.export.azuremonitor.AzureMonitorMetricsExportAutoConfiguration",
"com.microsoft.azure.spring.autoconfigure.metrics.AzureMonitorMetricsExportAutoConfiguration"
})
public class ApplicationInsightsTelemetryAutoConfiguration {

Expand Down
2 changes: 1 addition & 1 deletion ...crosoft/applicationinsights/autoconfigure/ApplicationInsightsWebMvcAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

@dhaval24 dhaval24 Oct 25, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@grlima grlima Oct 25, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@dhaval24 dhaval24 Oct 25, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@littleaj littleaj Oct 25, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dhaval24 dhaval24 Oct 25, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@gavlyukovskiy gavlyukovskiy Oct 25, 2018

Choose a reason for hiding this comment

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

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.

return registration;
}

Expand Down
5 changes: 5 additions & 0 deletions ...pps/SpringBootTest/src/main/java/com/springbootstartertest/controller/TestController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,9 @@ public String trackEventSpringBoot() {
client.trackEvent("EventDataPropertyTest", properties, metrics);
return "hello";
}

@GetMapping("/throwsException")
public void resultCodeTest() throws Exception {
throw new Exception("This is an exception");
}
}
1 change: 1 addition & 0 deletions test/smoke/testApps/SpringBootTest/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ azure.application-insights.channel.in-process.endpoint-address=http://fakeingest
azure.application-insights.instrumentation-key=00000000-0000-0000-0000-cba987654321
azure.application-insights.logger.level=TRACE
azure.application-insights.default-modules.ProcessPerformanceCountersModule.enabled=false
azure.application-insights.heart-beat.enabled=false
13 changes: 13 additions & 0 deletions ...gBootTest/src/smokeTest/java/com/springbootstartertest/smoketest/SpringbootSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
import static org.junit.Assert.assertEquals;

import com.microsoft.applicationinsights.internal.schemav2.EventData;
import com.microsoft.applicationinsights.internal.schemav2.RequestData;
import com.microsoft.applicationinsights.smoketest.AiSmokeTest;
import com.microsoft.applicationinsights.smoketest.TargetUri;
import com.microsoft.applicationinsights.telemetry.Duration;
import com.microsoft.localforwarder.library.inputs.contracts.Request;
import org.junit.Test;

public class SpringbootSmokeTest extends AiSmokeTest{
Expand Down Expand Up @@ -34,4 +37,14 @@ public void trackEvent() throws Exception {
assertEquals(expectedProperties, d2.getProperties().get("key"));
assertEquals(expectedMetric, d2.getMeasurements().get("key"));
}

@Test
@TargetUri("/throwsException")
public void testResultCodeWhenRestControllerThrows() throws Exception {
assertEquals(1, mockedIngestion.getCountForType("RequestData"));
RequestData d = getTelemetryDataForType(0, "RequestData");
final String expectedResponseCode = "500";
assertEquals(expectedResponseCode, d.getResponseCode());
assertEquals(false, d.getSuccess());
}
}