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.

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

Dhdoshi/spring boot name bug #411

Closed
wants to merge 6 commits into from
Closed

Dhdoshi/spring boot name bug #411

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c51b53a
Adding Parametrized constructor to support extrnal spring boot web ap…
Aug 18, 2017
11516bf
Adding tests to support external name setting for spring boot webapp
Aug 18, 2017
37b6fc8
Updating comments
dhaval24 Aug 24, 2017
16dda62
Merge branch 'master' into dhdoshi/SpringBootNameBug
dhaval24 Aug 29, 2017
cf59793
Adding private method to read from properties file, removing public p…
dhaval24 Aug 30, 2017
544b0ad
Fixed removing public parametrized constructor
dhaval24 Sep 10, 2017
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
60 changes: 37 additions & 23 deletions ...rc/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,29 +21,28 @@

package com.microsoft.applicationinsights.web.internal;

import java.io.IOException;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Date;
import java.util.LinkedList;
import com.microsoft.applicationinsights.TelemetryClient;
import com.microsoft.applicationinsights.TelemetryConfiguration;
import com.microsoft.applicationinsights.agent.internal.coresync.impl.AgentTLS;
import com.microsoft.applicationinsights.common.CommonUtils;
import com.microsoft.applicationinsights.internal.agent.AgentConnector;
import com.microsoft.applicationinsights.internal.logger.InternalLogger;
import com.microsoft.applicationinsights.internal.util.ThreadLocalCleaner;

import javax.servlet.Filter;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import com.microsoft.applicationinsights.common.CommonUtils;
import com.microsoft.applicationinsights.TelemetryClient;
import com.microsoft.applicationinsights.TelemetryConfiguration;
import com.microsoft.applicationinsights.agent.internal.coresync.impl.AgentTLS;
import com.microsoft.applicationinsights.internal.agent.AgentConnector;
import com.microsoft.applicationinsights.internal.logger.InternalLogger;
import com.microsoft.applicationinsights.internal.util.ThreadLocalCleaner;
import java.io.IOException;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Date;
import java.util.LinkedList;

/**
* Created by yonisha on 2/2/2015.
Expand All @@ -59,20 +58,24 @@ public final class WebRequestTrackingFilter implements Filter {
private boolean agentIsUp = false;
private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

//Only for Spring Boot Application (External Name Setting)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to limit use of it for Spring Boot. May one decide to do it in regular app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not quite sure about it, may be I am wrong though. I think other application like Spring/Jsp/Servlet etc they pick up configuration with xml files. However, in Spring boot configuration is done with Java classes and application name is set using application.properties file. Therefore there was a need to explicitly set the name. Correct me if I am wrong here.

Copy link
Member

Choose a reason for hiding this comment

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

  1. As soon as we make new constructor public, we cannot control if it's going to be used for Spring Boot only or not.
  2. There might be another framework we haven't tried yet that required application name setup outside of configuration xml files.

Copy link
Member

Choose a reason for hiding this comment

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

On a side note - is there any implication if name is set in both configuration and the code with this constructor for non-SpringBoot app? Name from the code will take precedence and any mappings created for the previous name will be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dmitry-Matveev I believe that the name from the code will take precedence as the getName() method will return after executing the if statement as soon as it sees the name is set externally.

Copy link
Member

Choose a reason for hiding this comment

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

@dhaval24 , Thank for looking into this! Then the question is - will changing name right in the code affect the behavior of the SDK in some unpredictable way?
For example, if name from the code takes precedence, could it be that some configuration settings are not picked up (simply because they are mapped to the name used in the configuration but not in code)? If no such or similar case is possible - we should be OK with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dmitry-Matveev honestly, I am not quite sure if this can affect the SDK in some unpredictable way. Do you have any ideas how I can check this?

private String applicationName;
Copy link
Contributor

Choose a reason for hiding this comment

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

make it final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure can make that as final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SergeyKanzhelev I just looked into the code and it seems that we cannot make the applicationName final because if we do so it has to be initialized in the constructor itself (either through parameterized constructor or default constructor) and we do not have a way to do it with default constructor because with non spring boot application the name of the application is caught from the servlet context and in our implementation it is retrieved from getName() method which parses the context and is invoked post constructor call. Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

how applicaitonName will be used? Is it send along the telemetry? If it will end up as roleName - worse naming it appropriately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For, this one I have a question too. I think even without setting the name Telemetry flows in the portal. Only it comes in AI logs. I don' know the exact implication of it, in this context

Copy link
Member

Choose a reason for hiding this comment

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

@dhaval24 , please check where this name is used through the code and whether it makes it into the telemetry item at all. If not, keeping it as applicationName is ok.


// endregion Members

// region Public

/**
* Processing the given request and response.
* @param req The servlet request.
* @param res The servlet response.
*
* @param req The servlet request.
* @param res The servlet response.
* @param chain The filters chain
* @throws IOException Exception that can be thrown from invoking the filters chain.
* @throws IOException Exception that can be thrown from invoking the filters chain.
* @throws ServletException Exception that can be thrown from invoking the filters chain.
*/
public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException {
ApplicationInsightsHttpResponseWrapper response = new ApplicationInsightsHttpResponseWrapper((HttpServletResponse)res);
ApplicationInsightsHttpResponseWrapper response = new ApplicationInsightsHttpResponseWrapper((HttpServletResponse) res);
setKeyOnTLS(key);

boolean isRequestProcessedSuccessfully = invokeSafeOnBeginRequest(req, response);
Expand All @@ -81,7 +84,7 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
chain.doFilter(req, response);
invokeSafeOnEndRequest(req, response, isRequestProcessedSuccessfully);
} catch (ServletException se) {
onException(se, req, response,isRequestProcessedSuccessfully);
onException(se, req, response, isRequestProcessedSuccessfully);
throw se;
} catch (IOException ioe) {
onException(ioe, req, response, isRequestProcessedSuccessfully);
Expand Down Expand Up @@ -118,9 +121,10 @@ private void onException(Exception e, ServletRequest req, ServletResponse res, b

/**
* Initializes the filter from the given config.
*
* @param config The filter configuration.
*/
public void init(FilterConfig config){
public void init(FilterConfig config) {
try {
initialize(config);

Expand Down Expand Up @@ -162,7 +166,7 @@ private boolean invokeSafeOnBeginRequest(ServletRequest req, ServletResponse res
boolean success = true;

try {
RequestTelemetryContext context = new RequestTelemetryContext(new Date().getTime(), (HttpServletRequest)req);
RequestTelemetryContext context = new RequestTelemetryContext(new Date().getTime(), (HttpServletRequest) req);
ThreadContext.setRequestTelemetryContext(context);

webModulesContainer.invokeOnBeginRequest(req, res);
Expand Down Expand Up @@ -206,13 +210,17 @@ private void setKeyOnTLS(String key) {
public WebRequestTrackingFilter() {
}

public WebRequestTrackingFilter(String applicationName) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like public constructor is still present - maybe merge issue?

this.applicationName = applicationName;
}

private synchronized void initialize(FilterConfig filterConfig) {
try {

//if agent is not installed (jar not loaded), can skip the entire registration process
try {
AgentConnector test = AgentConnector.INSTANCE;
} catch(Throwable t) {
} catch (Throwable t) {
InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.INFO, "Agent was not found. Skipping the agent registration");
return;
}
Expand Down Expand Up @@ -257,6 +265,12 @@ private String registerWebApp(String name) {
}

private String getName(ServletContext context) {

// If Application is of type Spring Boot
Copy link
Contributor

Choose a reason for hiding this comment

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

again, do not limit it to spring boot only

if (applicationName != null) {
return applicationName;
}

String name = null;
try {
String contextPath = context.getContextPath();
Expand Down
139 changes: 132 additions & 7 deletions ...st/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,26 @@
package com.microsoft.applicationinsights.web.internal;

import com.microsoft.applicationinsights.TelemetryClient;
import com.microsoft.applicationinsights.internal.reflect.ClassDataUtils;
import com.microsoft.applicationinsights.internal.reflect.ClassDataVerifier;
import com.microsoft.applicationinsights.web.utils.ServletUtils;
import org.junit.Assert;
import org.junit.Test;
import javax.servlet.*;
import javax.servlet.http.HttpServletResponse;

import org.mockito.Mockito;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
import com.microsoft.applicationinsights.web.utils.ServletUtils;

import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import java.io.IOException;
import java.lang.reflect.Field;

import static org.junit.Assert.assertFalse;
import static org.mockito.Mockito.*;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

/**
* Created by yonisha on 2/3/2015.
Expand Down Expand Up @@ -74,6 +77,17 @@ public void testFilterInitializedSuccessfullyFromConfiguration() throws ServletE
Filter filter = createInitializedFilter();
WebModulesContainer container = ServletUtils.getWebModuleContainer(filter);

Assert.assertNotNull("Container shouldn't be null", container);
Assert.assertTrue("Modules container shouldn't be empty", container.getModulesCount() > 0);

}

/* For parametrized constructor initialization test */
@Test
public void testParametrizedFilterInitializedSuccessfullyFromConfiguration() throws ServletException {
Filter filter = createParametrizedInitializedFilter("My-App");
WebModulesContainer container = ServletUtils.getWebModuleContainer(filter);

Assert.assertNotNull("Container shouldn't be null", container);
Assert.assertTrue("Modules container shouldn't be empty", container.getModulesCount() > 0);
}
Expand Down Expand Up @@ -102,58 +116,123 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
verify(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
}


/* For Parametrized Name setting */
@Test
public void testParametrizedFiltersChainWhenExceptionIsThrownOnModulesInvocation() throws Exception {
Filter filter = createParametrizedInitializedFilter("My-App");

// mocking
WebModulesContainer containerMock = ServletUtils.setMockWebModulesContainer(filter);
Mockito.doAnswer(new Answer() {
@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
throw new Exception("FATAL!");
}
}).when(containerMock).invokeOnBeginRequest(any(ServletRequest.class), any(ServletResponse.class));

FilterChain chain = mock(FilterChain.class);

ServletRequest request = ServletUtils.generateDummyServletRequest();

// execute
filter.doFilter(request, ServletUtils.generateDummyServletResponse(), chain);

// validate
verify(chain).doFilter(any(ServletRequest.class), any(ServletResponse.class));
}

@Test
public void testUnhandledRuntimeExceptionWithTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithTelemetryClient();
testException(createdData, new java.lang.IllegalArgumentException());

//For parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithTelemetryClient();
testException(createdDataParametrized, new java.lang.IllegalArgumentException());
}


@Test
public void testUnhandledRuntimeExceptionWithoutTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithoutTelemetryClient();
testException(createdData, new RuntimeException());

//For Parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithoutTelemetryClient();
testException(createdDataParametrized, new RuntimeException());

}

@Test
public void testUnhandledRuntimeExceptionWithTelemetryClientThatThrows() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithTelemetryClientThatThrows();
testException(createdData, new RuntimeException());

//For Parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithTelemetryClientThatThrows();
testException(createdDataParametrized, new RuntimeException());
}

@Test
public void testUnhandledServletExceptionWithTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithTelemetryClient();
testException(createdData, new ServletException());

//For parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithTelemetryClient();
testException(createdDataParametrized, new ServletException());
}

@Test
public void testUnhandledServletExceptionWithoutTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithoutTelemetryClient();
testException(createdData, new ServletException());

//For Parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithoutTelemetryClient();
testException(createdDataParametrized, new ServletException());
}

@Test
public void testUnhandledServletExceptionWithTelemetryClientThatThrows() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithTelemetryClientThatThrows();
testException(createdData, new ServletException());

//For Parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithTelemetryClientThatThrows();
testException(createdDataParametrized, new ServletException());

}

@Test
public void testUnhandledIOExceptionWithTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithTelemetryClient();
testException(createdData, new IOException());

//For parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithTelemetryClient();
testException(createdDataParametrized, new IOException());
}

@Test
public void testUnhandledIOExceptionWithoutTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithoutTelemetryClient();
testException(createdData, new IOException());

//For Parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithoutTelemetryClient();
testException(createdDataParametrized, new IOException());
}

@Test
public void testUnhandledIOExceptionWithTelemetryClientThatThrows() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithTelemetryClientThatThrows();
testException(createdData, new IOException());

//For Parametrized
FilterAndTelemetryClientMock createdDataParametrized = createParametrizedInitializedFilterWithTelemetryClientThatThrows();
testException(createdDataParametrized, new IOException());
}

// region Private methods
Expand Down Expand Up @@ -187,22 +266,66 @@ private Filter createInitializedFilter() throws ServletException {
return filter;
}

private Filter createParametrizedInitializedFilter(String appName) throws ServletException {
Filter filter = new WebRequestTrackingFilter(appName);

filter.init(null);
return filter;
}

private FilterAndTelemetryClientMock createInitializedFilterWithTelemetryClientThatThrows() throws ServletException, NoSuchFieldException, IllegalAccessException {
return createInitializedFilterWithMockTelemetryClient(true, true);
}

//For Parametrized name initialization
private FilterAndTelemetryClientMock createParametrizedInitializedFilterWithTelemetryClientThatThrows() throws ServletException, NoSuchFieldException, IllegalAccessException {
return createInitializedFilterWithMockTelemetryClient(true, true);
}

private FilterAndTelemetryClientMock createInitializedFilterWithTelemetryClient() throws ServletException, NoSuchFieldException, IllegalAccessException {
return createInitializedFilterWithMockTelemetryClient(true, false);
}

//For parametrized name initialization
private FilterAndTelemetryClientMock createParametrizedInitializedFilterWithTelemetryClient() throws ServletException, NoSuchFieldException, IllegalAccessException {
return createInitializedParametrizedFilterWithMockTelemetryClient(true, false);
}


private FilterAndTelemetryClientMock createInitializedFilterWithoutTelemetryClient() throws ServletException, NoSuchFieldException, IllegalAccessException {
return createInitializedFilterWithMockTelemetryClient(false, false);
}

//For parametrized name initialization
private FilterAndTelemetryClientMock createParametrizedInitializedFilterWithoutTelemetryClient() throws ServletException, NoSuchFieldException, IllegalAccessException {
return createInitializedParametrizedFilterWithMockTelemetryClient(false, false);
}

private FilterAndTelemetryClientMock createInitializedFilterWithMockTelemetryClient(boolean withTelemetryClient, boolean clientThrows) throws ServletException, NoSuchFieldException, IllegalAccessException {
Filter filter = createInitializedFilter();


Field field = WebRequestTrackingFilter.class.getDeclaredField("telemetryClient");
field.setAccessible(true);

StubTelemetryClient mockTelemetryClient = null;
if (withTelemetryClient) {
mockTelemetryClient = new StubTelemetryClient();
if (clientThrows) {
mockTelemetryClient.shouldThrow = true;
}
}
field.set(filter, mockTelemetryClient);

return new FilterAndTelemetryClientMock(filter, mockTelemetryClient);
}


/*For Parametrized constructor testing*/
private FilterAndTelemetryClientMock createInitializedParametrizedFilterWithMockTelemetryClient(boolean withTelemetryClient, boolean clientThrows) throws ServletException, NoSuchFieldException, IllegalAccessException {
Filter filter = createParametrizedInitializedFilter("My-App");
Copy link
Member

Choose a reason for hiding this comment

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

There is no validation that "My-app" has actually made it though and changed the way the code works.
You may want to add a test that checks my-app assignment or tests a change in the execution flow caused by the name being pre-assigned.



Field field = WebRequestTrackingFilter.class.getDeclaredField("telemetryClient");
field.setAccessible(true);

Expand All @@ -218,4 +341,6 @@ private FilterAndTelemetryClientMock createInitializedFilterWithMockTelemetryCli
return new FilterAndTelemetryClientMock(filter, mockTelemetryClient);
}
// endregion Private methods


}