Merge pull request #4445 from Particular/john/trackbutleaveone

Ensuring we leave at least one unhealthy instance is not automatically deleted for untracked endpoints

Author: johnsimons

src/ServiceControl.Persistence/EndpointsView.cs

@@ -1,7 +1,6 @@
 namespace ServiceControl.Persistence
 {
     using System;
-    using System.Text.Json.Serialization;
 
     public class EndpointsView
     {
@@ -12,6 +11,5 @@ public class EndpointsView
         public bool MonitorHeartbeat { get; set; }
         public HeartbeatInformation HeartbeatInformation { get; set; }
         public bool IsSendingHeartbeats { get; set; }
-        [JsonIgnore] public bool IsNotSendingHeartbeats { get; set; }
     }
 }

src/ServiceControl.UnitTests/Monitoring/HeartbeatEndpointSettingsSyncHostedServiceTests.cs

@@ -134,16 +134,18 @@ public async Task
         const string endpointName1 = "Sales";
         Guid instanceA = DeterministicGuid.MakeId(endpointName1, "A");
         Guid instanceB = DeterministicGuid.MakeId(endpointName1, "B");
+        Guid instanceC = DeterministicGuid.MakeId(endpointName1, "C");
         var mockMonitoringDataStore = new MockMonitoringDataStore(
             [new KnownEndpoint { EndpointDetails = new EndpointDetails { Name = endpointName1 } }]);
         var mockEndpointInstanceMonitoring = new MockEndpointInstanceMonitoring([
-            new EndpointsView { IsNotSendingHeartbeats = true, Name = endpointName1, Id = instanceA },
-            new EndpointsView { IsNotSendingHeartbeats = true, Name = endpointName1, Id = instanceB },
+            new EndpointsView { IsSendingHeartbeats = false, Name = endpointName1, Id = instanceA },
+            new EndpointsView { IsSendingHeartbeats = false, Name = endpointName1, Id = instanceB },
+            new EndpointsView { IsSendingHeartbeats = false, Name = endpointName1, Id = instanceC },
             new EndpointsView
             {
-                IsNotSendingHeartbeats = false,
+                IsSendingHeartbeats = true,
                 Name = endpointName1,
-                Id = DeterministicGuid.MakeId(endpointName1, "C")
+                Id = DeterministicGuid.MakeId(endpointName1, "D")
             }
         ]);
         var service = new HeartbeatEndpointSettingsSyncHostedService(
@@ -177,10 +179,10 @@ public async Task
         var mockMonitoringDataStore = new MockMonitoringDataStore(
             [new KnownEndpoint { EndpointDetails = new EndpointDetails { Name = endpointName1 } }]);
         var mockEndpointInstanceMonitoring = new MockEndpointInstanceMonitoring([
-            new EndpointsView { IsNotSendingHeartbeats = true, Name = endpointName1, Id = instanceA },
+            new EndpointsView { IsSendingHeartbeats = false, Name = endpointName1, Id = instanceA },
             new EndpointsView
             {
-                IsNotSendingHeartbeats = false,
+                IsSendingHeartbeats = true,
                 Name = endpointName1,
                 Id = DeterministicGuid.MakeId(endpointName1, "B")
             }

src/ServiceControl/Monitoring/EndpointInstanceMonitoring.cs

@@ -120,7 +120,6 @@ public EndpointsView[] GetEndpoints()
             {
                 var view = endpoint.GetView();
                 view.IsSendingHeartbeats = heartbeatLookup[endpoint.Id].Any(x => x.IsSendingHeartbeats());
-                view.IsNotSendingHeartbeats = heartbeatLookup[endpoint.Id].Any(x => x.IsNotSendingHeartbeats());
                 list.Add(view);
             }
 

src/ServiceControl/Monitoring/HeartbeatEndpointSettingsSyncHostedService.cs

@@ -35,6 +35,7 @@ protected override async Task ExecuteAsync(CancellationToken cancellationToken)
             {
                 try
                 {
+                    logger.LogInformation($"Performing sync for {nameof(HeartbeatEndpointSettingsSyncHostedService)}");
                     await PerformSync(cancellationToken);
                 }
                 catch (Exception ex) when (ex is not OperationCanceledException)
@@ -62,8 +63,9 @@ async Task PerformSync(CancellationToken cancellationToken)
 
     async Task PurgeMonitoringDataThatDoesNotNeedToBeTracked(CancellationToken cancellationToken)
     {
-        ILookup<string, Guid> monitorEndpointsLookup = endpointInstanceMonitoring.GetEndpoints()
-            .Where(view => view.IsNotSendingHeartbeats)
+        EndpointsView[] endpointsViews = endpointInstanceMonitoring.GetEndpoints();
+        ILookup<string, Guid> monitorEndpointsLookup = endpointsViews
+            .Where(view => !view.IsSendingHeartbeats)
             .ToLookup(view => view.Name, view => view.Id);
         await foreach (EndpointSettings endpointSetting in endpointSettingsStore.GetAllEndpointSettings()
                            .WithCancellation(cancellationToken))
@@ -72,10 +74,13 @@ async Task PurgeMonitoringDataThatDoesNotNeedToBeTracked(CancellationToken cance
             {
                 if (monitorEndpointsLookup.Contains(endpointSetting.Name))
                 {
-                    foreach (Guid endpointId in monitorEndpointsLookup[endpointSetting.Name])
+                    // We leave one dead instance behind, so that in ServicePulse we still display the endpoint as unhealthy, and is up to the user to manually delete it.
+                    // Otherwise, we would delete all dead instances and it could be that the endpoint should be alive but all instances are down and then we display nothing in ServicePulse which is no good!
+                    foreach (Guid endpointId in monitorEndpointsLookup[endpointSetting.Name].SkipLast(1))
                     {
                         endpointInstanceMonitoring.RemoveEndpoint(endpointId);
                         await monitoringDataStore.Delete(endpointId);
+                        logger.LogInformation($"Removed endpoint '{endpointSetting.Name}' from monitoring data.");
                     }
                 }
             }
@@ -102,6 +107,8 @@ async Task InitialiseSettings(HashSet<string> monitorEndpoints, CancellationToke
             if (!monitorEndpoints.Contains(endpointSetting.Name))
             {
                 await endpointSettingsStore.Delete(endpointSetting.Name, cancellationToken);
+                logger.LogInformation(
+                    $"Removed EndpointTracking setting for '{endpointSetting.Name}' endpoint, since this endpoint is no longer monitored.");
             }
 
             settingsNames.Add(endpointSetting.Name);
@@ -113,6 +120,8 @@ async Task InitialiseSettings(HashSet<string> monitorEndpoints, CancellationToke
             await endpointSettingsStore.UpdateEndpointSettings(
                 new EndpointSettings { Name = string.Empty, TrackInstances = userSetTrackInstances },
                 cancellationToken);
+            logger.LogInformation(
+                $"Initialized default value of EndpointTracking to {(userSetTrackInstances ? "tracking" : "not tracking")}.");
         }
 
         // Initialise settings for any missing endpoint
@@ -121,6 +130,8 @@ await endpointSettingsStore.UpdateEndpointSettings(
             await endpointSettingsStore.UpdateEndpointSettings(
                 new EndpointSettings { Name = name, TrackInstances = userSetTrackInstances },
                 cancellationToken);
+            logger.LogInformation(
+                $"Initialized '{name}' value of EndpointTracking to {(userSetTrackInstances ? "tracking" : "not tracking")}.");
         }
     }
 }

src/ServiceControl/Monitoring/HeartbeatMonitor.cs

@@ -47,7 +47,6 @@ public RecordedHeartbeat MarkDeadIfOlderThan(DateTime cutoff)
         }
 
         public bool IsSendingHeartbeats() => heartbeat?.Status == HeartbeatStatus.Alive;
-        public bool IsNotSendingHeartbeats() => heartbeat?.Status == HeartbeatStatus.Dead;
         volatile RecordedHeartbeat heartbeat = new RecordedHeartbeat(HeartbeatStatus.Unknown, null);
     }
 }