Changed the logic for loading the iKey from an environment variable to not override config or code settings. #379
Conversation
…o not override config or code settings.
| Assert.DoesNotThrow(() => client.TrackTrace(message)); | ||
| Assert.Equal(expectedKey, sentTelemetry.Context.InstrumentationKey); | ||
|
|
||
| // Set in code method two. |
There was a problem hiding this comment.
Please split into two unit tests. Generally we are trying to keep one test for the single scenario/check
There was a problem hiding this comment.
I did that originally, it was just a lot of copied and pasted code, and the test naming got weirder but I can do it again.
| Assert.Equal(expectedKey, sentTelemetry.Context.InstrumentationKey); | ||
|
|
||
| Environment.SetEnvironmentVariable("APPINSIGHTS_INSTRUMENTATIONKEY", null); | ||
| TelemetryConfiguration.Active.InstrumentationKey = oldActiveKey; // Restore state to not impact other tests. |
There was a problem hiding this comment.
You called 'ClearActiveTelemetryConfiguration' in the beginning. So you still need to restore keys?
There was a problem hiding this comment.
I'm hoping to avoid going through and finding and fixing any more tests that fail from circumstantial ordering that happen to depend on previous tests setting context for them.
| // If no configuration override was passed to the constructor then the default configuration instance will have been used. | ||
| // If no iKey was present in the config then the environment variable will have been loaded into the default instance. | ||
| instrumentationKey = this.configuration.InstrumentationKey; | ||
| if (string.IsNullOrEmpty(instrumentationKey) && this.configuration != TelemetryConfiguration.Active) |
There was a problem hiding this comment.
Do we need this logic at all? Do we have test for non-Active config and why do we need to use environment variable for it?
There was a problem hiding this comment.
I think we need this logic because if you specify a custom config with no iKey it won't pickup from the environment variable but this way provides a fallback when trying to send telemetry.
| // If no configuration override was passed to the constructor then the default configuration instance will have been used. | ||
| // If no iKey was present in the config then the environment variable will have been loaded into the default instance. | ||
| instrumentationKey = this.configuration.InstrumentationKey; | ||
| if (string.IsNullOrEmpty(instrumentationKey) && this.configuration != TelemetryConfiguration.Active) |
There was a problem hiding this comment.
So the decision is to use the ikey from the ApplicaitonInsights.config to take priority? TelemtryConfigurationFactory seems to kept this logic. Can you please add a test for this case or just point to it
There was a problem hiding this comment.
I believe that the test which I modified covers this but we can figure out how to make it clearer.
… iKey. Fixed and removed some unit tests.
No description provided.