Added application insights starter for spring boot #518

gavlyukovskiy · 2018-01-09T01:01:56Z

I want to contribute new module for Spring Boot users.
Initially issue was about wrong setting of application name in Spring Boot applications, but then I thought it would be nice to configure application insights like Spring Boot users used to do it - through spring boot configuration.

This starter takes care of proper initialization of TelemetryConfiguration and TelemetryClient. Basically only setting azure.application-insights.instrumentation-key is required to start using Application Insights with Spring Boot.

I tried to make tests self explanatory but here is a couple of features I've made:

Ability to configure Application Insights using application.properties

azure.application-insights.enabled=true|false
azure.application-insights.instrumentation-key=<key>
azure.application-insights.quick-pulse.enabled=true|false
azure.application-insights.channel.in-process.developer-mode=true|false
azure.application-insights.channel.in-process.flush-interval-in-seconds=<number>
azure.application-insights.channel.in-process.max-telemetry-buffer-capacity=<number>
azure.application-insights.channel.in-process.max-transmission-storage-files-capacity-in-mb=<number>
azure.application-insights.channel.in-process.throttling=true|false
azure.application-insights.logger.type=console|file
azure.application-insights.logger.level=all|trace|info|warn|error|off

Adding custom modules by declaring as a bean:

@Bean
public TelemetryModule myTelemetryModule() {
    return new TelemetryModule() {
        @Override
        public void initialize(TelemetryConfiguration configuration) {
            // do some staff here
        }
    };
}

Initializing WebRequestTrackingFilter with right application name.
Adding spring, spring boot versions as tags and resolving right IP if running with spring cloud infrastructure.

I also have added a test case for which I need #516 to be merged -
ApplicationInsightsTelemetryAutoConfigurationTests#shouldReloadInstrumentationKeyOnTelemetryClient.

As I heard you don't have much experience with Spring Boot so if something is not clear I'm free to answer your questions.

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

Design discussion issue in comments of Dhdoshi/spring boot name bug #411 and internal discussions
Changes in public surface reviewed
CHANGELOG.md updated

dhaval24 · 2018-01-09T01:06:40Z

@gavlyukovskiy this is phenomenal. Though, I haven't looked into details of this, work seems really promising. Indeed it would be a very very valuable contribution for Application Insights Java SDK. I would take time to review it and leave my comments. Adding more reviewers to this PR. @grlima @littleaj please take some time to go over this. This is the kind of thing that we really wanted and is indeed big help.

maciejkopecpl · 2018-01-25T11:55:15Z

As a Spring Boot user, which is at the moment integrating Application Insights with it, I'm waiting for this enhancement very much :)

dhaval24 · 2018-02-12T02:54:27Z

@gavlyukovskiy I was experimenting with SpringBoot and it's realm of creating Uber jars. I tried an experiment where we can specify Java agent as maven dependency and then modifying some configurations to embed the agent inside the uber jar. However, we still get the class not found error. I will post a seperate issue on the steps taken to reach this stage. Let me know if you have worked with Agents in spring boot. Ideally I would love to have this starter also have java agent inside it so that user can get full end to end Java monitoring experience with just single dependency which can easily then be a part of Uber Jar (deployable units as said in SpringBoot)

timja

As a user looking for something like this, it looks great 👍

gavlyukovskiy · 2018-02-17T00:39:05Z

Added documentation and readme, waiting for review.
@dhaval24 Regarding agent I don't see a way to include agent in starter as spring boot doesn't support that, as I understood from spring-projects/spring-boot#6626 some hacks can be used but only in client's build script.

You can use sample project https://github.com/gavlyukovskiy/azure-application-insights-spring-boot-sample to test this starter end-to-end.

dhaval24 · 2018-02-17T00:55:27Z

Thanks @gavlyukovskiy for pushing the changes. We will do a deep review for this, as this would be a paradigm shift :-) adding more people to review

dhaval24

@gavlyukovskiy Initial overlook. Left few comments. Readme still needs to get final polish, after all code changes. I haven't checked behavioral aspect, will do it in next round of review.

dhaval24 · 2018-02-18T18:31:54Z

azure-application-insights-spring-boot-starter/build.gradle

+
+// region Publishing properties
+
+projectPomName = project.msftAppInsightsJavaSdk + " Spring Boot starter"


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.

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'

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.

dhaval24 · 2018-02-18T18:32:56Z

...main/java/com/microsoft/applicationinsights/boot/ApplicationInsightsModuleConfiguration.java

+            return new ProcessPerformanceCountersModule();
+        }
+        catch (Exception e) {
+            throw new IllegalStateException("Could not initialize Windows performance counters module", e);


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.

I will change this to logging error.

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.

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.

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

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?

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

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.

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

dhaval24 · 2018-02-18T18:33:39Z

azure-application-insights-spring-boot-starter/README.md

+# Logging level [all, trace, info, warn, error, off]
+azure.application-insights.logger.level=info
+
+# Enable/Disable default telemetry modules


What if user doesn't care to specify all these? By default are they turned on?

By default they are all turned on, added these properties just in case user wants to disable those.

Sure, it makes sense to specify all the available properties to tweak in the readme, however please make a note there that all these properties are turned on by default so that user can have an understanding.

I was thinking about calling them default already makes them turned on by default ;) I will add note that all those enabled by default

dhaval24 · 2018-02-18T18:34:10Z

azure-application-insights-spring-boot-starter/README.md

+#### Configure more parameters using `application.properties`
+```properties
+# Enable/Disable tracking
+azure.application-insights.enabled=true


Same question as below for defaults.

By default it's true and should not be specified to run application. Only single required property is azure.application-insights.instrumentation-key

Makes sense.

dhaval24 · 2018-02-18T18:40:40Z

azure-application-insights-spring-boot-starter/build.gradle

+
+archivesBaseName = 'azure-application-insights-spring-boot-starter'
+
+dependencies {


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

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.

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

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

dhaval24 · 2018-02-18T18:43:35Z

...rter/src/main/java/com/microsoft/applicationinsights/boot/ApplicationInsightsProperties.java

+        this.logger = logger;
+    }
+
+    public static class Logger {


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.

yes, there are defaults. Will change to ERROR

dhaval24 · 2018-02-18T18:46:01Z

...va/com/microsoft/applicationinsights/boot/ApplicationInsightsTelemetryAutoConfiguration.java

+@ConditionalOnClass(TelemetryConfiguration.class)
+@Import({
+        ApplicationInsightsModuleConfiguration.class,
+        ApplicationInsightsWebModuleConfiguration.class


What if the user's spring boot application is not a web application. This would be unnecessary import then correct? I think internally you can add a conditional here based on property (WebRequestTracking)

We can add @ConditionalOnWebApplciation on whole ApplicationInsightsWebModuleConfiguration,

@gavlyukovskiy Yes that is one way. However, there are instances when the User uses Application Insights only for pushing logs or pushing custom events and doesn't require HTTP tracing. In that scenario, I think this won't work. The better way can be to have a property which says disable/enable WebRequestTracking (By default it is set as enabled). and then condition on it. Thoughts?

WDYT about azure.application-insights.web.enabled=true/false as umbrella for all web modules? But we should state that

azure.application-insights.default-modules.WebRequestTrackingTelemetryModule.enabled=true azure.application-insights.default-modules.WebSessionTrackingTelemetryModule.enabled=true azure.application-insights.default-modules.WebUserTrackingTelemetryModule.enabled=true azure.application-insights.default-modules.WebPerformanceCounterModule.enabled=true azure.application-insights.default-modules.WebOperationIdTelemetryInitializer.enabled=true azure.application-insights.default-modules.WebOperationNameTelemetryInitializer.enabled=true azure.application-insights.default-modules.WebSessionTelemetryInitializer.enabled=true azure.application-insights.default-modules.WebUserTelemetryInitializer.enabled=true azure.application-insights.default-modules.WebUserAgentTelemetryInitializer.enabled=true

will be implicitly turned off.

Umbrella should provide an ease, but thinking more about it how would it affect if the user does some mess up and keeps the umbrella property turned off but enables one of the sub properties in the umbrella ? However, I agree this looks more clean.

I don't think we should merge it somehow, if you say web is disabled then every web module is disabled, if you want to disable some specific web module - go and do it, no need to disable whole web and then enable specific ones. There are only 9 of them :)

yes, this looks fine. If you disable the web then every web module is disabled or else, you can disable individual ones as needed.

dhaval24 · 2018-02-18T18:54:07Z

...va/com/microsoft/applicationinsights/boot/ApplicationInsightsTelemetryAutoConfiguration.java

+        return new TelemetryClient(configuration);
+    }
+
+    @Bean


We should not include this. This is the old style of sampling where as in SamplingV2 we use FixedRateTelemetryProcessor. It is the default and recommended way to sample

So this is not the TelemetrySampler instance but TelemetryProcessor. It actually means that we can't do it @ConditionalOnMissingBean and user won't be able to override this one, at least we can do it configurable through application.properties

Yes, the class com.microsoft.applicationinsights.internal.channel.samplingV2.FixedRateSamplingTelemetryProcessor is a BuiltInTelemetryProcessor. It gets plugged into telemetry processor pipeline. By default we can keep this sampling processor turned on with Sampling Rate of 100 (essentially no sampling) but of-course there should be way to remove the processor from the pipeline totally, if @ConditionalMissingBeanisn't the way for this we need to get this with the help of application.properties

Should be definitely in application.properties, @ConditionalMissingBean is conditional that checks that bean of this type was not provided by the user. For instance if you have few implementations of TelemetrySampler by default FixedRateTelemetrySampler will be used, but if user has defined own bean like this

@Bean public TelemetrySampler myTelemetrySampler() { return new MyTelemetrySampler(); }

our won't be created.

Yes, I do get that. In this case we should not keep this bean inside. And let user create a processor if needed. One question, if the user wants to use a built in processor, say fixed rate sampling telemetry processor, how does he do that? Can we specify it in the properties file? If yes can we configure different attributes of the processor too with Properties file?

We can add something like azure.application-insights.telemetry-processor.type in properties if we have limited number of available processors. The problem is that you may have multiple processors at the same time. We can create this property with allowed values [fixed_rate, foo, bar, none] and default will be fixed_rate. Tell me what you exactly need and I can find some ideas how we can implement this.

For example data source in spring boot configured like that, first you chose type of data source:

spring.datasource.type=<datasource class>

and based on the type you chose you can use appropriate set of properties that you can use based on which data source you have chosen. If you chose org.apache.tomcat.jdbc.pool.DataSource then you can use spring.datasource.tomcat.* to configure tomcat data source parameters; if com.zaxxer.hikari.HikariDataSource then properties spring.datasource.hikari.* are used.

Currently there is only FixedRateSampling Telemetry Processor for sampling. In future we would have adaptive sampling telemetry processor. Also you can look into this package com.microsoft.applicationinsights.internal.processor for other built in processors we have today. We can alteast create properties like what you mentioned for this built in processors. Does it make sense?

dhaval24 · 2018-02-18T19:02:35Z

.../java/com/microsoft/applicationinsights/boot/ApplicationInsightsWebMvcAutoConfiguration.java

+        registration.setFilter(webRequestTrackingFilter);
+        registration.addUrlPatterns("/*");
+        registration.setName("webRequestTrackingFilter");
+        registration.setOrder(Ordered.HIGHEST_PRECEDENCE + 10);


Why do we need HIGHEST_PRECEDENCE+10. All the AI WebRequestTrackingFilter needs is that it should be first in the Filter Chain. Are there any spring framework filters that run even before the highest precedence? Also a question - is filter the best way to intercept the request execution time and other details? We have noticed an issue with Async request which returns a future and starts execution in different thread. In that case AI behaves weirdly. #444

+10 just gives the user the ability to have some interceptor before ours. It may be useful in some cases for example when user want to log request or to measure overhead given by this filter. For instance we have filter called FinalCatchFilter which is the very first filter and logs any exception that was not handled in the chain.

Okay, however my question was more centered around by providing this kind of facility would it mess up the actual numbers of request execution time? If that is the case we should be very careful with this. I would probably say no to it if it messes up the metrics.

In general no one cares of order which is Ordered.LOWEST_PRECEDENCE by default. I see only two spring filters that is higher than WebRequestTrackingFilter -MetricsFilter and OrderedCharacterEncodingFilter, both of them are Ordered.HIGHEST_PRECEDENCE so you can't be sure that even with highest order yours win, also order you your concrete filter becomes non deterministic, which is not good thing.

If user specifies order that he is most probably know what is he doing and leaving some buffer is fine.

Preferably I would not touch it unless someone will give use case where this should be changed.

@gavlyukovskiy I am not saying that we should provide the handle for user to tweak the filter. I only want to ensure that AI filter gets the highest precedence whenever possible but as you said it can be non deterministic, keeping the highest precedence + 10 should be a good enough guess in most cases.

dhaval24 · 2018-02-18T19:07:46Z

.../java/com/microsoft/applicationinsights/boot/ApplicationInsightsWebMvcAutoConfiguration.java

+        return new WebRequestTrackingFilter(applicationName);
+    }
+
+    @Bean


This should also be conditional as far as I know. There should be a way to disable quick pulse if needed. In Standard xml implementation we have QuickPulse XML element which can be set to disabled. We need to have that property here too and then se this bean on conditional with that.

Oh, probably I missed this one, actually I had a way to disable quick pulse but then I removed it because it seemed to me useless thing, I will return it back.

Yes please do add. There should be ability to disable quick pulse if needed.

@dhaval24 should quick pulse depend on web application or it's applies to non web as well?

@gavlyukovskiy frankly, I do not know about this on the top of my head. I haven't worked on this part of the code base (QuickPulse), however from an end-user perspective QuickPulse / Live View on portal shows information such as Incoming request rate, request duration, request failure rate, CPU usage, Memory usage, Exception Rate, Dependency rate, dependency failure rate and Dependency call rate. Based on this it seems like QuickPulse should not have explicit dependency on web application. Only web related features like Request parameters should not be populated on the portal.

dhaval24 · 2018-02-19T20:14:18Z

@gavlyukovskiy have you looked at the retry PR #561 ? Have you addressed additional configuration parameters introduced there? I know at least one introduced there, which allows the user to configure the number of Instant retries the channel can make. Please also modify your code to allow that configuration property and also make sure that whatever properties were allowed to configure using InProcessTelemetryChannel are still intact.

Clarified all default values in readme; Changed default logging level to error; Added @ConditionalOnWebApplication for web configurations.

gavlyukovskiy · 2018-02-20T22:26:47Z

I fixed all issues that we had decision on. Only two things left:

Telemetry processors configuration including new sampling module.
Exception thrown from ProcessPerformanceCountersModule.

dhaval24 · 2018-02-22T23:28:30Z

.../java/com/microsoft/applicationinsights/boot/ApplicationInsightsWebMvcAutoConfiguration.java

-    public WebRequestTrackingFilter webRequestTrackingFilter(@Value("${spring.application.name:application}") String applicationName) {
+    public WebRequestTrackingFilter webRequestTrackingFilter(@Value("${spring.application.name:application}") String applicationName,
+            // we have implicit dependency on configured telemetryConfiguration here
+            @SuppressWarnings("unused") TelemetryConfiguration telemetryConfiguration) {


@gavlyukovskiy Can you explain the need of this? I do understand in the changes above you made this conditional on web application which is auto configured after the default configuration. Is this what you are referring to and believe that TelemetryConfiguration would always be initialized ?

@AutoConfigureAfter and @AutoConfigureBefore sets the order of creating bean definitions, but the beans itself are created in different order which controlled by dependency graph. I set explicit dependency on TelemetryConfiguration to force it to be created before WebRequestTrackingFilter to ensure that call TelemetryConfiguration#getTelemetryModules will return all modules.

@gavlyukovskiy thank you very much. Also about the sampling do you have any idea on getting that in?

I'm not sure I'm following the full flow how channel and and processor work together. I understood processor controls whether telemetry should be sent and channel has settings to control telemetry sending, right?

Yes so if processor returns false then the telemetry is dropped and doesn't hit transmission channel. So processor controls if telemetry should be sent or not. Channel is responsible for ultimately pumping the telemetry and it has several characterstics which tell what is the end point what is the buffer size etc.

And for what TelemetrySampler is used? Should we drop FixedRateTelemetrySampler?

Yes please drop it. It is not a recommended way to sample. We should mark it as deprecated also :-)

What do you think about moving these types of telemetry to enum:

Dependency

Event

Exception

PageView

Request

Trace

? With that we can make autocomplete in properties.

@gavlyukovskiy I am okay with this unless this is something globally exposed. Autocompletion would be desirable but I would like to understand what could be the implications if this is globally exposed because we won't be able to remove this until next major version and making it public then people can take explicit dependency on this enum and if we want to make it public in any situation then it should not reside in this processor but rather should have it's home somewhere else where it is more easy to locate and used in other parts of the code base.

dhaval24

@gavlyukovskiy on the surface the changes look fine except couple of things which you mentioned are left and I had also left one comment here. It would be more on testing the behavior and experience of this now and waiting for other's review.

…num for property auto complete

gavlyukovskiy · 2018-02-25T01:11:47Z

...oft/applicationinsights/internal/channel/samplingV2/FixedRateSamplingTelemetryProcessor.java

-        this.includedTypes = new HashSet<Class>();
-        this.excludedTypes = new HashSet<Class>();
-        try {
-            this.allowedTypes = new HashMap<String, Class>() {{


I little bit refactored two things here, first is double brace initialization which is not considered as good practice because of creating of anonymous class. Second is using .class instead of Class#forName, there is no reason to use this as all classes are present in the core module.

…d readme.

dhaval24

@gavlyukovskiy we might need to do this in better way.
I am more of thinking like refactoring the code base and moving all the constants to one single package.
The package can have then different final classes for constants. Like QuickPulseConstantClass, InProcessTelemetryChannelConstantClass and so on. We can then store these constant values as public static final constants. Look at this (https://en.wikipedia.org/wiki/Constant_interface) The alternative to Anti-Pattern. What is your thought here?
I would not like to create public constants inside the InprocessChannel itself because it is then exposed to API users and we might loose flexibility to update the values without following deprecating procedures.

This method also has some drawbacks like this but is much more neat and cleaner. @grlima @littleaj and @Dmitry-Matveev any suggestion here?

dhaval24 · 2018-03-03T16:05:49Z

@gavlyukovskiy changes regarding @dependsOn bean looks great. It will always make sure that QuickPulse is initialized after the telemetryConfiguration bean is initialized.

gavlyukovskiy · 2018-03-03T22:51:58Z

@dhaval24 I don't see how constants class will solve problem of changing defaults. Once you made some values as public they can be used regardless of its location and you will be in the same situation. Although I don't think changing value of a constant is a breaking change. Removing this constant would be breaking change, but I doubt one will rely on the specific value of some library constant.
As an alternative we can leave them private and just copy into starter defaults.

dhaval24 · 2018-03-03T22:55:54Z

@gavlyukovskiy I think let's just keep as it is right now (i.e public). I also think the same that changing value of constant would not be a breaking change, but I would still wait here for other reviewers to sign off on this. If we decide going public then we can refactor to move constants out in seperate package as I mentioned. I can start doing that refactoring if there is an agreement on that, or else we might just keep this as private and copy it into starter defaults for now. thoughts?

@grlima @littleaj and @Dmitry-Matveev I want your opinion on this too.

dhaval24 · 2018-03-11T18:59:54Z

@gavlyukovskiy could you please merge the latest master and push the changes back resolving the merge conflicts. Also what would be the best way if I want to commit on top of this. I think we might be able to create a PR against a seperate branch and then I will also work together to polish this out.

dhaval24 · 2018-03-11T19:29:13Z

@gavlyukovskiy we should also take a look at #422

dhaval24 · 2018-03-13T01:00:29Z

@gavlyukovskiy could you please change to create this PR against spring-boot-starter branch. That way it would easy for me to add commits on top of this and collaborate to make this final.

# 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

gavlyukovskiy · 2018-03-13T22:10:09Z

@dhaval24 I merged latest master into my branch and resolved all conflicts. I also changed pull request against your spring-boot-starter branch.

dhaval24 · 2018-03-13T22:15:50Z

azure-application-insights-spring-boot-starter/README.md

 azure.application-insights.channel.in-process.flush-interval-in-seconds=5
-# Size of disk that we can use. Default value: 10 megabytes.
+# Size of disk that we can use. Must be between 1 and 1000. Default value: 10 megabytes.


Does't the default and maximum value both seem to be too low. What do you think? Should we bump up this capacity?

Actually I don't really know what is this property about at all :) probably it's a question to @littleaj that have bumped maximum value from 100 to 1000 recently.

@gavlyukovskiy this property is used for disk persistence. When the network fails the telemetry gets stored in the disk and when it is restored we transmit that telemetry to the cloud.

@dhaval24 nice to know, added your explanation to the documentation :)

dhaval24 · 2018-03-13T22:16:45Z

azure-application-insights-spring-boot-starter/README.md

 azure.application-insights.channel.in-process.max-transmission-storage-files-capacity-in-mb=10
 # Enable/Disable throttling on sending telemetry data. Default value: true.
 azure.application-insights.channel.in-process.throttling=true

-# Percent of telemetry events that will be sent to Application Insights. Default value: 100% (all telemetry events).
+# Percent of telemetry events that will be sent to Application Insights. Must be between 0.0 and 100.0. Default value: 100 (all telemetry events).


We should add this comment here :
"All sampling percentage must be in a ratio of 100/N where N is a whole number (2, 3, 4, …). E.g. 50 for 1/2 or 33.33 for 1/3.
* Failure to follow this pattern can result in unexpected / incorrect computation of values in the portal."

This is important. We can only set specific sampling percentages for UI to be consistent

I didn't understand, can it be 2/3?

so 100/N = Sampling ratio.
Eg 100/2 = 50
100/3 = 33
100/4 = 25
100/5 = 20
100/10 = 10 etc.

probably we can create property for N instead? It would be less error prone if we say that N must be between 1 (100%) and 100 (1%).

what about 0%, is it valid?

just read the documentation for .NET, it can be 1000 (0.1%) as well. Looks really odd to me, I mean that 1/3 is not really 33 or 33.3 or 33.33, it's 33.(3) which you can properly specify in properties and for this reason per documentation I have to chose closest percentage to 100/N.

dhaval24 · 2018-03-13T22:17:02Z

...rter/src/main/java/com/microsoft/applicationinsights/boot/ApplicationInsightsProperties.java

@@ -243,7 +243,7 @@ public void setSampling(Sampling sampling) {

        static class Sampling {
            /**
-             * Percent of telemetry events that will be sent to Application Insights.
+             * Percent of telemetry events that will be sent to Application Insights. Must be between 0.0 and 100.0.


same sampling comment here.

dhaval24 · 2018-03-13T22:19:07Z

@gavlyukovskiy thank you for the modifying the PR. I would merge this in the branch and would be better to work on. I added few more comments. Nitty gritty ones.

gavlyukovskiy · 2018-03-14T08:34:25Z

Added note regarding sampling percentage.

Also I think it's important to clarify that users should not copy-paste all the properties unless they need to tweak something.
I'm seeing such behavior with my library, users just copy everything from readme and it's harder to change default values to something more optimal. E.g. if you change default transmission capacity it will not affect users that just copied properties from readme.

dhaval24 · 2018-03-15T17:50:36Z

@gavlyukovskiy @littleaj @grlima I am merging this PR in spring-boot-starter branch and we will continue to iterate there.

gavlyukovskiy · 2018-03-16T17:16:37Z

@dhaval24 don't forget to merge #516 as well, without it logging appenders will not send data to AI because loggers are initialized earlier and, most probably, had send at least single event at the moment when TelemetryConfiguration was initialized.

dhaval24 · 2018-03-16T17:18:38Z

@gavlyukovskiy thanks for reminding. Sure I will just merge that.

Added application insights starter for spring boot

9bc3711

dhaval24 requested review from littleaj and grlima January 9, 2018 01:06

dhaval24 added the Enhancement label Jan 9, 2018

dhaval24 assigned gavlyukovskiy Jan 9, 2018

dhaval24 self-requested a review January 9, 2018 01:19

dhaval24 added this to the future milestone Jan 27, 2018

timja reviewed Feb 13, 2018

View reviewed changes

Added a way to disable default modules, added documentation and readme

dd97415

gavlyukovskiy force-pushed the spring-boot-starter branch from 8168bdc to 59e25e3 Compare February 17, 2018 00:08

Polish readme

cce54ae

gavlyukovskiy force-pushed the spring-boot-starter branch from 59e25e3 to cce54ae Compare February 17, 2018 00:09

dhaval24 requested review from SergeyKanzhelev and Dmitry-Matveev February 17, 2018 00:55

dhaval24 reviewed Feb 18, 2018

View reviewed changes

Added properties to disable quick pulse integration, all web modules;

946f04a

Clarified all default values in readme; Changed default logging level to error; Added @ConditionalOnWebApplication for web configurations.

Fixed bean ordering issue

91f112b

dhaval24 reviewed Feb 22, 2018

View reviewed changes

dhaval24 mentioned this pull request Feb 23, 2018

POST /QuickPulseService.svc fails if instrumentation key is set programmatically #573

Closed

Added properties for channel and new sampler, created TelemetryType e…

abeff08

…num for property auto complete

gavlyukovskiy commented Feb 25, 2018

View reviewed changes

Added documentation to added properties, added default values, update…

e8f688f

…d readme.

Polish

afe1d0c

dhaval24 suggested changes Mar 3, 2018

View reviewed changes

jkschneider mentioned this pull request Mar 5, 2018

Support Azure Application Insights micrometer-metrics/micrometer#441

Closed

removed classification of 401s as successful (microsoft#606)

579eb5c

gavlyukovskiy mentioned this pull request Mar 13, 2018

Added check for TelemetryConfiguration#getInstrumentationKey emptiness #516

Merged

gavlyukovskiy changed the base branch from master to spring-boot-starter March 13, 2018 21:17

gavlyukovskiy force-pushed the spring-boot-starter branch from 45e915e to dcadfc1 Compare March 13, 2018 22:03

Updated documentation regarding configuration values window

62086f3

dhaval24 reviewed Mar 13, 2018

View reviewed changes

dhaval24 mentioned this pull request Mar 14, 2018

Failed to scan packages 'com.microsoft.applicationinsights' (When AI libraries are part of Uber SpringBoot Jar) #553

Closed

Updated documentation of properties

a4fda92

dhaval24 merged commit 77d88a6 into microsoft:spring-boot-starter Mar 15, 2018

dhaval24 mentioned this pull request Apr 12, 2018

Azure Application Insights SpringBoot Starter #646

Merged

3 tasks

grlima mentioned this pull request Oct 25, 2018

Fix the filter order in starter #766

Merged

gavlyukovskiy deleted the spring-boot-starter branch December 22, 2023 18:22


		// region Publishing properties

		projectPomName = project.msftAppInsightsJavaSdk + " Spring Boot starter"


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

		dependencies {

Added application insights starter for spring boot #518

Added application insights starter for spring boot #518

Conversation

gavlyukovskiy commented Jan 9, 2018 • edited Loading

dhaval24 commented Jan 9, 2018 • edited Loading

maciejkopecpl commented Jan 25, 2018

dhaval24 commented Feb 12, 2018

timja left a comment

Choose a reason for hiding this comment

gavlyukovskiy commented Feb 17, 2018 • edited Loading

dhaval24 commented Feb 17, 2018

dhaval24 left a 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

gavlyukovskiy Feb 22, 2018 • edited Loading

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

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

gavlyukovskiy Feb 19, 2018 • edited Loading

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 Feb 19, 2018

gavlyukovskiy commented Feb 20, 2018

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 Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavlyukovskiy Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 left a comment

Choose a reason for hiding this comment

gavlyukovskiy commented Jan 9, 2018 •

edited

Loading

dhaval24 commented Jan 9, 2018 •

edited

Loading

gavlyukovskiy commented Feb 17, 2018 •

edited

Loading

gavlyukovskiy Feb 22, 2018 •

edited

Loading

gavlyukovskiy Feb 19, 2018 •

edited

Loading

gavlyukovskiy Feb 25, 2018 •

edited

Loading

gavlyukovskiy Feb 25, 2018 •

edited

Loading