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.

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

Added application insights starter for spring boot #518

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
55 changes: 55 additions & 0 deletions azure-application-insights-spring-boot-starter/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* ApplicationInsights-Java
* Copyright (c) Microsoft Corporation
* All rights reserved.
*
* MIT License
* Permission is hereby granted, free of charge, to any person obtaining a copy of this
* software and associated documentation files (the ""Software""), to deal in the Software
* without restriction, including without limitation the rights to use, copy, modify, merge,
* publish, distribute, sublicense, and/or sell copies of the Software, and to permit
* persons to whom the Software is furnished to do so, subject to the following conditions:
* The above copyright notice and this permission notice shall be included in all copies or
* substantial portions of the Software.
* THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
* PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
* FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*/

apply from: "$buildScriptsDir/common-java.gradle"
apply from: "$buildScriptsDir/publishing.gradle"

archivesBaseName = 'azure-application-insights-spring-boot-starter'

dependencies {
Copy link
Contributor

Choose a reason for hiding this comment

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

bunch of new dependencies. We need to be bit careful what is absolutely needed and also register them.

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 think we have everything needed here to use with spring boot, only new dependency is assertj and junit of newer version and both testCompile.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy Isn't junit 5.x still in beta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it was released in September and actually yesterday they did second GA release - 5.1.0 :)

compile (project(':core'))
compile (project(':web'))
provided('org.springframework.boot:spring-boot:1.5.9.RELEASE')
provided('org.springframework.boot:spring-boot-autoconfigure:1.5.9.RELEASE')
provided('org.springframework.boot:spring-boot-starter-web:1.5.9.RELEASE')
provided('org.springframework.boot:spring-boot-configuration-processor:1.5.9.RELEASE')

testCompile('org.junit.jupiter:junit-jupiter-api:5.0.2')
testCompile('org.junit.jupiter:junit-jupiter-params:5.0.2')
testCompile('org.junit.jupiter:junit-jupiter-engine:5.0.2')
testCompile('org.springframework.boot:spring-boot-starter-test:1.5.9.RELEASE')
testCompile('org.assertj:assertj-core:3.9.0')
}

jar {
enabled = false
}

// region Publishing properties

projectPomName = project.msftAppInsightsJavaSdk + " Spring Boot starter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this POM project name consistent with other AI pom generation scripts? For example look at the build.gradle in core module. It is slightly different. How does this difference affect here.

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 took web module to copy this I intentionally did not include shadowJar because I have no need in it.

ProjectPomName actually is equal to Microsoft Application Insights Java SDK Spring Boot starter, I don't know why other modules use msftAppInsights and then add ' Java SDK', for example project.msftAppInsights + " Java SDK Core" == project.msftAppInsightsJavaSdk + ' Core'

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, shadow jar is not needed in this case. Shadowing is usually done only for dependencies that we need to encapsulate like Apache HTTP Client. In this case the SpringBoot Starter jar will be shipped as a jar and it will pull all the transitive dependencies once loaded into the SpringBoot Project, so we do not need that.

projectPomDescription = "This is the Spring Boot starter of " + project.msftAppInsightsJavaSdk

whenPomConfigured = { p ->
def agentArtifactId = project(":agent").jar.baseName
p.dependencies = p.dependencies.findAll { dep -> dep.artifactId != agentArtifactId }
writePomToArtifactsDirectory(p, project.name)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* ApplicationInsights-Java
* Copyright (c) Microsoft Corporation
* All rights reserved.
*
* MIT License
* Permission is hereby granted, free of charge, to any person obtaining a copy of this
* software and associated documentation files (the ""Software""), to deal in the Software
* without restriction, including without limitation the rights to use, copy, modify, merge,
* publish, distribute, sublicense, and/or sell copies of the Software, and to permit
* persons to whom the Software is furnished to do so, subject to the following conditions:
* The above copyright notice and this permission notice shall be included in all copies or
* substantial portions of the Software.
* THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
* PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
* FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*/

package com.microsoft.applicationinsights.boot;

import com.microsoft.applicationinsights.boot.conditional.ConditionalOnOperatingSystem;
import com.microsoft.applicationinsights.boot.conditional.OperatingSystem;
import com.microsoft.applicationinsights.boot.initializer.SpringBootContextInitializer;
import com.microsoft.applicationinsights.extensibility.initializer.DeviceInfoContextInitializer;
import com.microsoft.applicationinsights.extensibility.initializer.SdkVersionContextInitializer;
import com.microsoft.applicationinsights.internal.perfcounter.ProcessPerformanceCountersModule;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.core.env.Environment;

@Configuration
public class ApplicationInsightsModuleConfiguration {

@Bean
public SpringBootContextInitializer springBootContextInitializer(Environment environment) {
return new SpringBootContextInitializer(environment);
}

@Bean
public SdkVersionContextInitializer sdkVersionContextInitializer() {
return new SdkVersionContextInitializer();
}

@Bean
public DeviceInfoContextInitializer deviceInfoContextInitializer() {
return new DeviceInfoContextInitializer();
}

@Bean
@ConditionalOnOperatingSystem(OperatingSystem.WINDOWS)
public ProcessPerformanceCountersModule processPerformanceCountersModule() {
try {
return new ProcessPerformanceCountersModule();
}
catch (Exception e) {
throw new IllegalStateException("Could not initialize Windows performance counters module", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

The throw from here means it would crash the user application? In that event it is much advised to rather log the message using logger rather than throwing.

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 will change this to logging error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

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 remember why I did it, actually you can't just log error here because bean instance must be returned. I'm not quite sure how to fix this properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I don't see any reason why it would throw an exception. ProcessBuiltInPerformanceCountersFactory is instance of WindowsPerformanceCountersFactory and if something goes wrong inside this factory it returns empty list.

Choose a reason for hiding this comment

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

Seems like very unlikely situation. Can't you return null in such case?

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 don't think it's possible, in best case we can either create conditional where we try to create instance and return true only if it was successfully created, or have message with clear action to do, something like 'We are failed to create performance metric, please set azure.application-insights.default-modules.ProcessPerformanceCountersModule.enabled=false to avoid this error message'. But in general I don't see any chance for this exception to be thrown in real code. @dhaval24 what do you think?

Choose a reason for hiding this comment

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

@gavlyukovskiy from technical point of view it's possible, such bean declaration is 100% valid and it works:
@Bean public CustomBean customBean(){ return null; }

Copy link
Contributor Author

@gavlyukovskiy gavlyukovskiy Feb 22, 2018

Choose a reason for hiding this comment

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

You're right, it's valid. But then I should check that module is not null when initializing them in TelemetryConfiguration, I can do this as well, but it just seem useless to me to add a lot of checks that verify situation that can not happen. That's why I throw IllegalStateException here. If there are will be issue with that it indicates bug on our side and you can't save user from all bugs you may ever have. Although user has simple switch to disable this module so it won't be a blocker anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy I am fine with failing and asking to set the message please set azure.application-insights.default-modules.ProcessPerformanceCountersModule.enabled=false to avoid this error message'. Please add this conditional.

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
/*
* ApplicationInsights-Java
* Copyright (c) Microsoft Corporation
* All rights reserved.
*
* MIT License
* Permission is hereby granted, free of charge, to any person obtaining a copy of this
* software and associated documentation files (the ""Software""), to deal in the Software
* without restriction, including without limitation the rights to use, copy, modify, merge,
* publish, distribute, sublicense, and/or sell copies of the Software, and to permit
* persons to whom the Software is furnished to do so, subject to the following conditions:
* The above copyright notice and this permission notice shall be included in all copies or
* substantial portions of the Software.
* THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
* PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE
* FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
* OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*/

package com.microsoft.applicationinsights.boot;

import com.microsoft.applicationinsights.channel.TelemetryChannel;
import com.microsoft.applicationinsights.internal.logger.InternalLogger.LoggerOutputType;
import com.microsoft.applicationinsights.internal.logger.InternalLogger.LoggingLevel;
import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties("azure.application-insights")
public class ApplicationInsightsProperties {

private boolean enabled = true;
private String instrumentationKey;
private Channel channel = new Channel();
private QuickPulse quickPulse = new QuickPulse();
private Logger logger = new Logger();

public boolean isEnabled() {
return enabled;
}

public void setEnabled(boolean enabled) {
this.enabled = enabled;
}

public String getInstrumentationKey() {
return instrumentationKey;
}

public void setInstrumentationKey(String instrumentationKey) {
this.instrumentationKey = instrumentationKey;
}

public Channel getChannel() {
return channel;
}

public void setChannel(Channel channel) {
this.channel = channel;
}

public QuickPulse getQuickPulse() {
return quickPulse;
}

public void setQuickPulse(QuickPulse quickPulse) {
this.quickPulse = quickPulse;
}

public Logger getLogger() {
return logger;
}

public void setLogger(Logger logger) {
this.logger = logger;
}

public static class QuickPulse {
private boolean enabled = true;

public boolean isEnabled() {
return enabled;
}

public void setEnabled(boolean enabled) {
this.enabled = enabled;
}
}

public static class Channel {
Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy I think we need default values here. If the user does not specify this property the default InProcessTelemetryChannel should be created. Does @ConditionalOnMissingBean annotation you added address this, in case user doesn't specify have this property in his properties file will the default channel get created (I hope so it does based on Line 85 in TelemetryConfigurationFactory Enum). However if the user does specify this property then you are using a parametrized constructor which implies that user has to now provide all the values and has lost the ability to tweak single property of channel say turn on developer mode or change end point address. Please correct me if I am wrong here, but if that is indeed the case we need to make sure there are default values for this and they should match default values of InProcessTelemetryChannel class.

Further we also need to set conditionals on the value the user specifies. I know that limit enforcer does this but setting conditions here would also be very useful. What do you think ?

Copy link
Contributor Author

@gavlyukovskiy gavlyukovskiy Feb 25, 2018

Choose a reason for hiding this comment

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

I added default values for every property in latest commit. Yes, I added @ConditionalOnMissingBean for user to be able to override this whole bean if needed, not the specific property. Even now with all properties configurable there are could be use-cases for that, for example using custom channel that will log every telemetry into the log or something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the conditions you're asking? Something like that the maximum buffer size should be between 1 and 1000?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gavlyukovskiy Yes exactly this is what I meant. If user uses inprocess telemetry channel and specifies different values then what channel expects then we should take the default value. As I mentioned that there is a logic inside the channel using LimitsEnforcer class however in order to have our starter to be very self sufficient and robust I think we should have this checks here. What do you think?

private InProcess inProcess = new InProcess();

public InProcess getInProcess() {
return inProcess;
}

public void setInProcess(InProcess inProcess) {
this.inProcess = inProcess;
}

public static class InProcess {
private boolean developerMode = false;
private String endpointAddress;
private int maxTelemetryBufferCapacity;
private int flushIntervalInSeconds;
private int maxTransmissionStorageFilesCapacityInMb;
private boolean throttling = true;

public boolean isDeveloperMode() {
return developerMode;
}

public void setDeveloperMode(boolean developerMode) {
this.developerMode = developerMode;
}

public String getEndpointAddress() {
return endpointAddress;
}

public void setEndpointAddress(String endpointAddress) {
this.endpointAddress = endpointAddress;
}

public int getMaxTelemetryBufferCapacity() {
return maxTelemetryBufferCapacity;
}

public void setMaxTelemetryBufferCapacity(int maxTelemetryBufferCapacity) {
this.maxTelemetryBufferCapacity = maxTelemetryBufferCapacity;
}

public int getFlushIntervalInSeconds() {
return flushIntervalInSeconds;
}

public void setFlushIntervalInSeconds(int flushIntervalInSeconds) {
this.flushIntervalInSeconds = flushIntervalInSeconds;
}

public int getMaxTransmissionStorageFilesCapacityInMb() {
return maxTransmissionStorageFilesCapacityInMb;
}

public void setMaxTransmissionStorageFilesCapacityInMb(int maxTransmissionStorageFilesCapacityInMb) {
this.maxTransmissionStorageFilesCapacityInMb = maxTransmissionStorageFilesCapacityInMb;
}

public boolean isThrottling() {
return throttling;
}

public void setThrottling(boolean throttling) {
this.throttling = throttling;
}
}
}

public static class Logger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this class mean default logging values? If yes, then level should be error. Info logging will flood the user's production environment and reduce throughput.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there are defaults. Will change to ERROR

private LoggerOutputType type = LoggerOutputType.CONSOLE;
private LoggingLevel level = LoggingLevel.INFO;

public LoggerOutputType getType() {
return type;
}

public void setType(LoggerOutputType type) {
this.type = type;
}

public LoggingLevel getLevel() {
return level;
}

public void setLevel(LoggingLevel level) {
this.level = level;
}
}
}
Loading