Adding Performance counter collector support to Web App for Windows Containers

[shibayan]

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

• 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]

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

[shibayan]

@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 is PremiumContainer, it can be determined that Web App for Windows Containers is running, so it was added to the condition.

[cijothomas]

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

ping @SergeyKanzhelev

[shibayan]

Any updates?

[cijothomas]

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

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

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

Hi

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]

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

[cijothomas]

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

[cijothomas]

@shibayan I will take this change as part of this PR #1195

[shibayan]

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

[cijothomas]

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

[shibayan]

Great work! Awesome!

[shibayan]

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

[cijothomas]

closing as #1218 is merged now.Thanks