Add monitor to keep track of current value of App Services env vars by d3r3kk · Pull Request #870 · microsoft/ApplicationInsights-dotnet-server

d3r3kk · 2018-03-30T07:10:58Z

Corrects aspects of issue #868, further to PR #869.

Use to keep the value of WEBSITE_SITE_NAME and other App Services values current during runtime, as these values can change when swapping App Service applications from one slot to the next.

I ran Unit Tests locally.
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

Initial PR to bring discussion on the fix, and get some feedback before pushing to Heartbeat update as well.

- Use to keep the value of WEBSITE_SITE_NAME and other App Services values current during runime

pharring · 2018-03-30T17:59:38Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+    /// </summary>
+    internal static class AppServiceEnvVarMonitor
+    {
+        internal static TimeSpan CheckInterval = TimeSpan.FromSeconds(30);


Nit: private static readonly (unless you need to change it in unit tests.)

pharring · 2018-03-30T18:00:48Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        /// </summary>
+        private static void CheckVariablesIntermittent()
+        {
+            DateTime checkTime = DateTime.Now;


Always use DateTime.UtcNow to avoid an expensive local timezone lookup.

Done - good tip, thanks.

pharring · 2018-03-30T18:03:26Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        private static void CheckVariablesIntermittent()
+        {
+            DateTime checkTime = DateTime.Now;
+            if (checkTime - LastCheckTime > CheckInterval)


For bonus perf points, instead of LastCheckTime compute and store NextCheckTime (i.e. the time after which you should check again) and avoid the subtraction in the common case and make it an addition in the rare case.
i.e.

DateTime now = DateTime.UtcNow; if (now > NextCheckTime) { NextCheckTime = now + CheckInterval; // etc. }

Awesome. Done.

SergeyKanzhelev · 2018-03-30T18:04:37Z

@d3r3kk can you please confirm you actually tried it and this code works before I'll be reviewing it? From the fast glance this is NOT the fix that suppose to be working.

pharring · 2018-03-30T18:06:06Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+    internal static class AppServiceEnvVarMonitor
+    {
+        internal static TimeSpan CheckInterval = TimeSpan.FromSeconds(30);
+        internal static DateTime LastCheckTime = DateTime.MinValue;


Nit: private (unless you need to change it in unit tests.)

CheckInterval can be private, NextCheckTime internal to allow tests to work - added a line comment to call this intention out.

pharring · 2018-03-30T18:06:12Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        /// <summary>
+        /// Environment variables tracked by this monitor.
+        /// </summary>
+        internal static Dictionary<string, string> CheckedValues = new Dictionary<string, string>()


Nit: private static readonly (unless you need to change it in unit tests.)

Agreed, done. I am always thrown by the readonly keyword with containers...

pharring · 2018-03-30T18:06:41Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        {
+            { "WEBSITE_SITE_NAME", string.Empty },
+            { "WEBSITE_HOME_STAMPNAME", string.Empty },
+            {  "WEBSITE_HOSTNAME", string.Empty }


Nit: remove extra space (alignment)

Done, this was breaking the build (stylecop).

d3r3kk · 2018-03-30T18:17:26Z

Thanks for looking this over @pharring, exactly the feedback I was looking for (keeping perf in mind for this task is pretty important I think).

@SergeyKanzhelev I am looking for feedback if this is a decent idea to pursue or not. In principle it should work with the idea presented, but I understand if you need to keep your PR review time streamlined - apologies for the noise! I will ensure it works in a test now and ping you for feedback once I've proven it out further.

SergeyKanzhelev · 2018-03-30T18:29:10Z

I gave a link to PR in Azure functions that fixing this issue. Problem there is that sandbox doesn't update every env variable after swap. So I'd like you to make sure it works

…erviceEnvVar monitor

d3r3kk · 2018-03-30T23:13:18Z

@SergeyKanzhelev Please have a look at this now that I've got it a bit more put together. I've added some test methods to ensure they do what we want, and that the change satisfies the same demands the Web Functions PR did.

cijothomas · 2018-04-02T19:32:09Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        internal static DateTime NextCheckTime = DateTime.MinValue;
+
+        // how often we allow the code to re-check the environment
+        private static readonly TimeSpan CheckInterval = TimeSpan.FromSeconds(30);


Let user configure it?

Could do, not sure how much value would be there. I propose we raise an issue and chat about it for the next release.

Let's not overexpose API we are not certain in and may want to pull in the very next PR. We can make it configurable later if we stick with this solution, we cannot unmake public API once it's in stable release without the breaking change.

- Add heartbeat provider for Web Apps/App Services - Add monitor for App Services environment variables (which can change during the runtime of an AppServices app) - Ensure adequate testing is added, update current Env Var tests to be testable in a parallel environment

Dmitry-Matveev

Asynchronous solution would seem better rather than modifying values from Telemetry Initializer. However, this solution will work as well. Discussed options offline with Derek, More comments below.

Dmitry-Matveev · 2018-04-03T00:02:47Z

Src/WindowsServer/WindowsServer.Shared/AzureWebAppRoleEnvironmentTelemetryInitializer.cs

        {
-            return Environment.GetEnvironmentVariable(WebAppHostNameEnvironmentVariable) ?? string.Empty;
+            string nodeName = string.Empty;
+            AppServiceEnvVarMonitor.GetUpdatedEnvironmentVariable(WebAppHostNameEnvironmentVariable, ref nodeName);


This is no longer lazy initialized and we've added ~3 extra stack frames to every request/dependency in the customer app. Variable will be read synchronously every 30 seconds (may be fine if we do not Expand..() it, though).
We may think of an asynchronous approach here. Discussed offline with Derek.

Dmitry-Matveev · 2018-04-03T00:06:35Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        {
+            { "WEBSITE_SITE_NAME", string.Empty },
+            { "WEBSITE_HOME_STAMPNAME", string.Empty },
+            { "WEBSITE_HOSTNAME", string.Empty }


In comments for environment variable extraction in this PR, it's noted that WEBSITE_HOSTNAME is not a reliable source:

//...refactored so that it did not use WEBSITE_HOSTNAME, which is determined to be unreliable for functions during slot swaps.

Do we want to rely on that field, or we may use some tricks, e.g. from the same PR to build something more reliable from what we have.

I say we should resort to these tactics if we have an issue or have proof that App Services are affected by the same failing that Azure Functions are. I have put an issue together to verify that we can detect slot swap using only WEBSITE_HOSTNAME, and to update to using the WEBSITE_SITE_NAME + WEBSITE_SLOT_NAME if the need arises. Note that I've included the WEBSITE_SLOT_NAME in the list of environment variables that the monitor watches for now.

Dmitry-Matveev · 2018-04-03T00:07:47Z

Src/WindowsServer/WindowsServer.Shared/Implementation/AppServiceEnvVarMonitor.cs

+        internal static DateTime NextCheckTime = DateTime.MinValue;
+
+        // how often we allow the code to re-check the environment
+        private static readonly TimeSpan CheckInterval = TimeSpan.FromSeconds(30);


Let's not overexpose API we are not certain in and may want to pull in the very next PR. We can make it configurable later if we stick with this solution, we cannot unmake public API once it's in stable release without the breaking change.

- Use env var monitor to check for udpates to environment in controlled/minimal fashion - default to 30 second interval for checking the changes to environment - corrections for stylecop

d3r3kk · 2018-04-03T04:35:26Z

I have merged this change into PR #869. Please move comments and issues to that PR.

Add heartbeat provider for Azure App Services (in particular, Web Apps) that will add some infrastructure-identifying properties. - Solves issue #648, #868, and #872. - Both `AzureWebAppRoleEnvironment` and `AppServiceHeartbeatTelemetryModule` need to update their telemetry cache whenever the environment chances (ie, slot swap). `AppServiceEnvironmentVariableMonitor` solves this. - Keep the value of WEBSITE_SITE_NAME and other App Services values current during runtime - Merge PR #870 and #869, regarding heartbeat providers for App Services - Ensure adequate testing is added, update current Env Var tests to be testable in a parallel environment - `EnvironmentVariableMonitor` only updates the environment variables at set intervals, its subscribers only update when they need to. - Disable monitor if we don't detect that we are in Azure App Service environment, and on SecurityExceptions, disable the monitor altogether

Add monitor to keep track of current value of App Services env vars

d4a21c3

- Use to keep the value of WEBSITE_SITE_NAME and other App Services values current during runime

d3r3kk mentioned this pull request Mar 30, 2018

Add Heartbeat properties for App Services #869

Closed

4 tasks

pharring reviewed Mar 30, 2018

View reviewed changes

d3r3kk added 3 commits March 30, 2018 15:57

Correct perf issues and suggestions from PR, add tests

aab6721

Make container of env vars cached into a readonly member

86911c0

Update AzureWebAppRoleEnvironmentTelemetryInitializer to use the AppS…

9c407f9

…erviceEnvVar monitor

cijothomas reviewed Apr 2, 2018

View reviewed changes

d3r3kk added a commit that referenced this pull request Apr 2, 2018

Merge PR #869 and #870 into a single PR for submission to 2.6.0-beta3

cb857fb

Dmitry-Matveev reviewed Apr 3, 2018

View reviewed changes

d3r3kk closed this Apr 3, 2018

This was referenced Apr 3, 2018

Verify that App Services slot swaps are being detected in Web SDK #872

Closed

Add App Services Heartbeat with environment variable monitor #873

Merged

d3r3kk deleted the dekeeler/web-app-envvars branch April 5, 2018 04:21

Conversation

d3r3kk commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pharring Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pharring Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Mar 30, 2018

Uh oh!

SergeyKanzhelev commented Mar 30, 2018

Uh oh!

d3r3kk commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitry-Matveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Apr 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

d3r3kk commented Mar 30, 2018 •

edited

Loading

pharring Mar 30, 2018 •

edited

Loading

pharring Mar 30, 2018 •

edited

Loading