microsoft / ApplicationInsights-dotnet-server

Microsoft Application Insights for .NET Web Applications

https://azure.microsoft.com/services/application-insights/

133 stars 67 forks source link

Adding Performance counter collector support to Web App for Windows Containers #1167

Closed shibayan closed 5 years ago

shibayan commented 5 years ago

Fix Issue #676 Fixed the problem that performance counters are not collected in Web App for Windows Containers.

[x] I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

[ ] Changes in public surface reviewed
[ ] 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.

cijothomas commented 5 years ago

@shibayan Hi thanks for your contribution! Can you describe what the issue is and how is the fix helping with it?

shibayan commented 5 years ago

@cijothomas Thank you for replying!

Web App for Windows Containers can read performance counters directly. However, when the PerformanceCollectorModule finds the WEBSITE_SITE_NAME environment variable, it tries to use special PerformanceCollector for the normal App Service (Windows).

https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/01f38eb50ffc05bd4e60c28754e8f5672356a307/Src/PerformanceCollector/Perf.Shared/PerformanceCollectorModule.cs#L81-L82

Because of this process, the value of the performance counter could not be obtained on Windows Containers.

When WEBSITE_SKU isPremiumContainer, it can be determined that Web App for Windows Containers is running, so it was added to the condition.

cijothomas commented 5 years ago

@shibayan Thanks for explanation!

@SergeyKanzhelev can you add someone from AppServices team to confirm the above so that we can take a dependency of this behavior.

cijothomas commented 5 years ago

ping @SergeyKanzhelev

shibayan commented 5 years ago

Any updates?

cijothomas commented 5 years ago

@shibayan Thanks for reminding here. I will be talking internally to the person from Web App team, and will come back to this this week.

cijothomas commented 5 years ago

@shibayan I was told from App Service team that its safer to rely on the env variable XENON=1, to detect this. (SKUs might change in future.) Can you modify code to use XENON=1 as the criterion to detect Windows COntainer?

shibayan commented 5 years ago

@cijothomas Thanks! I first thought about using XENON, but I did not pass it to the container as an environment variable, so I wrote the code that uses the SKU.

Below is an App Service that I own, but XENON is not in an environment variable. https://aspnet-ltsc2019.azurewebsites.net/

Can I ask App Service Team to pass XENON to a container?

jvano commented 5 years ago

XENON environment variable is not passed to the container and it is only available in the SCM site. We are making a change to App Service to add a new environment variable called WEBSITE_ISOLATION.

When WEBSITE_ISOLATION="hyperv", it means that the website is executed in a Hyper-V Container.

It will take some time for this change to be propagated to production but feel free to incorporate this into the AI SDK.

Once this change is in prod, please remove the logic to detect Windows Containers using WEBSITE_SKU="PremiumContainer"

Thanks,

Joaquín

shibayan commented 5 years ago

@jvano I understood. Thank you! I will modify the Pull Request as taught to you.

cijothomas commented 5 years ago

@shibayan I will comeback to this soon. i am also working in this area and there are likely lot of conflicts.

cijothomas commented 5 years ago

@shibayan I will take this change as part of this PR https://github.com/microsoft/ApplicationInsights-dotnet-server/pull/1195

shibayan commented 5 years ago

@cijothomas That's great! I look forward to the release.

cijothomas commented 5 years ago

1195 is ready to merge. I will take this in that or a new follow up PR.

shibayan commented 5 years ago

Great work! Awesome!

shibayan commented 5 years ago

As I was able to achieve my original purpose, I will close this PR soon :)

cijothomas commented 5 years ago

closing as #1218 is merged now.Thanks