Optimize returned instances from Eureka

Author: bart-vmware

src/Discovery/src/Eureka/AppInfo/ApplicationInfoCollection.cs

@@ -4,6 +4,7 @@
 
 using System.Collections;
 using System.Collections.Concurrent;
+using System.Collections.ObjectModel;
 using System.Text;
 using System.Text.Json;
 using Steeltoe.Common;
@@ -17,11 +18,8 @@ namespace Steeltoe.Discovery.Eureka.AppInfo;
 /// </summary>
 public sealed class ApplicationInfoCollection : IReadOnlyCollection<ApplicationInfo>
 {
-    private readonly object _addRemoveInstanceLock = new();
-
     internal ConcurrentDictionary<string, ApplicationInfo> ApplicationMap { get; } = new();
     internal ConcurrentDictionary<string, ConcurrentDictionary<string, InstanceInfo>> VipInstanceMap { get; } = new();
-    internal ConcurrentDictionary<string, ConcurrentDictionary<string, InstanceInfo>> SecureVipInstanceMap { get; } = new();
 
     public string? AppsHashCode { get; internal set; }
     public long? Version { get; private set; }
@@ -51,18 +49,25 @@ internal ApplicationInfoCollection(IList<ApplicationInfo> apps)
         return ApplicationMap.GetValueOrDefault(appName.ToUpperInvariant());
     }
 
-    internal List<InstanceInfo> GetInstancesBySecureVipAddress(string secureVipAddress)
+    internal ReadOnlyCollection<InstanceInfo> GetInstancesByVipAddress(string vipAddress)
     {
-        ArgumentException.ThrowIfNullOrWhiteSpace(secureVipAddress);
+        ArgumentException.ThrowIfNullOrWhiteSpace(vipAddress);
 
-        return GetByVipAddress(secureVipAddress, SecureVipInstanceMap);
-    }
+        List<InstanceInfo> result = [];
+        string addressUpper = vipAddress.ToUpperInvariant();
 
-    internal List<InstanceInfo> GetInstancesByVipAddress(string vipAddress)
-    {
-        ArgumentException.ThrowIfNullOrWhiteSpace(vipAddress);
+        if (VipInstanceMap.TryGetValue(addressUpper, out ConcurrentDictionary<string, InstanceInfo>? instancesById))
+        {
+            foreach (InstanceInfo instance in instancesById.Values)
+            {
+                if ((ReturnUpInstancesOnly && instance.EffectiveStatus == InstanceStatus.Up) || !ReturnUpInstancesOnly)
+                {
+                    result.Add(instance);
+                }
+            }
+        }
 
-        return GetByVipAddress(vipAddress, VipInstanceMap);
+        return result.AsReadOnly();
     }
 
     /// <inheritdoc />
@@ -92,79 +97,51 @@ internal void Add(ApplicationInfo app)
 
         foreach (InstanceInfo instance in app.Instances)
         {
-            AddInstanceToVip(instance);
+            AddToVipInstanceMap(instance);
         }
     }
 
-    private void AddInstanceToVip(InstanceInfo instance)
+    private void AddToVipInstanceMap(InstanceInfo instance)
     {
-        foreach (string vipAddress in ExpandVipAddresses(instance.VipAddress))
+        foreach (string vipAddress in ExpandVipAddresses(instance))
         {
-            AddInstanceToVip(instance, vipAddress, VipInstanceMap);
-        }
+            string addressUpper = vipAddress.ToUpperInvariant();
 
-        foreach (string secureVipAddress in ExpandVipAddresses(instance.SecureVipAddress))
-        {
-            AddInstanceToVip(instance, secureVipAddress, SecureVipInstanceMap);
+            ConcurrentDictionary<string, InstanceInfo> instancesById = VipInstanceMap.GetOrAdd(addressUpper, new ConcurrentDictionary<string, InstanceInfo>());
+            instancesById.AddOrUpdate(instance.InstanceId, _ => instance, (_, _) => instance);
         }
     }
 
-    private void AddInstanceToVip(InstanceInfo instance, string address, ConcurrentDictionary<string, ConcurrentDictionary<string, InstanceInfo>> dictionary)
+    private static HashSet<string> ExpandVipAddresses(InstanceInfo instance)
     {
-        lock (_addRemoveInstanceLock)
-        {
-            string addressUpper = address.ToUpperInvariant();
-
-            if (!dictionary.TryGetValue(addressUpper, out ConcurrentDictionary<string, InstanceInfo>? instances))
-            {
-                instances = new ConcurrentDictionary<string, InstanceInfo>();
-                dictionary[addressUpper] = instances;
-            }
+        HashSet<string> addresses = [];
 
-            instances[instance.InstanceId] = instance;
+        if (instance.SecureVipAddress != null)
+        {
+            string[] secureAddresses = instance.SecureVipAddress.Split(',', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries);
+            addresses.UnionWith(secureAddresses);
         }
-    }
 
-    private static string[] ExpandVipAddresses(string? addresses)
-    {
-        if (string.IsNullOrWhiteSpace(addresses))
+        if (instance.VipAddress != null)
         {
-            return [];
+            string[] vipAddresses = instance.VipAddress.Split(',', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries);
+            addresses.UnionWith(vipAddresses);
         }
 
-        return addresses.Split(',', StringSplitOptions.TrimEntries | StringSplitOptions.RemoveEmptyEntries);
+        return addresses;
     }
 
-    internal void RemoveInstanceFromVip(InstanceInfo instance)
+    internal void RemoveFromVipInstanceMap(InstanceInfo instance)
     {
         ArgumentNullException.ThrowIfNull(instance);
 
-        foreach (string vipAddress in ExpandVipAddresses(instance.VipAddress))
-        {
-            RemoveInstanceFromVip(instance, vipAddress, VipInstanceMap);
-        }
-
-        foreach (string secureVipAddress in ExpandVipAddresses(instance.SecureVipAddress))
-        {
-            RemoveInstanceFromVip(instance, secureVipAddress, SecureVipInstanceMap);
-        }
-    }
-
-    private void RemoveInstanceFromVip(InstanceInfo instance, string address,
-        ConcurrentDictionary<string, ConcurrentDictionary<string, InstanceInfo>> dictionary)
-    {
-        lock (_addRemoveInstanceLock)
+        foreach (string vipAddress in ExpandVipAddresses(instance))
         {
-            string addressUpper = address.ToUpperInvariant();
+            string addressUpper = vipAddress.ToUpperInvariant();
 
-            if (dictionary.TryGetValue(addressUpper, out ConcurrentDictionary<string, InstanceInfo>? instances))
+            if (VipInstanceMap.TryGetValue(addressUpper, out ConcurrentDictionary<string, InstanceInfo>? instancesById))
             {
-                _ = instances.TryRemove(instance.InstanceId, out _);
-
-                if (instances.IsEmpty)
-                {
-                    _ = dictionary.TryRemove(addressUpper, out _);
-                }
+                instancesById.TryRemove(instance.InstanceId, out _);
             }
         }
     }
@@ -190,11 +167,11 @@ internal void UpdateFromDelta(ApplicationInfoCollection delta)
                     case ActionType.Added:
                     case ActionType.Modified:
                         existingApp.Add(instance);
-                        AddInstanceToVip(instance);
+                        AddToVipInstanceMap(instance);
                         break;
                     case ActionType.Deleted:
                         existingApp.Remove(instance);
-                        RemoveInstanceFromVip(instance);
+                        RemoveFromVipInstanceMap(instance);
                         break;
                 }
             }
@@ -261,22 +238,4 @@ internal static ApplicationInfoCollection FromJson(JsonApplications? jsonApplica
 
         return apps;
     }
-
-    private List<InstanceInfo> GetByVipAddress(string name, ConcurrentDictionary<string, ConcurrentDictionary<string, InstanceInfo>> dictionary)
-    {
-        List<InstanceInfo> result = [];
-
-        if (dictionary.TryGetValue(name.ToUpperInvariant(), out ConcurrentDictionary<string, InstanceInfo>? instances))
-        {
-            foreach (InstanceInfo instance in instances.Values.ToArray())
-            {
-                if ((ReturnUpInstancesOnly && instance.EffectiveStatus == InstanceStatus.Up) || !ReturnUpInstancesOnly)
-                {
-                    result.Add(instance);
-                }
-            }
-        }
-
-        return result;
-    }
 }

src/Discovery/src/Eureka/EurekaDiscoveryClient.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the Apache 2.0 License.
 // See the LICENSE file in the project root for more information.
 
+using System.Text;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Steeltoe.Common.Discovery;
@@ -156,12 +157,11 @@ public EurekaDiscoveryClient(EurekaApplicationInfoManager appInfoManager, Eureka
         return Applications.GetRegisteredApplication(appName);
     }
 
-    internal IReadOnlyList<InstanceInfo> GetInstancesByVipAddress(string vipAddress, bool secure)
+    internal IReadOnlyList<InstanceInfo> GetInstancesByVipAddress(string vipAddress)
     {
         ArgumentException.ThrowIfNullOrWhiteSpace(vipAddress);
 
-        List<InstanceInfo> instances = secure ? Applications.GetInstancesBySecureVipAddress(vipAddress) : Applications.GetInstancesByVipAddress(vipAddress);
-        return instances.AsReadOnly();
+        return Applications.GetInstancesByVipAddress(vipAddress);
     }
 
     /// <inheritdoc />
@@ -533,18 +533,40 @@ public Task<IList<IServiceInstance>> GetInstancesAsync(string serviceId, Cancell
     {
         ArgumentException.ThrowIfNullOrWhiteSpace(serviceId);
 
-        IReadOnlyList<InstanceInfo> nonSecureInstances = GetInstancesByVipAddress(serviceId, false);
-        IReadOnlyList<InstanceInfo> secureInstances = GetInstancesByVipAddress(serviceId, true);
-
-        IEnumerable<InstanceInfo> instances = secureInstances.Concat(nonSecureInstances).DistinctBy(instance => instance.InstanceId);
+        IReadOnlyList<InstanceInfo> instances = GetInstancesByVipAddress(serviceId);
         IServiceInstance[] serviceInstances = instances.Select(instance => new EurekaServiceInstance(instance)).Cast<IServiceInstance>().ToArray();
 
-        _logger.LogDebug("Returning {Count} service instances: {ServiceInstances}", serviceInstances.Length,
-            string.Join(", ", serviceInstances.Select(instance => $"{instance.ServiceId}={instance.Uri}")));
+        if (_logger.IsEnabled(LogLevel.Debug))
+        {
+            string instanceNames = string.Join(", ", serviceInstances.Select(FormatServiceInstance));
+            _logger.LogDebug("Returning {Count} service instances for '{ServiceId}': {ServiceInstances}", serviceInstances.Length, serviceId, instanceNames);
+        }
 
         return Task.FromResult<IList<IServiceInstance>>(serviceInstances);
     }
 
+    private static string FormatServiceInstance(IServiceInstance instance)
+    {
+        var builder = new StringBuilder();
+
+        if (instance.SecureUri != null)
+        {
+            builder.Append(instance.SecureUri);
+        }
+
+        if (instance.NonSecureUri != null)
+        {
+            if (builder.Length > 0)
+            {
+                builder.Append(';');
+            }
+
+            builder.Append(instance.NonSecureUri);
+        }
+
+        return $"{instance.InstanceId}={builder}";
+    }
+
     /// <inheritdoc />
     public IServiceInstance GetLocalServiceInstance()
     {