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

Fix AzureSdkDiagnosticListener from crashing user app #2293

Closed
wants to merge 2 commits into from

Conversation

cijothomas
Copy link
Contributor

AzureSdkDiagnosticListener component within DependencyTrackingTelemetryModule currently instantiates a new TelemetryClient, whenever a new Listener is created. (any statement like new DiagnosticListener("Azure.somename") will trigger this. All Azure sdks does this inside them)

In the event that the telemetryconfig used to initialize the module is disposed, but the DependencyTrackingTelemetryModule itself is not disposed, any attempt to create new AzureSdkListener() will result in an unhandled exception from the SDK, as AzureSdkDiagnosticListener will try to instantiate a new TelemetryClient using a disposed config.

This violates the SDK's error handling policy of never crashing customer application after a successful initialization.

This PR modifies AzureSdk listener to create a single TelemetryClient during initialization, and re-use it for every subsequent AzureSdkListeners. If config is disposed, telemetry is not tracked (must be obvious!), but this PR also prevents the unhandled exception.

Issues like #2195 will be fixed with this PR. However, the user code which is disposing telemetry config, without disposing the DependencyTrackingTelemetryModule must be fixed - but this is not a SDK bug, but user code issue and must handled by end users.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
haxscramper haxscramper
listener.StartActivity(sendActivity, null);
listener.StopActivity(sendActivity, null);

var listenerAnother = new DiagnosticListener("Azure.AnotherClient");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these statements represent customer code (typically when they use some Azure sdks) which would have thrown unhandled exception, without the fix in this PR.
This would have resulted in exception, only if the DiagnosticListener was created after SDK initialization, because any DiagnosticListener created before SDK initialization will get callback within the subscribe code itself, which is try..catch..wrapped. (https://github.com/microsoft/ApplicationInsights-dotnet/blob/develop/WEB/Src/DependencyCollector/DependencyCollector/Implementation/DiagnosticSourceListenerBase.cs#L52-L58).

Any listener created afterwards goes here (https://github.com/microsoft/ApplicationInsights-dotnet/blob/develop/WEB/Src/DependencyCollector/DependencyCollector/Implementation/DiagnosticSourceListenerBase.cs#L72-L97) which has no try..catch. Will do a follow up to make sure SDK is protected in all those scenarios as well.

@cijothomas
Copy link
Contributor Author

cijothomas commented Jun 2, 2021

clsoing as this got lot of accidnetal changes.

@cijothomas cijothomas closed this Jun 2, 2021
@cijothomas cijothomas deleted the cijothomas/fixazuresdkerror branch June 2, 2021 04:44
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.

None yet

1 participant