-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Read iKey from environment variable (APPINSIGHTS_INSTRUMENTATIONKEY) #315
Changes from 4 commits
715bf0a
9405d44
f1ad891
c022129
ac35f0f
75cd735
73933ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
using Microsoft.ApplicationInsights.DataContracts; | ||
using Microsoft.ApplicationInsights.Extensibility; | ||
using Microsoft.ApplicationInsights.Extensibility.Implementation; | ||
using Microsoft.ApplicationInsights.Extensibility.Implementation.Platform; | ||
using Microsoft.ApplicationInsights.Extensibility.Implementation.Tracing; | ||
|
||
/// <summary> | ||
|
@@ -19,6 +20,7 @@ | |
public sealed class TelemetryClient | ||
{ | ||
private const string VersionPrefix = "dotnet:"; | ||
private const string InstrumentationKeyWebSitesEnvironmentVariable = "APPINSIGHTS_INSTRUMENTATIONKEY"; | ||
|
||
private readonly TelemetryConfiguration configuration; | ||
private TelemetryContext context; | ||
|
@@ -371,6 +373,11 @@ public void Initialize(ITelemetry telemetry) | |
if (string.IsNullOrEmpty(instrumentationKey)) | ||
{ | ||
instrumentationKey = this.configuration.InstrumentationKey; | ||
|
||
if (string.IsNullOrEmpty(instrumentationKey)) | ||
{ | ||
instrumentationKey = PlatformSingleton.Current.GetEnvironmentVariable(InstrumentationKeyWebSitesEnvironmentVariable); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this ikey has the least priority? Is it what we want for Web Apps case? In Web Apps you can configure ikey in 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, priority order is correct for web sites (@iusafaro) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per offline discussion we will change the order so it is: code -> environment variable -> config |
||
} | ||
} | ||
|
||
var telemetryWithProperties = telemetry as ISupportProperties; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there maybe a .NET Core equivalent for getting environment variables we could use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something in Microsoft.Extensions.Configuration.EnvironmentVariables or Microsoft.Extensions.Configuration could be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).