Skip to content
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

Merged
merged 8 commits into from
Dec 5, 2019
Merged

Conversation

TimothyMothra
Copy link
Member

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

@@ -29,6 +30,11 @@ public void Process(ITelemetry item)
{
TelemetryDebugWriter.WriteTelemetry(item);

if (string.IsNullOrWhiteSpace(item.Context.InstrumentationKey))
{
CoreEventSource.Log.TransmissionProcessorNoInstrumentationKey();
Copy link
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
Member 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
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
Member 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
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
Member 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;
}

Copy link
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
Member Author

#1483

@TimothyMothra TimothyMothra merged commit 8e9c396 into develop Dec 5, 2019
@TimothyMothra TimothyMothra deleted the tilee/etw_ikey branch December 5, 2019 21:30
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