-
Notifications
You must be signed in to change notification settings - Fork 123
Request and dependency telemetry is tracked by each Webhost in the app #621
Comments
related to microsoft/ApplicationInsights-dotnet#613 |
@lmolkova Please let me know, if possible, when there is a preview nuget package available containing the fix - I will be happy to battle test it in a real Service Fabric environment. |
@mkosieradzki we have not decided on the version the fix for double-tracking will be included into. Once we have fix for NullReferenceException (#622). can we ask you try the preview from our myget feed? |
@lmolkova I would be happy to try it out. |
@mkosieradzki cool! I'll let you know once we have it |
Plan is to have 2.3.0-beta1 (with fix for NRE) released on April 1st week. Let me know if there is a need to get a private version even earlier. |
Any news on this one? |
@mkosieradzki Official release of 2.3.0-beta1 will be in 3-4 days. Thank you for your patience! |
We have published beta1 in myget. It should be in nuget.org Monday. I'll update when i push to nuget. |
@cijothomas @lmolkova Thanks a lot. I have uploaded it to my test cluster. It seems to be working, but I will have conclusive information in couple of days. |
@cijothomas @lmolkova This specific bug seems to be fixed. Now I am experiencing different issues with duplicate telemetry. I am trying to diagnose the exact reason, but long story short I am observing that if I open 4 listeners in my Service Fabric service all requests are reported 4 times. Each listener has its own WebHostBuilder and I will try to isolate the issue, |
Ok. Got a repro. Below is the telemetry generated by a single request with five listeners registered.
I have also attached a project reproducing this issue. BTW. If you take a closer looks properties are duplicated inside duplicate telemetries as well ;) |
@mkosieradzki we are aware of it, this #621 is exactly about it. |
@lmolkova You are right. Sorry for a duplicate report. Not crashing the process is really a lot ;), so thanks for fixing this so far. I am also facing duplicate telemetries when using ApplicationInsights serilog sink and .NET Core (Does not happen on .NET Framework). Each duplicate telemetry has a different SDK version, but I believe it's an issue with specific serilog sink, however I will investigate this one as well. |
Closing this as wontfix as we have no immediate plan to fix this. If the pattern of multiple webhosts in a single process is being used by lot of customers, we will revisit and reprioritize. |
@cijothomas this is quite common for Service Fabric deployments. Consider to re-open and investigate possible solutions |
I see. @lmolkova I think we had chatted about this where you mentioned this model is not so common/not encouraged in SF? Or was it someone else ... |
Some explanation:
So it's actually not only Shared Process, but Shared Process hosting model is really critical to Service Fabric scalability. Feel free to talk to the Service Fabric team, and they will explain the details. |
@mkosieradzki we discussed it with SF team. They changed VS templates to default to Exclusive process and they expect most customers to NOT use shared mode. @mikkelhegn could you please help us re-evaluate importance of this scenario? |
/cc @brahmnes |
Is this dead? Am I supposed to properly rewrite this SDK, but in a way it actually works, or what? |
@lmolkova That the SF team change the template to Exclusive process is important how? It might be that this is the most common scenario, but as @mkosieradzki points out, there are legit use cases for the shared process model - and doubly so for multiple listeners inside a single process. We currently have this issue with multiple listeners (more than two actually), and we were considering having a webhost per tenant for one of our services - but so far we are holding back on it due to the insights concern. |
@esbenbach @mkosieradzki Could you help us with following: assuming multiple hosts run in the same process: would they have the same configuration? Would you need to have different instrumentation keys for them or just any other piece of configuration: TelemetryInitializers, TelemetryProcessors, etc? |
Thanks for reopening the issue. Single telemetry configuration is more than enough. In a perfect solution, telemetry configuration can be set-up outside of the WebHost - because WebHost has a shorter lifetime than Service Fabric host process. I would be happy to integrate this solution with Service Fabric SDK... Some scenarios:
However there are scenarios where there are no active listeners and you want to have telemetry enabled… like for example:
|
In our particular case a single cfg instance is enough. There is probably a case to be made for separate cfgs but im unable to come up with a sane example where it is useful |
Bump! It has been three months... |
Heads up! I'm working on the fix. |
Just wanted to add that this is also problem in my company. We are using shared process hosting extensively to ensure future scalability and this issue is problematic for us. In my case similarly as stated above single configuration is ok. |
I am thinking this is related:
Null Exception. |
@lmolkova Thank you very much for fixing this. It works really nice now. I can use Application Insights again! |
@pksorensen I believe this is microsoft/ApplicationInsights-dotnet#613. Please update your AppInsights and check if the issue goes away. |
Yes @lmolkova - looks like updating solved it. |
Multiple WebHosts could run in the same process.
This scenario is possible with any ASP.NET Core app and when using the Shared Process hosting model in ServiceFabric
In this case AppInsights services are added to each service provider, however, singletons that track requests and dependencies are static and process-wide - they are not aware of WebHost boundaries.
So the requests and dependencies are tracked by multiple DiagnostiSource subscribers - one per web host.
Repro Steps
Ignored functional test in AspNetCore2.0 app will be added shortly
Actual Behavior
each webhost tracks requests and depedencies of all running web hosts
Expected Behavior
We should not re-create diagnostic listeners if there is an active one.
This blocks scenario with web hosts from having different configurations: imagine a different set of ikeys or telmetryinitializers/processors. I'm not sure it's feasible to support this scenario (even if feasible, it's much harder to implement), in this case, there could a listener per webhost, but the listener must only track its own webhost telemetry.
The text was updated successfully, but these errors were encountered: