Skip to content

add etw log for missing instrumentation key#1331

Merged
TimothyMothra merged 8 commits intodevelopfrom
tilee/etw_ikey
Dec 5, 2019
Merged

add etw log for missing instrumentation key#1331
TimothyMothra merged 8 commits intodevelopfrom
tilee/etw_ikey

Conversation

@TimothyMothra
Copy link
Copy Markdown

@TimothyMothra TimothyMothra commented Nov 27, 2019

#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.

@TimothyMothra TimothyMothra added this to the 2.12 milestone Nov 27, 2019
Copy link
Copy Markdown
Member

@Dmitry-Matveev Dmitry-Matveev left a 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.


if (string.IsNullOrWhiteSpace(item.Context.InstrumentationKey))
{
CoreEventSource.Log.TransmissionProcessorNoInstrumentationKey();
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Contributor

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..

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown
Member

@Dmitry-Matveev Dmitry-Matveev Dec 3, 2019

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.

Copy link
Copy Markdown
Author

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...

if (string.IsNullOrEmpty(item.Context.InstrumentationKey))
{
if (CoreEventSource.IsVerboseEnabled)
{
CoreEventSource.Log.ItemRejectedNoInstrumentationKey(item.ToString());
}
return;
}

if (string.IsNullOrEmpty(item.Context.InstrumentationKey))
{
if (TelemetryChannelEventSource.IsVerboseEnabled)
{
TelemetryChannelEventSource.Log.ItemRejectedNoInstrumentationKey(item.ToString());
}
return;
}

Timothy Mothra Lee added 2 commits December 3, 2019 15:51
Copy link
Copy Markdown
Member

@Dmitry-Matveev Dmitry-Matveev left a 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.

@TimothyMothra
Copy link
Copy Markdown
Author

#1483

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants