Contacted Regions Added to TraceSummary (#3353)

* Contacted Regions Added to TraceSummary * Comments resolved * Contracts Updated * Contracts Updated * Contracts Updated * Contacted Regions removed from constructor * TraceSummaryissues solved * Summary Tests modified for contacted regiosn
Azure · Aug 3, 2022 · 6360c86 · 6360c86
1 parent e7dddbb
commit 6360c86
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 60 deletions.
diff --git a/Microsoft.Azure.Cosmos/src/Diagnostics/CosmosTraceDiagnostics.cs b/Microsoft.Azure.Cosmos/src/Diagnostics/CosmosTraceDiagnostics.cs
@@ -46,7 +46,7 @@ public override TimeSpan GetClientElapsedTime()
 
         public override IReadOnlyList<(string regionName, Uri uri)> GetContactedRegions()
         {
-            return this.Value?.RegionsContacted;
+            return this.Value?.Summary?.RegionsContacted;
         }
 
         internal bool IsGoneExceptionHit()

diff --git a/Microsoft.Azure.Cosmos/src/Handler/TransportHandler.cs b/Microsoft.Azure.Cosmos/src/Handler/TransportHandler.cs
@@ -134,7 +134,7 @@ internal async Task<ResponseMessage> ProcessMessageAsync(
                 }
                 finally
                 {
-                    processMessageAsyncTrace.UpdateRegionContacted(clientSideRequestStatisticsTraceDatum);
+                    processMessageAsyncTrace.Summary.UpdateRegionContacted(clientSideRequestStatisticsTraceDatum);
                 }
 
                 ResponseMessage responseMessage = response.ToCosmosResponseMessage(

diff --git a/Microsoft.Azure.Cosmos/src/Tracing/ITrace.cs b/Microsoft.Azure.Cosmos/src/Tracing/ITrace.cs
@@ -68,11 +68,6 @@ interface ITrace : IDisposable
         /// </summary>
         IReadOnlyDictionary<string, object> Data { get; }
 
-        /// <summary>
-        /// Consolidated Region contacted Information of this and children nodes
-        /// </summary>
-        IReadOnlyList<(string, Uri)> RegionsContacted { get; }
-
         /// <summary>
         /// Starts a Trace and adds it as a child to this instance.
         /// </summary>
@@ -120,10 +115,5 @@ ITrace StartChild(
         /// <param name="trace">Existing trace.</param>
         void AddChild(ITrace trace);
 
-        /// <summary>
-        /// Update region contacted information to the parent Itrace
-        /// </summary>
-        /// <param name="traceDatum"></param>
-        void UpdateRegionContacted(TraceDatum traceDatum);
     }
 }
diff --git a/Microsoft.Azure.Cosmos/src/Tracing/NoOpTrace.cs b/Microsoft.Azure.Cosmos/src/Tracing/NoOpTrace.cs
@@ -10,9 +10,9 @@ namespace Microsoft.Azure.Cosmos.Tracing
     internal sealed class NoOpTrace : ITrace
     {
         public static readonly NoOpTrace Singleton = new NoOpTrace();
+        public static readonly TraceSummary NoOpTraceSummary = new TraceSummary();
 
         private static readonly IReadOnlyList<ITrace> NoOpChildren = new List<ITrace>();
-        private static readonly IReadOnlyList<(string, Uri)> NoOpRegionsContacted = new List<(string, Uri)>();
 
         private static readonly IReadOnlyDictionary<string, object> NoOpData = new Dictionary<string, object>();
 
@@ -30,7 +30,7 @@ private NoOpTrace()
 
         public TraceLevel Level => default;
 
-        public TraceSummary Summary => default;
+        public TraceSummary Summary => NoOpTraceSummary;
 
         public TraceComponent Component => default;
 
@@ -40,8 +40,6 @@ private NoOpTrace()
 
         public IReadOnlyDictionary<string, object> Data => NoOpData;
 
-        public IReadOnlyList<(string, Uri)> RegionsContacted => NoOpRegionsContacted;
-
         public void Dispose()
         {
             // NoOp

diff --git a/Microsoft.Azure.Cosmos/src/Tracing/Trace.cs b/Microsoft.Azure.Cosmos/src/Tracing/Trace.cs
@@ -17,15 +17,13 @@ internal sealed class Trace : ITrace
         private static readonly IReadOnlyDictionary<string, object> EmptyDictionary = new Dictionary<string, object>();
         private readonly List<ITrace> children;
         private readonly Lazy<Dictionary<string, object>> data;
-        private readonly ISet<(string, Uri)> regionContactedInternal;
         private ValueStopwatch stopwatch;
 
         private Trace(
             string name,
             TraceLevel level,
             TraceComponent component,
             Trace parent,
-            ISet<(string, Uri)> regionContactedInternal,
             TraceSummary summary)
         {
             this.Name = name ?? throw new ArgumentNullException(nameof(name));
@@ -37,8 +35,6 @@ private Trace(
             this.Parent = parent;
             this.children = new List<ITrace>();
             this.data = new Lazy<Dictionary<string, object>>();
-
-            this.regionContactedInternal = regionContactedInternal;
             this.Summary = summary ?? throw new ArgumentNullException(nameof(summary));
         }
 
@@ -62,38 +58,6 @@ private Trace(
 
         public IReadOnlyDictionary<string, object> Data => this.data.IsValueCreated ? this.data.Value : Trace.EmptyDictionary;
 
-        public IReadOnlyList<(string, Uri)> RegionsContacted 
-        { 
-            get
-            {
-                lock (this.regionContactedInternal)
-                {
-                    return this.regionContactedInternal.ToList();
-                }
-            }
-        }
-
-        /// <summary>
-        /// Update region contacted information to this node
-        /// </summary>
-        /// <param name="traceDatum"></param>
-        public void UpdateRegionContacted(TraceDatum traceDatum)
-        {
-            if (traceDatum is ClientSideRequestStatisticsTraceDatum clientSideRequestStatisticsTraceDatum)
-            {
-                if (clientSideRequestStatisticsTraceDatum.RegionsContacted == null || 
-                            clientSideRequestStatisticsTraceDatum.RegionsContacted.Count == 0)
-                {
-                    return;
-                }
-
-                lock (this.regionContactedInternal)
-                {
-                    this.regionContactedInternal.UnionWith(clientSideRequestStatisticsTraceDatum.RegionsContacted);
-                }
-            }
-        }
-
         public void Dispose()
         {
             this.stopwatch.Stop();
@@ -123,7 +87,6 @@ public ITrace StartChild(
                 level: level,
                 component: component,
                 parent: this,
-                regionContactedInternal: this.regionContactedInternal,
                 summary: this.Summary);
 
             this.AddChild(child);
@@ -156,14 +119,13 @@ public static Trace GetRootTrace(
                 level: level,
                 component: component,
                 parent: null,
-                regionContactedInternal: new HashSet<(string, Uri)>(),
                 summary: new TraceSummary());
         }
 
         public void AddDatum(string key, TraceDatum traceDatum)
         {
             this.data.Value.Add(key, traceDatum);
-            this.UpdateRegionContacted(traceDatum);
+            this.Summary.UpdateRegionContacted(traceDatum);
         }
 
         public void AddDatum(string key, object value)

diff --git a/Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs b/Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs
@@ -263,7 +263,7 @@ public void RecordResponse(
             {
                 if (locationEndpoint != null)
                 {
-                    this.RegionsContacted.Add((regionName, locationEndpoint));
+                    this.TraceSummary?.AddRegionContacted(regionName, locationEndpoint);
                 }
 
                 if (responseStatistics.StoreResult != null && !((HttpStatusCode)responseStatistics.StoreResult.StatusCode).IsSuccess()
@@ -352,7 +352,7 @@ public void RecordHttpResponse(HttpRequestMessage request,
                 if (request.Properties != null && 
                         request.Properties.TryGetValue(HttpRequestRegionNameProperty, out object regionName))
                 {
-                    this.RegionsContacted.Add((Convert.ToString(regionName), locationEndpoint));
+                    this.TraceSummary.AddRegionContacted(Convert.ToString(regionName), locationEndpoint);
                 }
 
                 this.shallowCopyOfHttpResponseStatistics = null;
@@ -380,7 +380,7 @@ public void RecordHttpException(HttpRequestMessage request,
                 if (request.Properties != null &&
                         request.Properties.TryGetValue(HttpRequestRegionNameProperty, out object regionName))
                 {
-                    this.RegionsContacted.Add((Convert.ToString(regionName), locationEndpoint));
+                    this.TraceSummary.AddRegionContacted(Convert.ToString(regionName), locationEndpoint);
                 }
 
                 this.shallowCopyOfHttpResponseStatistics = null;

diff --git a/Microsoft.Azure.Cosmos/src/Tracing/TraceSummary.cs b/Microsoft.Azure.Cosmos/src/Tracing/TraceSummary.cs
@@ -6,8 +6,10 @@ namespace Microsoft.Azure.Cosmos.Tracing
 {
     using System;
     using System.Collections.Generic;
+    using System.Linq;
     using System.Text;
     using System.Threading;
+    using Microsoft.Azure.Cosmos.Tracing.TraceData;
 
     /// <summary>
     /// The total count of failed requests for an <see cref="ITrace"/>.
@@ -41,5 +43,58 @@ public int GetFailedCount()
             return this.failedRequestCount;
         }
 
+        /// <summary>
+        /// Consolidated Region contacted Information of this and children nodes
+        /// </summary>
+        private readonly HashSet<(string, Uri)> regionContactedInternal = new HashSet<(string, Uri)>();
+
+        /// <summary>
+        /// Consolidated Region contacted Information of this and children nodes
+        /// </summary>
+        public IReadOnlyList<(string, Uri)> RegionsContacted
+        {
+            get
+            {
+                lock (this.regionContactedInternal)
+                {
+                    return this.regionContactedInternal.ToList();
+                }
+            }
+        }
+
+        /// <summary>
+        /// Update region contacted information to this node
+        /// </summary>
+        /// <param name="traceDatum"></param>
+        public void UpdateRegionContacted(TraceDatum traceDatum)
+        {
+            if (traceDatum is ClientSideRequestStatisticsTraceDatum clientSideRequestStatisticsTraceDatum)
+            {
+                if (clientSideRequestStatisticsTraceDatum.RegionsContacted == null ||
+                            clientSideRequestStatisticsTraceDatum.RegionsContacted.Count == 0)
+                {
+                    return;
+                }
+
+                lock (this.regionContactedInternal)
+                {
+                    this.regionContactedInternal.UnionWith(clientSideRequestStatisticsTraceDatum.RegionsContacted);
+                }
+            }
+        }
+
+        /// <summary>
+        /// Add region contacted information to this node
+        /// </summary>
+        /// <param name="regionName"></param>
+        /// <param name="locationEndpoint"></param>
+        public void AddRegionContacted(string regionName, Uri locationEndpoint)
+        {
+            lock (this.regionContactedInternal)
+            {
+                this.regionContactedInternal.Add((regionName, locationEndpoint));
+            }
+        }
+
     }
 }
diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/SummaryDiagnosticsTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/SummaryDiagnosticsTests.cs
@@ -52,7 +52,7 @@ public async Task PointOperationSummaryTest()
 
             Assert.AreEqual(summaryDiagnostics.DirectRequestsSummary.Value[(200, 0)], 1);
             Assert.IsFalse(summaryDiagnostics.GatewayRequestsSummary.IsValueCreated);
-            Assert.AreEqual(summaryDiagnostics.AllRegionsContacted.Value.Count, 1);
+            Assert.AreEqual(trace.Summary.RegionsContacted.Count, 1);
         }
 
         [TestMethod]
@@ -91,11 +91,16 @@ public async Task QuerySummaryTest()
                 FeedResponse<ToDoActivity> response = await feedIterator.ReadNextAsync();
                 traces.Add(((CosmosTraceDiagnostics)response.Diagnostics).Value);
             }
+            HashSet<(string, Uri)> headers = new HashSet<(string, Uri)>();
+            foreach (Trace item in traces)
+            {
+                headers.UnionWith(item.Summary.RegionsContacted);
+            }
 
             SummaryDiagnostics summaryDiagnostics = new SummaryDiagnostics(TraceJoiner.JoinTraces(traces));
             Assert.AreEqual(summaryDiagnostics.DirectRequestsSummary.Value.Keys.Count, 1);
             Assert.IsTrue(summaryDiagnostics.DirectRequestsSummary.Value[(200, 0)] > 1);
-            Assert.AreEqual(summaryDiagnostics.AllRegionsContacted.Value.Count, 1);
+            Assert.AreEqual(headers.Count, 1);
         }
 
         [TestMethod]