Added check for TelemetryConfiguration#getInstrumentationKey emptiness

[gavlyukovskiy]

Without this change it is impossible to load instrumentation key from active configuration if it was updated after TelemetryClient creation.
I need this in order to pick up instrumentation key that was set from spring configuration.

[dhaval24]

@gavlyukovskiy change looks fine to me. Just one question as you mentioned that you need to check if instrumentration key is updated after initialization, could there be a case when this (Strings.isNullOrEmpty(configuration.getInstrumentationKey()) && Strings.isNullOrEmpty(getContext().getInstrumentationKey())) condition return false but should return true. I mean in case of non spring boot environment?

[gavlyukovskiy]

I don't think it's real scenario, but it could happen when instrumentation key was not set at the moment of TelemetryClient instantiation and sending at least single event, then when instrumentation key is set on TelemetryConfiguration, new key won't be propagated into already created clients.

[dhaval24]

I don't know the ideal solution here. May be this might work for the time being as it is practically very rare for such a situation to happen. However, we might need to think on creating some kind of Factory for generating TelemetryClient objects instead of using new. In this way we might be able to make sure that instrumentation key and configuration value is always retained. @gavlyukovskiy thoughts?

[gavlyukovskiy]

As far as TelemetryClient uses active configuration I don't think with factory you can achieve something more, In other words if you want to validate that instrumentation key is present on the moment of TelemetryClient instantiation you don't need a factory right now. Although such validation directly harms spring boot integration - with spring boot many modules are initialized with empty key and I need all those clients to fetch new key after spring boot sets it into TelemetryConfiguration.

Anyhow this seems to me like a bug, because mechanism to reload key from TelemetryConfiguration is already exists on TelemetryClient.java:409:

if (Strings.isNullOrEmpty(ctx.getInstrumentationKey())) {
    ctx.setInstrumentationKey(configuration.getInstrumentationKey());
}

and this code is never executed because of isDisabled check in every track* method, that returns true if ctx.getInstrumentationKey() is empty.

[dhaval24]

@gavlyukovskiy If I understand this correctly - you are saying that most of the modules are initialized from SpringBoot with empty key where as TelemetryConfiguration would have Instrumentation key set by spring boot so the whole purpose then would be to inject the empty clients with key from configuration. In this context isDisabled() method is not useful and prevents the execution of TelemetryClient.java:409 so In your opinion isDisabled should only check for configuration.isDisabled to return true instead of also checking for instrumentation key?

[gavlyukovskiy]

Yes, this is the only reason basically. Honestly I don't follow intention of using TelemetryContext to store instrumentation key, so most probably checking instrumentation key on context itself is useless as well.

[dhaval24]

I think the reason behind storing instrumentation key in TelemetryContext is because this holds the meta-data which is sent to AI service with each telemetry instance and it also contains iKey. According to you this method should be trimmed to following

public boolean isDisabled() {
     return configuration.isTrackingDisabled();
}

correct? We might wan't to take that route eventually, but it would be better to play the safer game for the time being and go with the changes you have commited unless there is a clear understanding that we do not break anything with this change. Let me know your thoughts.

[littleaj]

@gavlyukovskiy , @dhaval24
Could a test or two be written which fails without this fix? Just to better show what workflow this enables.

[gavlyukovskiy]

@littleaj Hi, I added such test in PR #518 - ApplicationInsightsTelemetryAutoConfigurationTests#shouldReloadInstrumentationKeyOnTelemetryClient

Without spring boot specifics it looks like this:

TelemetryClient myClient = new TelemetryClient();

EventTelemetry eventTelemetry1 = new EventTelemetry("test1");
myClient.trackEvent(eventTelemetry1); // was not sent due to missing instrumentation key

TelemetryConfiguration.getActive().setInstrumentationKey("...");

EventTelemetry eventTelemetry2 = new EventTelemetry("test2");
myClient.trackEvent(eventTelemetry2); // was not sent because instrumentation key was not reloaded

[dhaval24]

@gavlyukovskiy I think it would make sense to move this PR against spring-boot-starter branch as well. Please merge latest master and with this changes and convert the PR to point to "spring-boot-starter" branch.