Add Heartbeat properties for App Services by d3r3kk · Pull Request #869 · microsoft/ApplicationInsights-dotnet-server

d3r3kk · 2018-03-27T23:14:06Z

Fix Issue #868.
Add heartbeat properties when App Services environment is detected. Keep App Services environment in sync using existing telemetry initializer used for same purpose.

I ran Unit Tests locally.
Changes in public surface reviewed
- No change to public API
Design discussion issue Enabling Heartbeat in App Services for .NET and .NET Core #868
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

…s) that will add some infrastructure-identifying properties. - Implements part of Option 1 in issue #868

…into WindowsServer. Update logic to ensure we keep heartbeat values updated. - Update logic in AzureWebAppRoleEnvironment to also signal AppServicesHeartbeat to update itself when changes are detected - Update AppServicesHeartbeat to expose an Instance variable to make it easier for update signals to be sent to it - Update CHANGELOG - Issue #868 discusses the need for the change to the AzureWebAppRoleEnvrionmeentInitializer

d3r3kk · 2018-03-28T16:28:51Z

@SergeyKanzhelev can you please have a look at this and let me know if this satisfies the topics we covered in issue #868? I believe I captured all the items discussed there, but I do still need to find a way to allow for updates in non-internal scenarios (AspNetCore).

SergeyKanzhelev · 2018-03-28T17:08:22Z

+    /// Provides default values for the heartbeat feature of Application Insights that
+    /// are specific to Azure App Services (Web Apps, Functions, etc...).
+    /// </summary>
+    internal class AppServicesHeartbeatTelemetryModule : ITelemetryModule, IDisposable


must be public so it can be instantiated from code

SergeyKanzhelev · 2018-03-28T17:08:36Z

+        /// <summary>
+        /// Initializes a new instance of the<see cref="AppServicesHeartbeatTelemetryModule" /> class.
+        /// </summary>
+        internal AppServicesHeartbeatTelemetryModule()


must be public so it can be instantiated from code

Done. Totally missed this one. Thanks.

SergeyKanzhelev · 2018-03-28T17:09:09Z

+
+        private static AppServicesHeartbeatTelemetryModule instance;
+        private object lockObject = new object();
+        private bool isInitialized = false;


does this double initialization control matter for this module? There is no harm in initializing twice. Furthermore - this protection in module creates a false perception that nobody will call Initialize twice. In reality it may easily happen with re-creating of entire TelemetryConfiguration

I don't need to protect it, no. I do need to know when it gets re-initialized but that's all.

SergeyKanzhelev · 2018-03-28T17:09:30Z

+            new KeyValuePair<string, string>("appSrv_wsHost", "WEBSITE_HOSTNAME")
+        };
+
+        private static AppServicesHeartbeatTelemetryModule instance;


why singleton?

To allow access to the 'Update' method. I don't have a better mechanism at this moment, but I think we can define a better one given some time.

SergeyKanzhelev · 2018-03-28T17:10:08Z

+        }
+
+        /// <summary>
+        /// Gets a value to provide internal access to the only instance of this class that *should* be available.


I don't understand why it is a singleton. Can you provide more details

We need a way to allow for the update of the heartbeat values associated to AppServices. I made this a singleton to allow other modules to signal this module to update the values. Perhaps there is a better mechanism for this, but I am not aware of one at this time. (I have thoughts on how to improve it in the future though, but it might take some time).

SergeyKanzhelev · 2018-03-28T17:13:54Z

+
+        [Event(34,
+            Message = "Could not gain access to the Heartbeat Manager instance during initialization. Exception raised: {0}",
+            Level = EventLevel.Verbose)]


Make it warning at least. This error is very unlikely to happen. It indicates that something is broken in SDK. So we want to have it at least at warning level.

Agreed. I've gone over the other calls and altered their levels (including some I had set previously for the Azure IMS data as well).

SergeyKanzhelev · 2018-03-28T17:15:28Z

+                        string hbeatKey = kvp.Key.ToString();
+                        if (isUpdateOperation)
+                        {
+                            hbeatManager.SetHeartbeatProperty(hbeatKey, hbeatValue);


Would you expect hbeatManager to be null ever?

Yes, I've added an early exit to counter this possibility.

SergeyKanzhelev · 2018-03-28T17:15:49Z

+                WindowsServerEventSource.Log.HeartbeatManagerAccessFailure(hearbeatManagerAccessException.Message);
+            }
+
+            return hbeatManager;


You do not check for null here. It may be a good idea though

Agreed. Done.

SergeyKanzhelev · 2018-03-28T17:17:06Z

+                try
+                {
+                    // get the variable, then expand it (otherwise we get the name we queried for in the value)
+                    string hbeatValue = Environment.GetEnvironmentVariable(kvp.Value);


this may throw security exception when it's not full trust appdomain. It is good idea to have a global try{}catch in the Initialize

Agreed. Done.

SergeyKanzhelev · 2018-03-28T17:18:52Z

I don't see how you are addressing synchronization with telemetry initializer and slot swap issue

d3r3kk · 2018-03-28T19:27:14Z

+            else if (!nodeName.Equals(this.lastNodeValue, StringComparison.Ordinal))
+            {
+                // if the AppServices heartbeat telemetry module exists, signal it to update the values in heartbeat
+                AppServicesHeartbeatTelemetryModule.Instance?.UpdateHeartbeatWithAppServiceEnvVarValues();


@SergeyKanzhelev, here is where I signal the AppServicesHeartbeatTelemetryModule to update the values in the heartbeat properties. This is the reason I chose to go with the Singleton pattern. Perhaps there is a better way to go about this?

- make new AppServicesHeartbeat module public! - add try/catch scenarios around initialize - add further diagnostic messages where appropriate

d3r3kk · 2018-03-30T05:31:15Z

@SergeyKanzhelev ping?

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

…, update code for testability.

d3r3kk · 2018-03-30T16:26:12Z

@SergeyKanzhelev @cijothomas updated PR with tests, can be merged without any need for follow up PR.

NOTE: I've added logic in this PR to update environment variables from the Initializer. That will work, but there is a danger that we would potentially miss updated environment variables (the value is potentially cached, please let me know if that is the case when using InternalContext as we do) and there might be perf issues with the way I propose here. Please see PR Add monitor to keep track of current value of App Services env vars #870 for an alternate that I'm a bit happier with.

…erviceEnvVar monitor

cijothomas

Please address comments. Also, it'd be great if you can provide info about why we need to send these values as HeartBeats again, since they are already sent with every telemetry item.

cijothomas · 2018-04-02T18:59:35Z

+            {
+                this.lastNodeValue = nodeName;
+            }
+            else if (!nodeName.Equals(this.lastNodeValue, StringComparison.Ordinal))


How does this work? nodename is cached and won't change after 1st initialization.

Agreed. Please see PR #870 where I fix this issue you call out here, and where I am adding some protection for performance as well. (My intention is to merge that PR into this one, as I prefer that solution).

Merged #870 here. Fixed.

cijothomas · 2018-04-02T19:04:44Z

+                envVars.Add(kvp.Value, Environment.GetEnvironmentVariable(kvp.Value));
+                if (string.IsNullOrEmpty(envVars[kvp.Value]))
+                {
+                    Environment.SetEnvironmentVariable(kvp.Value, kvp.Key);


The effect of setting the ENV variable persists even after the test? This may have unintended consequences in the next test.
If modifying any ENV, make sure to set it back to original after the test

Agreed. Also, in PR #870 see that I've added a ClassInitializer/ClassCleanup method to handle environment changes.

Merged #870 here where I fixed this issue by using unique env vars in place of the standard during testing. Note that I left the original testing for the TelemetryInitializer intact (although it really should be updated to do the same).

cijothomas · 2018-04-02T19:06:22Z

    <Import_RootNamespace>Microsoft.ApplicationInsights.WindowsServer</Import_RootNamespace>
  </PropertyGroup>
  <ItemGroup>
+    <Compile Include="$(MSBuildThisFileDirectory)AppServicesHeartbeatTests.cs" />


AppServicesHeartbeatTelemetryModuleTests might be better name.

cijothomas · 2018-04-02T19:13:38Z

+                {
+                    try
+                    {
+                        // get the variable, then expand it (otherwise we get the name we queried for in the value)


I am not sure i understood this. Why not just read ENV variable like done everywhere else?
https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/Src/WindowsServer/WindowsServer.Shared/AzureWebAppRoleEnvironmentTelemetryInitializer.cs#L65

Agreed. There doesn't ever seem to be any need to handle environment variable expansion for this value. PR #870 does not attempt expansion.

ExpandEnvironmentVariable will make this method slower, let's skip if unnecessary.

cijothomas · 2018-04-02T19:18:03Z

+        /// </summary>
+        internal static readonly KeyValuePair<string, string>[] WebHeartbeatPropertyNameEnvVarMap = new KeyValuePair<string, string>[]
+        {
+            new KeyValuePair<string, string>("appSrv_SiteName", "WEBSITE_SITE_NAME"),


Aren't these values appended to all telemetry items by the corresponding TelemetryInitializers? Why do we send these values (duplicated) into HeartBeat again?

Heartbeat and telemetry initializers are two separate solutions. I am not 100% aware of all the downstream consumers of the TelemetryInitializers, but I am keenly aware of the heartbeat ones (app 2 infra). I want the app 2 infra feature to only look in 1 single place for the values it requires. I believe the duplication caused by heartbeats (default 15-minute cadence) is minimal enough to support the end purpose.

cijothomas · 2018-04-02T19:19:42Z

+        /// Environment variables we use in our heartbeat payload.
+        /// </summary>
+        /// <returns>A value indicating whether or not an update to the environment variables occurred.</returns>
+        public bool UpdateHeartbeatWithAppServiceEnvVarValues()


Instead of a TelemetryInitializers telling this module to update, can this module update itself (re-read ENV vars) every 'n' minute? And maybe inform the TelemetryInitializer that ENV values changed so that it'll also pick new values.

Agreed. I was thinking that a subscriber model would work really nicely actually - that way the heartbeat module and the initializer module could simply 'subscribe' to the environment variable monitor (see PR #870, that's the terminology I use there) and get updated only as necessary.

For this PR, I wanted to keep it as concise as possible to get the job done. I would certainly be open to creating such a subscriber in our next sprint.

After discussions with Dmitry I will create an alternate PR to this one that contains a subscriber model solution. Although due to time constraints we still may opt for this solution.

cijothomas · 2018-04-02T19:19:57Z

+        /// <param name="configuration">Unused parameter.</param>
+        public void Initialize(TelemetryConfiguration configuration)
+        {
+            this.isInitialized = this.UpdateHeartbeatWithAppServiceEnvVarValues();


this.isInitialized -- Where is this used?

Line 94 - in the call to AddAppServiceEnvironmentVariablesToHeartbeat. The first time (isInitialized=false) we use AddHeartbeatProperty, each subsequent time we use SetHeartbeatProperty.

cijothomas · 2018-04-02T19:29:34Z

Need to ensure the tests are run in CI builds. I searched for newly added tests but couldn't find them in test results for this CI build.

…Server.Net45.Tests project

Dmitry-Matveev

This PR should be merged with #870 before going in to make sure that environment variables are updated accordingly. See my comments in #870 as well.

Dmitry-Matveev · 2018-04-02T23:56:00Z

+                {
+                    try
+                    {
+                        // get the variable, then expand it (otherwise we get the name we queried for in the value)


ExpandEnvironmentVariable will make this method slower, let's skip if unnecessary.

- 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

…ment variables

- 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

Updates cover all prior issues raised. Please re-review (or allow Dmitry to review)

- PR suggestion

d3r3kk · 2018-04-03T07:19:11Z

@cijothomas

Need to ensure the tests are run in CI builds. I searched for newly added tests but couldn't find them in test results for this CI build.

There was some iteration on builds as I added the tests, perhaps some of them got added as 'New Tests' in prior builds from the one you checked (I looked, and in the .1 build for that day there was 1 newly added test).

HOWEVER

Tests in the WindowsServer.Test.Shared project where not being added into the WindowsServer.Net45.Tests project at all. (WindowsServer.NetCore.Tests did load them, so they were still covered, just not fully). I have corrected this oversight in this PR as well.

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

d3r3kk · 2018-04-05T04:16:43Z

This PR is replaced by PR #873.

d3r3kk added 2 commits March 26, 2018 15:45

Add heartbeat provider for Azure App Services (in particular, Web App…

514aa15

…s) that will add some infrastructure-identifying properties. - Implements part of Option 1 in issue #868

d3r3kk requested review from Dmitry-Matveev, SergeyKanzhelev and cijothomas March 27, 2018 23:14

d3r3kk self-assigned this Mar 27, 2018

d3r3kk added the enhancement label Mar 27, 2018

d3r3kk added this to the 2.6-Beta3 milestone Mar 27, 2018

Fix stylecop issues

f2bd436

SergeyKanzhelev previously requested changes Mar 28, 2018

View reviewed changes

d3r3kk commented Mar 28, 2018

View reviewed changes

d3r3kk added 2 commits March 28, 2018 13:06

Update for PR suggestions

53fba65

- make new AppServicesHeartbeat module public! - add try/catch scenarios around initialize - add further diagnostic messages where appropriate

Fix for build - stylecop

710d4bb

d3r3kk closed this Mar 28, 2018

d3r3kk reopened this Mar 28, 2018

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 monitor to keep track of current value of App Services env vars #870

Closed

2 tasks

d3r3kk changed the title ~~[Do Not Merge] Add Heartbeat properties for App Services~~ Add Heartbeat properties for App Services Mar 30, 2018

Add tests, clean up AppServiceHearbeatModule class (unused variables)…

0c261da

…, update code for testability.

d3r3kk mentioned this pull request Mar 30, 2018

Ensure Heartbeat properties for App Services is included in AspNetCore microsoft/ApplicationInsights-aspnetcore#627

Merged

3 tasks

d3r3kk added 4 commits March 30, 2018 10:29

Fix for installed modules count test

fc44730

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 previously requested changes Apr 2, 2018

View reviewed changes

d3r3kk added 2 commits April 2, 2018 13:06

Re-add the WindowsServer.Shared.Tests shared project into the Windows…

3f31500

…Server.Net45.Tests project

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

cb857fb

Dmitry-Matveev reviewed Apr 2, 2018

View reviewed changes

d3r3kk added 4 commits April 2, 2018 16:59

Fix tests to allow for parallel testing of methods that alter environ…

1bbe3a2

…ment variables

Finish merging PR #869 and #870

da7e4d0

- 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

Cleanup of some comments and usings

275271a

Update file name for tests to match module being tested

09a28fb

- PR suggestion

d3r3kk closed this Apr 5, 2018

d3r3kk deleted the dekeeler/heartbeat-appservices branch April 5, 2018 04:16

Conversation

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

Uh oh!

Uh oh!

d3r3kk commented Mar 28, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d3r3kk commented Mar 30, 2018

Uh oh!

d3r3kk commented Mar 30, 2018

Uh oh!

cijothomas 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!

cijothomas Apr 2, 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!

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

d3r3kk commented Mar 27, 2018 •

edited

Loading

cijothomas Apr 2, 2018 •

edited

Loading