Prevent duplicate dependency collection when multiple apps run in the same process #1013
Conversation
CHANGELOG.md
Outdated
| @@ -1,5 +1,8 @@ | |||
| # Changelog | |||
|
|
|||
| ## Version 2.8.0-beta3 | |||
There was a problem hiding this comment.
please advise what is our plan for beta3 and whether i need to change it to 2.9-beta1
There was a problem hiding this comment.
there was an informal discussion about a beta3, but i think the plan is to go on to 2.9-beta1.
Lets get this cleared up during standup today.
| /// </summary> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| #if DEPENDENCY_COLLECTOR | ||
| public |
There was a problem hiding this comment.
why should this class be public?
There was a problem hiding this comment.
because we want to reuse it in the AspNetCore SDK for request deduplication
There was a problem hiding this comment.
Can we move this to the DependencyCollector project instead of Common?
The mixed accessibility levels seem wrong. :)
| // We should ignore events for all of them except one | ||
| if (!SubscriptionManager.IsActive(this)) | ||
| { | ||
| DependencyCollectorEventSource.Log.NotActiveListenerNoTracking(evnt.Key, Activity.Current?.Id); |
There was a problem hiding this comment.
this is a part of a solution. It ensures subscription only happens once and picks one "victim" to send requests thru.
I think better solution will be to check application name in the listener and only process requests that belong to the application that created the subscriber
There was a problem hiding this comment.
All applications created subscribers. All of them are valid. Moreover, any listener could die at any moment and we still need to report telemetry through the next victim.
The limitation of this is that all applications have to have the same config/ikey. We discussed it briefly in the microsoft/ApplicationInsights-aspnetcore#621 and everyone seems to be on board with it.
There was a problem hiding this comment.
Same configuration is a pretty heavy requirement if not for the nature of Service Fabric environment where you may want AI configuration out of the single host scope. This code, however, will run in non-service Fabric scenarios as well - do we have a scenario where a customer will encounter multiple listeners outside of SF? Will the requirement for same config still be fine in such a case?
There was a problem hiding this comment.
we have not heard so far about any customers running multihost stuff except for ServiceFabric.
I assume it could be possible for extra app for administration/monitoring which has different DI container and serves a different purpose than the main app. I assume they may want to have different configurations (e.g. telemetry initializers, app names and verstions), but not necessarily.
I think we should document limitations clearly. I'll send PRs to improve docs in AI for AspNetCore and SF repos.
I believe we are limited by nature of DiagnosticListener here (static singleton subscription) and similar design choices made in Application Insights SDK. This helps us with runtime discovery and subscriptions though. So, if we decide to really support multiple configurations in the same process, we should make a bigger investment. So, I'd like to hear user feedback and see if anyone is actually impacted by this limitation.
| } | ||
|
|
||
| [TestMethod] | ||
| public void AttachAndDetachMultiple() |
There was a problem hiding this comment.
Minor: This test does not seem to Detach().
| } | ||
|
|
||
| [TestMethod] | ||
| public void AttachNamyAndDetachActive() |
| /// Helps manage subsciptions in scenarios where multiple apps hosted in the same process. | ||
| /// </summary> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public class ActiveSubsciptionManager |
There was a problem hiding this comment.
Does it have to be public (even with EditorBrowsableState.Never)?
There was a problem hiding this comment.
yes, as I'm going to reuse it in AspNetCore SDK for request deduplication
| // We should ignore events for all of them except one | ||
| if (!SubscriptionManager.IsActive(this)) | ||
| { | ||
| DependencyCollectorEventSource.Log.NotActiveListenerNoTracking(evnt.Key, Activity.Current?.Id); |
There was a problem hiding this comment.
Same configuration is a pretty heavy requirement if not for the nature of Service Fabric environment where you may want AI configuration out of the single host scope. This code, however, will run in non-service Fabric scenarios as well - do we have a scenario where a customer will encounter multiple listeners outside of SF? Will the requirement for same config still be fine in such a case?
See microsoft/ApplicationInsights-aspnetcore#621.
ServiceFabric supports running multiple apps on the same host. Each app has its own AppInsights pipeline configured. i.e. there are multiple active dependency collectors and diagnostics listeners. Each of them receives even about dependency calls and tracks duplicates.
This change allows only one listener to be 'active': i.e. track telemetry.
When it is disposed, a new active listener is chosen between survivors.
New class
ActiveSubscriptionManagerwas introduced (public, but non-browsable), that I want to reuse in AspNetCore SDK to prevent duplicate requests collection.For significant contributions please make sure you have completed the following items:
Changes in public surface reviewed: NO
Design discussion issue #
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.
The PR will trigger build, unit tests, and functional tests automatically. If your PR was submitted from fork - mention one of committers to initiate the build for you.
If you want to to re-run the build/tests, the easiest way is to simply Close and Re-Open this same PR. (Just click 'close pull request' followed by 'open pull request' buttons at the bottom of the PR)
Please follow [these] (https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/CONTRIBUTING.md) instructions to build and test locally.