-
Notifications
You must be signed in to change notification settings - Fork 205
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.
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
Azure Application Insights SpringBoot Starter #646
Conversation
Clarified all default values in readme; Changed default logging level to error; Added @ConditionalOnWebApplication for web configurations.
…num for property auto complete
…in spring boot starter
# Conflicts: # core/src/main/java/com/microsoft/applicationinsights/channel/concrete/inprocess/InProcessTelemetryChannel.java # core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionFileSystemOutput.java # core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java # core/src/test/java/com/microsoft/applicationinsights/internal/channel/inprocess/InProcessTelemetryChannelTest.java
Added application insights starter for spring boot
…n to be created first
…tTags are not reflected on UX if not in bond schema definition
…on for JMX counters and JVM counters
FilterRegistrationBean registration = new FilterRegistrationBean(); | ||
registration.setFilter(webRequestTrackingFilter); | ||
registration.addUrlPatterns("/*"); | ||
registration.setName("webRequestTrackingFilter"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be hardcoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj and @grlima I looked into this method. This is method is usually used to set the name of the RegistrationBean, however it is not necessary to set the name and the default behavior here is that if this is not set explicitly the name of the bean would be taken into the account.
Further, a surprising thing this method no longer seems to be present in the latest SpringBoot framework however no mention anywhere in the change logs too.
spring-projects/spring-boot@9cb5f3d#diff-4c7df2b424db8ecb8e18ac9485ddcc8d
Therefore just to be safe i would remove it. Anyways, it doesn't affect the behavior either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* | ||
* @author Dhaval Doshi | ||
*/ | ||
public class SpringBootTelemetryInitializer implements TelemetryInitializer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would someone want to use this having a spring boot app but not using the starter?
If so, an argument could be made to move this under the core project. Of course, that means removing the @Value
annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would break it into two parts:
- Would someone like to use AI with SpringBoot without starter? : Possibility, relatively very slim though.
- Can we move this to core? - I don't think so because we cannot distinguish if the app is of type springboot in the core and then downstream methods would fail. Further, I think this is nice separation of logics. I think spring related stuff should be at one place.
@@ -21,15 +21,11 @@ | |||
|
|||
package com.microsoft.applicationinsights.channel.concrete.inprocess; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is really hard to see what has changed here.
Please reduce the amount of code style changes here so we can easily locate the functional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure in my recent commit I have tried to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhaval24, What has functionally changed here? I'm only seeing whitespace changes. If nothing has changed related to spring boot starter, please revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj There is a new public constructor in this. In addition there are some member variables which are converted to public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I dislike the additional changes in this file....this should be fine. If you can find time to fix this, please do.
@@ -21,11 +21,9 @@ | |||
|
|||
package com.microsoft.applicationinsights.extensibility.initializer; | |||
|
|||
import com.microsoft.applicationinsights.internal.logger.InternalLogger; | |||
import com.microsoft.applicationinsights.extensibility.ContextInitializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary change...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is only about removing unused import. Why it's unnecessary? They are overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do style clean-up in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no harm touching some files here. I agree big changes can go separately, this is just a import removal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'm fine with fixing some style issues here and there, but try to keep it contained to files which are related to the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way, if we want a list of files having to do with this feature, we can simply look at files changed in this PR or the squashed commit. Having this here makes the git history look like this file has something to do with the spring boot starter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj I agree with you on this. I have tried to reduce majority of styling things at other places too.
#resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not resolved.... please revert since it's has no relevance to this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj I thought we had discussed that since this was only an import removal, it was fine. If this opinion has changed and you think that we should keep the additional import then I can bring it back. In my opinion, we rarely get chance to work on code cleaning and small changes (which do not affect the logic should be fine). While I do understand your concern for clean history this should not be something which is too big of change. @grlima what are your thoughts? If you both agree on reverting this small change I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj I have reverted this file. Thanks fore your concerns on separating the logic.
// chomp | ||
} | ||
} | ||
//If Agent Jar is not present in the class path skip the process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't change. Spring Boot Starter has nothing to do with detecting the agent. Does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is a bug in-itself. We write unnecessary log message and discover the agent in a very poor way. I think we discussed about this in past. Not sure if you remember it though and we had debugged through it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detecting the agent is an important part of our current functionality. It could be better, but it's a separate issue. It should be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's a seperate issue, but I would much rather prefer to have it fixed along with this roll out. If required this can be branched out of this PR, but changes need to be there when the starter rolls out otherwise we get a lot of superfluous error messages. I believe you would agree with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should address it separately. I made some comments in #579. I'd like to have a separate discussion regarding this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted this changes. I will submit another PR for this changes. #resolved
|
||
@Test | ||
public void testInProcessTelemetryChannelWithDefaultSpringBootParameters() { | ||
new InProcessTelemetryChannel("https://dc.services.visualstudio.com/v2/track", "10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there are other tests that follow this pattern, but I'd rather not continue to use it.
Checking the constructor doesn't throw is good, but we should assert on the effects the parameters have on the resulting object; e.g. assertEquals(theGivenEndpoint, channel.getEndpoint())
; I don't know if that method exists, but you get the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj I think this can be refactored but then we might need to use reflection to test the values of InProcessTelemetryChannel. Do you prefer it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, don't worry about it. This is fine for now. I didn't realize it didn't have accessors for most fields. Perhaps we can add accessors in the future or make the class not final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
private static final String pageViewTelemetryName = "PageView"; | ||
private static final String requestTelemetryName = "Request"; | ||
private static final String traceTelemetryName = "Trace"; | ||
public static final double DEFAULT_SAMPLING_PERCENTAGE = 100.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing dot is syntactically correct? if so, there's nothing wrong here. I just didn't know that 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't fail, but probably typo. I don't know either. I will make it 100.0 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably for the best. It's an easy character to miss if you're skimming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
|
||
private static Map<String, Class> allowedTypes; | ||
static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with moving this to the static constructor, but changing the map type is unnecessary. We don't need to introduce parsing into this. String.hashCode does a fine job here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By doing this IDE can perform auto-complete when users are typing this properties. That was the reason of this change. Providing recommendation is a good value addition. It would avoid people to look into docs multiple times and switch screens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhaval24, let's discuss offline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj I have given some deep thoughts on this, and I think I would agree with you on this. Here are the reasons: This enum is only used for the sampling, not required elsewhere. It adds an overhead of maintaining it and ensuring that all new types are also added to it. A person can easily miss this unless there is an automated way to do it. Further more this enum is public which then adds the overhead of not deprecating it before a major version. The degree of advantage it provides is not absolute necessity and overhead is pretty high.
I will make this changes :) Thanks for drawing the attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
@@ -0,0 +1,19 @@ | |||
package com.microsoft.applicationinsights.internal.channel.samplingV2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in FixedRateSamplingTelemetryProcessor. This should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response above for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be amended based on my response above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
* | ||
* @author Dhaval Doshi | ||
*/ | ||
public class SpringBootHeartBeatProvider implements HeartBeatPayloadProviderInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to extend the implementation in core? or make it a delegate? You shouldn't need to reimplement the whole class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think no. The design for this is such that, each separate functionality has it's own HB provider. For example springboot has it's own, Azure VM will have it's own and WebApps have it's own. This provides the flexibility for user to turn off the whole provider in itself incase if they do not need it rather then handpicking each properties to disable. The contract each Provider has to follow is to implement the interface. I think this provides nice abstraction, separates the module in itself from core and lets us be able to freely modify it without affecting others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i see. This is sending spring boot specific data; a decorator of heartbeat events. Makes sense.
* @return {@link com.microsoft.applicationinsights.TelemetryConfiguration} | ||
*/ | ||
public static TelemetryConfiguration getActiveWithoutInitializingConfig() { | ||
synchronized (s_lock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch :) will add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#resolved
My main concern is that we've essentially reimplemented configuration loading for the spring boot case. If not, there needs to be a test which creates a configuration object the normal way and an equivalent object in the spring boot way to make sure the spring boot code path makes the same object. |
public HeartBeatModule heartBeatModule(Environment environment) { | ||
try { | ||
HeartBeatModule heartBeatModule = new HeartBeatModule(); | ||
HeartbeatDefaultPayload.addDefaultPayLoadProvider(new SpringBootHeartBeatProvider(environment)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's expected from users to be able to provide own HeartBeatProvider
implementation it's worth to create bean:
@Bean
@ConditionalOnMissingBean
public HeartBeatPayloadProviderInterface heartBeatPayloadProviderInterface(Environment environment) {
return new SpringBootHeartBeatProvider(environment);
}
and inject HeartBeatPayloadProviderInterface
into HeartBeatModule
. It's important to have interface declaration instead of implementation - SpringBootHeartBeatProvider
because only this way users are able to override the bean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all cases we would like to have the properties of the current provider for better assessment. However it makes sense to do this more elegantly using bean. I would refactor this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gavlyukovskiy this is now addressed.
…arter accordingly, making HeartBeatPayloadProviderInterface bean, fixing critical issue with sampling configuration
This reverts commit bc19586.
I wanted to point out the following: By reverting the changes of how agent is detected to the original implementation, the user will see this misleading exception message when the Agent Jar is not present. AI: ERROR 04-05-2018 09:08:53.150, 31(localhost-startStop-1): Could not find Agent: 'java.lang.NoClassDefFoundError: com/microsoft/applicationinsights/agent/internal/coresync/AgentNotificationsHandler IMO, this is very confusing and I can remember couple of tickets when users pointed out to this weired behavior. My suggestion : We can take two approaches -
My opinion will hinge towards second suggestion, because effectively when a runtime exception is thrown compiler has to jump instructions which makes the program terribly slow. You can look at more details on this thread (https://stackoverflow.com/questions/299068/how-slow-are-java-exceptions) |
… version number to HB telemetry
Modularized the way to specify version number for starter and added the Starer version in HeartBeat payload for better diagnosis. |
private static final String eventTelemetryName = "Event"; | ||
private static final String exceptionTelemetryName = "Exception"; | ||
private static final String pageViewTelemetryName = "PageView"; | ||
private static final String requestTelemetryName = "Request"; | ||
private static final String traceTelemetryName = "Trace"; | ||
|
||
private static Map<String, Class> allowedTypes; | ||
static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj dual initialization is now fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed offline with stakeholders.
This PR introduces a final version of Azure Application Insights SpringBoot starter. It extends on the work done in #518 by @gavlyukovskiy.
The starter closely has all the required functionalities and tuning knobs that ApplicationInsights.xml file provides to the user using application.properties and spring dependency injection.
The core additional functionality which are added on the Top of #518 are as follows:
It also removes all field injection and instead now uses constructor injection and setter injection as recommended by Spring.
Thank you @gavlyukovskiy for your very vital contribution to our project.
Note: This should not be merged until #631 is merged. Once #631 is merged -> merge master in this branch resolve API changes and then merge.
For significant contributions please make sure you have completed the following items: