-
Notifications
You must be signed in to change notification settings - Fork 287
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
add etw log for missing instrumentation key #1331
Conversation
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.
Still have some doubts on emitting NoIkey message always - maybe, only in Release mode? See comments.
@@ -29,6 +30,11 @@ public void Process(ITelemetry item) | |||
{ | |||
TelemetryDebugWriter.WriteTelemetry(item); | |||
|
|||
if (string.IsNullOrWhiteSpace(item.Context.InstrumentationKey)) | |||
{ | |||
CoreEventSource.Log.TransmissionProcessorNoInstrumentationKey(); |
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.
Just to keep in mind - some custom channel implementations maybe OK with no instrumentation key, such as custom file writer to save telemetry locally.
Also, AI VS Output / AI VS Integration UX may not need IKey to power up its experiences - will we emit this message for those scenarios? These scenarios might be fairly common, we can put #if !debug here to avoid this message in VS/Debug.
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.
The #if !debug
would be a compile time switch. I don't think we can detect when a consumer is in debug mode
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.
Even if we do it, we should Log it once maybe, rather than for every item? Or maybe once in few seconds..
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.
@Dmitry-Matveev, I don't know how to balance the concern that an empty ikey could be valid. I don't know enough about these scenarios.
Do you think it's valid to optimize for the most common/expected scenarios?
@cijothomas, I agree for every item is too much, but I'm reluctant to log only once because I don't want the log to get missed. I can ask Raj for a second opinion on how frequent he thinks the log should be to get recognized.
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.
Something like Debugger.IsAttached
can be a better pick here - does not execute locally in VS, does execute when deployed to PROD. One notable issue - log won't be published if you debug in production, e.g. with WinDbg or remote debugging, but that should not matter already since you are capturing everything you need in debugger.
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.
@Dmitry-Matveev's comment about channels got me thinking about what if we only set this on our 1st party channels.
And I just discovered the log I'm adding already exists on both!
However, both channels restrict this to Verbose logs only.
I'm not sure if our troubleshooting guides are collecting this. I will investigate...
ApplicationInsights-dotnet/BASE/src/Microsoft.ApplicationInsights/Channel/InMemoryChannel.cs
Lines 136 to 144 in 7633ae8
if (string.IsNullOrEmpty(item.Context.InstrumentationKey)) | |
{ | |
if (CoreEventSource.IsVerboseEnabled) | |
{ | |
CoreEventSource.Log.ItemRejectedNoInstrumentationKey(item.ToString()); | |
} | |
return; | |
} |
ApplicationInsights-dotnet/BASE/src/ServerTelemetryChannel/ServerTelemetryChannel.cs
Lines 266 to 274 in 4ff3b7f
if (string.IsNullOrEmpty(item.Context.InstrumentationKey)) | |
{ | |
if (TelemetryChannelEventSource.IsVerboseEnabled) | |
{ | |
TelemetryChannelEventSource.Log.ItemRejectedNoInstrumentationKey(item.ToString()); | |
} | |
return; | |
} |
Include code to throttle log.
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.
We may want to improve our existing throttling mechanism for diagnostic events to check whether it can be lock free as this one.
#1260
If I telemetry item has made it all the way to the TelemetryChannel without an Ikey, it is safe to assume than an Ikey was never configured.
Here I want to emit a UserActionable ETW Error.
Note that ETW Event 56 is a config time warning, whereas ETW Event 57 is a channel time error.