Dhdoshi/spring boot name bug #411

dhaval24 · 2017-08-18T21:02:31Z

Fix #359

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public surface reviewed
CHANGELOG.md updated

…p name setting

SergeyKanzhelev · 2017-08-21T20:46:41Z

See the checklist in PR description? Please follow the check boxes:

Mention the change in public surface - new constructor.
Update CHANGELOG.md

Also make sure that the new public method has a Javadoc with explanation how it when to use it.

SergeyKanzhelev · 2017-08-21T20:47:40Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -59,20 +58,24 @@
    private boolean agentIsUp = false;
    private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

+    //Only for Spring Boot Application (External Name Setting)


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

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.

As soon as we make new constructor public, we cannot control if it's going to be used for Spring Boot only or not.

There might be another framework we haven't tried yet that required application name setup outside of configuration xml files.

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.

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

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

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

SergeyKanzhelev · 2017-08-21T20:48:08Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -257,6 +265,12 @@ private String registerWebApp(String name) {
    }

    private String getName(ServletContext context) {
+
+        // If Application is of type Spring Boot


again, do not limit it to spring boot only

SergeyKanzhelev · 2017-08-21T20:50:33Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -59,20 +58,24 @@
    private boolean agentIsUp = false;
    private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

+    //Only for Spring Boot Application (External Name Setting)
+    private String applicationName;


make it final

Sure can make that as final

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

SergeyKanzhelev · 2017-08-21T20:51:27Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -59,20 +58,24 @@
    private boolean agentIsUp = false;
    private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

+    //Only for Spring Boot Application (External Name Setting)
+    private String applicationName;


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

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

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

Dmitry-Matveev · 2017-08-21T21:33:25Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -59,20 +58,24 @@
    private boolean agentIsUp = false;
    private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

+    //Only for Spring Boot Application (External Name Setting)


As soon as we make new constructor public, we cannot control if it's going to be used for Spring Boot only or not.

There might be another framework we haven't tried yet that required application name setup outside of configuration xml files.

Dmitry-Matveev · 2017-08-21T21:35:31Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -59,20 +58,24 @@
    private boolean agentIsUp = false;
    private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

+    //Only for Spring Boot Application (External Name Setting)
+    private String applicationName;


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

Dmitry-Matveev · 2017-08-21T21:38:09Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -59,20 +58,24 @@
    private boolean agentIsUp = false;
    private final LinkedList<ThreadLocalCleaner> cleaners = new LinkedList<ThreadLocalCleaner>();

+    //Only for Spring Boot Application (External Name Setting)


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.

Dmitry-Matveev · 2017-08-21T21:43:20Z

.../test/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilterTests.java

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


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.

…armetrized constructor

dhaval24 · 2017-08-30T21:24:53Z

@SergeyKanzhelev @Dmitry-Matveev In the recent commits, I have removed the public constructor as it could have possible consequences of breaking other components like java agent, because whenever you set the name of the web-app it also gets the same name registered with the Java Agent and this only happens initially. Now if we expose public constructor, then someone might change the name of the application in the middle of the execution and in this case Java Agent might get unhooked.

There were couple of work-arounds that I had:

Whenever this constructor is called, set the name in agent too (kind of hacky)
Expose public method instead of constructor (still public) and a check before assigning the name- would free us from our end but customers would have false impression.
Private method which reads from the properties file (thought would be the best)

However, one caveat - this is still a patch, a full fleged solution would be to like having another config module similar to what we have for XML, though its worth while to investigate if we need those efforts at all.

Please provide your suggestions.

Dmitry-Matveev · 2017-09-08T03:08:29Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

+     * Parameterized constructor, used to set Application name explicitly
+     * @param applicationName
+     */
+    public WebRequestTrackingFilter(String applicationName) {


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

Dmitry-Matveev · 2017-09-08T03:10:20Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

+            }
+
+            properties.load(input);
+            name = properties.getProperty("spring.application.name");


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

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

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.

Dmitry-Matveev · 2017-09-08T03:11:57Z

web/src/main/java/com/microsoft/applicationinsights/web/internal/WebRequestTrackingFilter.java

@@ -284,6 +301,38 @@ private String getName(ServletContext context) {
        return name;
    }

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


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

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

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

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

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

gavlyukovskiy · 2017-10-19T08:07:57Z

Hard coding getting property spring.application.name from application.properties from the classpath it is not the proper way to go. According to section 24.3 Application property files of Spring Boot Reference Guide not only application.properties can be used, but also application.yaml and located not only in the classpath, but also in application directory or config subdirectory. For example we use config subdirectory for deployment to set deployment specific properties.

I understand that spring.application.name is not good candidate for such overriding, but at least case with application.yaml is very popular.
I think the most reliable way is to give ability to the user to define this property using constructor, assuming that Spring Boot users will make something like that:

@Bean
public FilterRegistrationBean webRequestTrackingFilter(@Value("${spring.application.name}") String applicationName) {
    FilterRegistrationBean registration = new FilterRegistrationBean();
    registration.setFilter(new WebRequestTrackingFilter(applicationName));
    registration.addUrlPatterns("/*");
    registration.setName("webRequestTrackingFilter");
    registration.setOrder(1);
    return registration;
}

Dmitry-Matveev · 2017-10-20T03:15:10Z

@gavlyukovskiy, I remember @dhaval24 was trying to address this with the constructor but as far as I remember the continuation on this, the things didn't play well down the road for some reason. @dhaval24 would have more details to share.

dhaval24 · 2017-10-20T04:59:06Z

@gavlyukovskiy I wast taking the exact same approach as you suggested in the first place however after some digging into the code I found that. we use the same name to bind Java Agent also. As I expressed in the previous comments on this PR there is possible risk of de-binding the agent from SDK because the person might be able to change the name of the field by calling the public constructor with argument. However, if I think deeply on this the constructor is only called when the user intents to get a new instance and hence might be okay.

However, the issue with this approach is that we might expose a public constructor which is not required at all. Also, when I was dealing with this I had an opinion that we might need a separate module to read the configurations from the properties file in the same way as we do for XML because I believe that there are several spring boot applications that are standalone only and doesn't have War files, these applications don't have any xml files in theory so it would be best if we can read all the config from properties file but then we had a discussion that is it worth the effort and therefore we haven't proceed on this yet. Let me know if you have any better ideas other than public constructor (may be addressing wider problems relating to spring boot)

gavlyukovskiy · 2017-10-20T09:41:04Z

Reading configuration from properties will not help much for Spring Boot users. It is not just read application.properties because of

Config location - file:./config/, file:./, classpath:/config/, classpath:/ by default and can be overridden by using parameter --spring.config.location=classpath:/myprops.properties when running jar/war
Different property types - .properties, .yaml
Profiles - you may have application-prod.properties and application-dev.properties
Relaxed binding - property spring.application.name can be read from system variables SPRING_APPLICATION_NAME

The only way to handle this is using Spring classes that are aware of those type of configuration - Environment.getProperty("property") (no relaxed binding support), @Value("${property}") (no relaxed binding support) and class annotated with @ConfigurationProperties.
Latter beside relaxed binding support also a type safe and has metadata support, that is used in IDE's to autocomplete properties and show their description.

If you want to support Spring Boot I think the better way is to add some property map (or tags) in your TelemetryConfiguration to use in your core project and a way to configure it from outside (while configuring Spring application context). Then create spring boot starter module for Application Insights, say application-insights-spring-boot-starter. In documentation there is suggestion to create two different modules auto-configuration and starter but it makes sense only if you have more than 1 starter with different libraries setup. In this starter configure TelemetryConfiguration and set all external properties you need using spring configuration.

If you want to look at starter examples you may refer to 3rd-party spring boot starters page or to my project with few starters for data source decorating libraries.

We in Cloudyn (Azure Cost Management) are interested in integration with Application Insights as all our applications are spring boot we anyhow will create some kind of a starter for our needs. If spring boot is in your priorities we can cooperate on this, my Microsoft email is [email protected].

dhaval24 · 2017-10-20T16:38:52Z

@gavlyukovskiy This makes much more sense to me now. Totally agree with your point that reading from properties won't suffice. Thanks for your interest in using Application Insights and definitely SpringBoot is one of the most popular web frameworks in Java community and it makes sense to have first class support for SpringBoot. I will start an email thread with you.

dhaval24 · 2017-11-30T18:14:24Z

Closing this PR as public constructor is now introduced to support this as part of #449 PR Merge

dhaval24 · 2017-11-30T18:14:52Z

#461 is still active and work will be in progress on that.

Dhaval Doshi added 2 commits August 18, 2017 13:46

Adding Parametrized constructor to support extrnal spring boot web ap…

c51b53a

…p name setting

Adding tests to support external name setting for spring boot webapp

11516bf

dhaval24 added the Bug label Aug 18, 2017

dhaval24 added this to the 1.0.10 milestone Aug 18, 2017

dhaval24 requested review from SergeyKanzhelev and Dmitry-Matveev August 18, 2017 21:02

msftclas added the cla-not-required label Aug 18, 2017

dhaval24 removed the Bug label Aug 18, 2017

SergeyKanzhelev reviewed Aug 21, 2017

View reviewed changes

SergeyKanzhelev suggested changes Aug 21, 2017

View reviewed changes

Dmitry-Matveev requested changes Aug 21, 2017

View reviewed changes

dhaval24 added 3 commits August 24, 2017 13:51

Updating comments

37b6fc8

Merge branch 'master' into dhdoshi/SpringBootNameBug

16dda62

Adding private method to read from properties file, removing public p…

cf59793

…armetrized constructor

Dmitry-Matveev requested changes Sep 8, 2017

View reviewed changes

Fixed removing public parametrized constructor

544b0ad

msftgits removed the cla-not-required label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

gavlyukovskiy mentioned this pull request Nov 7, 2017

Programmatic configuration for TelemetryConfiguration #461

Closed

dhaval24 closed this Nov 30, 2017

dhaval24 deleted the dhdoshi/SpringBootNameBug branch November 30, 2017 18:14

gavlyukovskiy mentioned this pull request Jan 9, 2018

Added application insights starter for spring boot #518

Merged

3 tasks

Dhdoshi/spring boot name bug #411

Dhdoshi/spring boot name bug #411

Conversation

dhaval24 commented Aug 18, 2017

SergeyKanzhelev commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavlyukovskiy commented Oct 19, 2017

Dmitry-Matveev commented Oct 20, 2017

dhaval24 commented Oct 20, 2017

gavlyukovskiy commented Oct 20, 2017 • edited Loading

dhaval24 commented Oct 20, 2017

dhaval24 commented Nov 30, 2017

dhaval24 commented Nov 30, 2017

gavlyukovskiy commented Oct 20, 2017 •

edited

Loading