Read iKey from environment variable (APPINSIGHTS_INSTRUMENTATIONKEY) #315
Conversation
|
|
||
| public string GetEnvironmentVariable(string name) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
Is there maybe a .NET Core equivalent for getting environment variables we could use?
There was a problem hiding this comment.
Maybe something in Microsoft.Extensions.Configuration.EnvironmentVariables or Microsoft.Extensions.Configuration could be used.
There was a problem hiding this comment.
Not in PCL. Those are .Net Core specific namespaces. If ever change target framework we can start supporting it.
There was a problem hiding this comment.
Good point, I keep thinking of .NET Core as a replacement for PCL but it is completely new and isn't portable backwards (as such).
|
|
||
| if (string.IsNullOrEmpty(instrumentationKey)) | ||
| { | ||
| instrumentationKey = PlatformSingleton.Current.GetEnvironmentVariable(InstrumentationKeyWebSitesEnvironmentVariable); |
There was a problem hiding this comment.
So this ikey has the least priority? Is it what we want for Web Apps case? In Web Apps you can configure ikey in AI.config file and then override it using Web App settings. Not other way around.
Also I'm concerned about performance of it. Maybe just read it once and cache? Or we need to support the scenario when environment variable will be changed later?
There was a problem hiding this comment.
I agree on caching, discussing the priority ordering, but I don't think it should change during runtime.
There was a problem hiding this comment.
Yes, priority order is correct for web sites (@iusafaro)
Resolved caching.
There was a problem hiding this comment.
@iusafaro, this priority order doesn't make sense to me. Can you pls elaborate why it is done this way?
There was a problem hiding this comment.
Per offline discussion we will change the order so it is: code -> environment variable -> config
Current coverage is 84.97% (diff: 55.55%)@@ develop #315 diff @@
==========================================
Files 137 137
Lines 5175 5184 +9
Methods 1084 1085 +1
Messages 0 0
Branches 752 755 +3
==========================================
+ Hits 4400 4405 +5
- Misses 471 473 +2
- Partials 304 306 +2
|
|
@iusafaro can you please ensure that we do not need to re-read instrumentation key from the environment variable after Web App swap? |
|
|
||
| if (string.IsNullOrEmpty(instrumentationKey)) | ||
| { | ||
| instrumentationKey = this.configuration.InstrumentationKey; |
There was a problem hiding this comment.
So we doing it before initializers have a chance to override it?
There was a problem hiding this comment.
Thinking about it - I guess it is fine
|
|
||
| if (string.IsNullOrEmpty(instrumentationKey)) | ||
| { | ||
| instrumentationKey = this.configuration.InstrumentationKey; |
There was a problem hiding this comment.
Thinking about it - I guess it is fine
microsoft/ApplicationInsights-dotnet-server#136