Programmatic configuration for TelemetryConfiguration #461

gavlyukovskiy · 2017-11-07T09:04:18Z

Fix #359, revisit #411.

Adds a way to configure TelemetryConfiguration from outside application insights library.
It is a must for spring boot integration because you can retrieve configuration only from spring context itself.
Whereas the same can be achieved by creating yet another @Configuration class, just like InterceptorRegistry, and asking user to import that one, it also can be accomplished by creating a spring boot starter with our full control of context initialization order.
I've prepared such starter that links with my forked version of sdk and once is added, requires only single property azure.application-insights.instrumentation-key to be set to integrate spring boot application with Application Insights.
As well allows you to use spring context to manage TelemetryConfiguration components - you can define any telemetry module, channel or sampler as @Bean and it will be used inside TelemetryConfiguration. Also it adds nice spring boot feature such like configuration metadata -
when IDE shows you the list of available configuration properties.

Conceptually all changes are backward compatible and only concern is that all modules that are using TelemetryConfiguration#getActive() must be initialized after TelemetryConfiguration is created by spring context, which is not possible for logger appenders, that's why I added lazy initialization inside logback appender (log4j and log4j2 can be modified in the same way) which leads to loss of initial log messages. If you think it is critical I can implement some kind of a buffer that will hold telemetry events until configuration is initialized.

You can look at my starter project to find more details. Also I can upload my sample project that uses forked version of sdk and the starter.

dhaval24 · 2017-11-07T16:22:33Z

@gavlyukovskiy Thank you very much for your contribution towards enabling spring boot support with Application Insights. Amazing work! I have added reviewers to your pull request and we will review it. Lets have a conversation around this and we shall take it from there. On a brief overview I feel this is a good concept.

SergeyKanzhelev

I left a comment. Maybe I do not understand the whole context - will be happy to learn more

SergeyKanzhelev · 2017-11-07T16:22:41Z

core/src/main/java/com/microsoft/applicationinsights/TelemetryConfiguration.java

+     * Telemetry configuration must not be set twice explicitly or through calling {@link TelemetryConfiguration#getActive()}.
+     *
+     * @param telemetryConfiguration preconfigured instance of Telemetry Configuration
+     */


We found it messy to allow to replace the configuration object. As you mentioned in comments one of the modules may get a hold of an object and will not pick up an update. Would it be better to update properties of an object instead?

With dependency injection we would simply initialize singleton of dependency injection with the value of telemetryconfiguration.active

That's why I added throw to be sure that configuration is initialized only once and no module is initialized with wrong TelemetryConfiguration. Probably configuration could be changed once context is loaded but I'm afraid that some module can get properties from the TelemetryConfiguration when those properties are not set yet - like WebRequestTrackingFilter#registerWebApp that is executed only once.

SergeyKanzhelev · 2017-11-07T16:23:05Z

core/src/main/java/com/microsoft/applicationinsights/TelemetryConfiguration.java

+     */
+    public static void setActive(TelemetryConfiguration telemetryConfiguration) {
+        synchronized (s_lock) {
+            if (initialized) {


Public APIs do not throw in Application Insights SDK

I agree with Sergey's point here. Throwing an exception in public API will crash the customers application and prevent it from starting. It is not a desired behavior.

I would agree with you about runtime exceptions, but this one is startup exception, where, from my point of view, is better to get exception when something goes wrong rather than found that telemetry is not working after application is started. But if this is a strong sdk convention, I can just log error into logs.

dhaval24

I have added some common observations and comments. We can discuss this in detail.

dhaval24 · 2017-11-07T17:28:40Z

core/src/main/java/com/microsoft/applicationinsights/TelemetryConfiguration.java

+     */
+    public static void setActive(TelemetryConfiguration telemetryConfiguration) {
+        synchronized (s_lock) {
+            if (initialized) {


I agree with Sergey's point here. Throwing an exception in public API will crash the customers application and prevent it from starting. It is not a desired behavior.

dhaval24 · 2017-11-07T17:33:54Z

core/src/main/java/com/microsoft/applicationinsights/TelemetryConfiguration.java

+    public static void setActive(TelemetryConfiguration telemetryConfiguration) {
+        synchronized (s_lock) {
+            if (initialized) {
+                throw new IllegalStateException("Telemetry configuration is already initialized. " +


Can we some how prevent this behavior programaticallly? If some one tries to call setActive() twice the second call is ignored?

Answered in previous thread, this is probably an option, but I wouldn't implement it like that, it is too dangerous and hard to debug if something goes wrong, it should not be initialized more than once.

dhaval24 · 2017-11-07T17:38:39Z

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

+                name = telemetryConfiguration.getApplicationName();
+            }
+            else if (CommonUtils.isNullOrEmpty(contextPath)) {
                URL[] jarPaths = ((URLClassLoader) (this.getClass().getClassLoader())).getURLs();


This is an issue with JDK 9. The default class loader of JDK 9 is not an instance of URLClassLoader but is instead called "App-Classloader". This call to getURLs() would fail here. Do we have a work around for this ? Have you tried this on a Spring Boot Project with JDK 9?

I haven't tried it on JDK 9, but probably ClassLoader#getResources/ClassLoader#getSystemResources will do the job.

dhaval24 · 2017-11-07T17:40:09Z

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

-            if (CommonUtils.isNullOrEmpty(contextPath)) {
+            TelemetryConfiguration telemetryConfiguration = TelemetryConfiguration.getActive();
+            if (telemetryConfiguration.getApplicationName() != null) {
+                name = telemetryConfiguration.getApplicationName();


Do we need a check here if someone has tried setting application name as null?

Thanks, I will add check into TelemetryConfiguration#setApplicationName that name is not null or empty.

gavlyukovskiy · 2017-11-07T21:10:40Z

I created repo with sample project here it shows how it is easy to start with starter in spring boot application.

dhaval24 requested review from Dmitry-Matveev, SergeyKanzhelev, dhaval24, grlima and littleaj November 7, 2017 16:10

SergeyKanzhelev suggested changes Nov 7, 2017

View reviewed changes

dhaval24 suggested changes Nov 7, 2017

View reviewed changes

dhaval24 added the Enhancement label Nov 8, 2017

dhaval24 assigned gavlyukovskiy Nov 17, 2017

This was referenced Nov 30, 2017

Dhdoshi/spring boot name bug #411

Closed

Not possible to register WebApp name/key properly when using WebRequestTrackingFilter in Spring boot #359

Closed

gavlyukovskiy closed this Dec 9, 2017

gavlyukovskiy force-pushed the master branch from fb62de5 to e17a2f9 Compare December 9, 2017 23:09

dhaval24 added this to the 2.0.0 milestone Dec 27, 2017

Programmatic configuration for TelemetryConfiguration #461

Programmatic configuration for TelemetryConfiguration #461

Uh oh!

Conversation

gavlyukovskiy commented Nov 7, 2017

Uh oh!

dhaval24 commented Nov 7, 2017

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhaval24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavlyukovskiy Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavlyukovskiy commented Nov 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gavlyukovskiy Nov 7, 2017 •

edited

Loading