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 all 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
101 changes: 71 additions & 30 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,31 @@

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.schemav2.Internal;
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.io.InputStream;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Date;
import java.util.LinkedList;
import java.util.Properties;

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

//For External Name Setting
private String applicationName;

// 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 +87,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 +124,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 +169,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 @@ -212,18 +219,14 @@ private synchronized void initialize(FilterConfig filterConfig) {
//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;
}

ServletContext context = filterConfig.getServletContext();

String name = getName(context);

String key = registerWebApp(name);
applicationName = getName(context);
String key = registerWebApp(applicationName);
setKey(key);

InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.INFO, "Successfully registered the filter '%s'", FILTER_NAME);
} catch (Throwable t) {
InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.ERROR, "Failed to register '%s', exception: '%s'", FILTER_NAME, t.getMessage());
Expand Down Expand Up @@ -257,7 +260,13 @@ private String registerWebApp(String name) {
}

private String getName(ServletContext context) {
String name = null;

String name = getApplicatioNameFromProperties();

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

try {
String contextPath = context.getContextPath();
if (CommonUtils.isNullOrEmpty(contextPath)) {
Expand All @@ -284,6 +293,38 @@ private String getName(ServletContext context) {
return name;
}

/*This method is used to parse application name from the properties file*/
private String getApplicatioNameFromProperties() {
Copy link
Member

Choose a reason for hiding this comment

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

We might want to add a unit test for this new behavior.

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 agree that there should be test to test this behavior. However the WebRequestTrackingFilter does not implement any interface that we can mock using Mockito framework. The only way that we can write a testcase for this using JUnit can be actually reading from the file and doing the verification. Now that comes with a caveat. We need to store that file into our source code somewhere. If you have any other ideas on testing let me know.

Copy link
Member

Choose a reason for hiding this comment

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

You can make WebRequestTrackingFilter implement an interface if it's required for tests since you control the code. Storing file within test project for test purposes is perfectly fine - you can consider such a file a 'test resource'.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dmitry-Matveev interface will not help here, mocking interface makes only interface methods available for mocking. Better to make class not final and Mockito will be able to mock it.

Copy link
Member

Choose a reason for hiding this comment

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

@gavlyukovskiy, makes sense. @dhaval24, could you please take a look at this approach?


Properties properties = new Properties();
InputStream input = null;
String name = null;
try {
String fileName = "application.properties";
input = WebRequestTrackingFilter.class.getClassLoader().getResourceAsStream(fileName);
if (input == null) {
return null;
}

properties.load(input);
name = properties.getProperty("spring.application.name");
Copy link
Member

Choose a reason for hiding this comment

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

Is it the way we're going to use for spring only or the name should be more generic?

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 yes as of now I am not aware of any other framework that reads the value from the properties file though this is through a very preliminary research. There can be places where this is the only way but not in my knowledge. For all other cases we are already reading from application context. Also just pushed the corrected version where the public constructor has been removed which was accidentally present in the previous commit.

Copy link
Member

Choose a reason for hiding this comment

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

This property is going to be our public interface to configure application name that something can take dependency on, so changing it once released is not an easy task.
I think renaming this property before release to a more generic scope rather than "spring" will make us stay on the safe side and prevent "deprecation"/rename issues in the future.

}
catch (IOException ex) {
InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.ERROR, "Exception in loading properties file: '%s'",ex.getMessage());
}
finally {
if (input != null) {
try {
input.close();
}
catch (IOException e) {
InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.WARN, "Error while closing properties file '%s'",e.getMessage());
}
}
}
return name;
}

private void setKey(String key) {
if (CommonUtils.isNullOrEmpty(key)) {
agentIsUp = false;
Expand Down
22 changes: 15 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,27 @@
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.Mock;
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 @@ -76,6 +80,7 @@ public void testFilterInitializedSuccessfullyFromConfiguration() throws ServletE

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

}

@Test
Expand Down Expand Up @@ -108,6 +113,7 @@ public void testUnhandledRuntimeExceptionWithTelemetryClient() throws IllegalAcc
testException(createdData, new java.lang.IllegalArgumentException());
}


@Test
public void testUnhandledRuntimeExceptionWithoutTelemetryClient() throws IllegalAccessException, NoSuchFieldException, ServletException {
FilterAndTelemetryClientMock createdData = createInitializedFilterWithoutTelemetryClient();
Expand Down Expand Up @@ -217,5 +223,7 @@ private FilterAndTelemetryClientMock createInitializedFilterWithMockTelemetryCli

return new FilterAndTelemetryClientMock(filter, mockTelemetryClient);
}

// endregion Private methods

}