Add Azure Application Insights connection string support

[cbismuth]

This implementation overrides instrumentation key when connection string is set from the AzureMonitorConfig class.

Fixes #3710.

[shakuzen]

This test is no longer effective because telemetryConfiguration.setInstrumentationKey is being set to the same value as the AzureMonitorConfig.instrumentationKey so the assertion can't know whether the value came from AzureMonitorConfig or telemetryConfiguration.setInstrumentationKey.

Same issue for the test useTelemetryConfigConnectionStringWhenSet

[cbismuth]

Hi @shakuzen, here is a new proposal with instrumentation key deprecation and connection string as the new default implementation, no backward compatibility breakage. What do you think of this? thanks.

[shakuzen]

Thank you for the updated changes. I think we're moving in the right direction. While things are API compatible with the changes as is, a user upgrading to these changes will not be able to use this without making changes to their configuration. I think we can provide a smoother and more gradual upgrade path. What do you think of the following idea? If the user sets connectionString, we use that. If the user does not set connectionString but does set instrumentationKey (users upgrading to the version with these changes will be in this situation initially), we will by default return from connectionString InstrumentationKey= concatenated with the instrumentationKey. If neither are set, then a validation error should be thrown. We should have unit tests for these different cases. In order to prove the easy upgrade path for users, all the existing tests should pass as-is. We can add additional tests for new behavior.

[cbismuth]

Thanks for your time and patience @shakuzen, here is a new proposal which defaults to connection string when instrumentation key is set. Tests have been updated accordingly to check each use case. Am I getting closer? thanks

[shakuzen]

We're getting closer. Thanks for keeping at it. I think it would be more robust to do the mentioned logic in AzureMonitorConfig rather than in the constructor for AzureMonitorMeterRegistry. I was thinking something like the following for AzureMonitorConfig#connectionString:

@Nullable
default String connectionString() {
    return getSecret(this, "connectionString").orElseGet(() -> {
        String instrumentationKey = instrumentationKey();
        if (instrumentationKey == null) {
            return null;
        }
        return "InstrumentationKey=" + instrumentationKey;
    });
}

That should hopefully result in simpler logic in the constructor where it only needs to worry about checking and using config.connectionString(). What do you think?

Since we accept a TelemetryConfiguration instance in the constructor for AzureMonitorMeterRegistry, we want to respect any configuration already on that, only setting things from AzureMonitorConfig if not already set there. The question is, if there is an instrumentationKey set on TelemetryConfiguration but no connectionString, should we set the connectionString from AzureMonitorConfig? I'm not sure, but I am leaning toward yes.

[cbismuth]

Yes you're right @shakuzen, this is far more readable, thanks for pointing this.

[shakuzen]

Thank you for the pull request and being so responsive to feedback. We're ready to merge this. I will polish some things in a follow-up commit after merging. Have you had a chance to try this out with an application just to verify the new configuration is working with sending metrics to Application Insights? If not, please do try out snapshots when they are published post-merge. Thank you again!

[shakuzen]

I don't know why the CI build is having trouble running, but I ran the build locally on this pull request and it passed, so I'll merge anyway.

[cbismuth]

Thanks @shakuzen! I didn't manage to make Circle CI build work on my side neither. I'll try this locally by the end of the week, no worries, I'll let you know, have a nice day.