Diagnostics: Adds Gateway Request Stats #2324

asketagarwal · 2021-03-19T17:03:59Z

Pull Request Template

The current client side request stats does not record http operations. This is extending the contract to include all http operations which is necessary for troubleshooting latency issues. It was chosen to extend the IClientSideRequestStats rather than wiring though ITrace because of the dependencies in the direct package. Using ITrace would require wiring through all the shared code which is much larger task.

Diagostics Changes

{
   "HttpResponseStatistics":[
      {
         "StartTimeUTC":"2021-03-22T10:11:47.5592589Z",
         "EndTimeUTC":"2021-03-22T10:11:50.3805645Z",
         "RequestUri":"https://127.0.0.1:8081/dbs/7423ce13-7c53-4d86-aa0f-8baafe5350a2/colls/176a5bb7-368b-4444-82
9b-ef5c8308afd8/docs/7dd28a8a-f2d3-4d9d-869a-40754c82db3c",
         "ResourceType":"Document",
         "HttpMethod":"GET",
         "ExceptionType":"System.OperationCanceledException",
         "ExceptionMessage":"The operation was canceled."
      },
      {
         "StartTimeUTC":"2021-03-22T10:11:47.5592589Z",
         "EndTimeUTC":"2021-03-22T10:11:51.2861564Z",
         "RequestUri":"https://127.0.0.1:8081/dbs/7423ce13-7c53-4d86-aa0f-8baafe5350a2/colls/176a5bb7-368b-4444-829b-ef5c8308afd8/docs/7dd28a8a-f2d3-4d9d-869a-40754c82db3c",
         "ResourceType":"Document",
         "HttpMethod":"GET",
         "ExceptionType":"System.OperationCanceledException",
         "ExceptionMessage":"The operation was canceled."
      },
      {
         "StartTimeUTC":"2021-03-22T10:11:47.5592589Z",
         "EndTimeUTC":"2021-03-22T10:11:52.2945455Z",
         "RequestUri":"https://127.0.0.1:8081/dbs/7423ce13-7c53-4d86-aa0f-8baafe5350a2/colls/176a5bb7-368b-4444-829b-ef5c8308afd8/docs/7dd28a8a-f2d3-4d9d-869a-40754c82db3c",
         "ResourceType":"Document",
         "HttpMethod":"GET",
         "StatusCode":"OK"
      }
   ]
}

Microsoft.Azure.Cosmos/src/HttpClient/CosmosHttpClient.cs

Microsoft.Azure.Cosmos/src/GatewayAccountReader.cs

Microsoft.Azure.Cosmos/src/GatewayStoreClient.cs

Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs

...oft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/GatewayClientSideRequestStats.cs

Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs

Microsoft.Azure.Cosmos/src/HttpClient/CosmosHttpClientCore.cs

Microsoft.Azure.Cosmos/src/Handler/TransportHandler.cs

…tewayRequestStats

kirankumarkolli · 2021-03-23T15:19:31Z

Do the address resolution statics cover these?

kirankumarkolli · 2021-03-23T15:34:36Z

Microsoft.Azure.Cosmos/src/HttpClient/CosmosHttpClientCore.cs

@@ -169,7 +169,7 @@ public static HttpMessageHandler CreateHttpClientHandler(int gatewayModeMaxConne
            INameValueCollection additionalHeaders,
            ResourceType resourceType,
            HttpTimeoutPolicy timeoutPolicy,
-            ITrace trace,


When to use ITrace vs IClientSideRequestStatistics?

IClientSideRequestStatistics will be used for any contracts with the Direct package. Taking a dependency on ITrace will cause a circle reference which make it difficult to make any contract changes. This allows the v2 SDK to get the additional tracing. If it is decided that ITrace should replace IClientSideRequestStatistics it will require refactoring the v2 SDK and the entire Direct package.

Microsoft.Azure.Cosmos/src/Routing/ClientCollectionCache.cs

Microsoft.Azure.Cosmos/src/HttpClient/CosmosHttpClientCore.cs

Microsoft.Azure.Cosmos/src/Routing/PartitionKeyRangeCache.cs

ealsur · 2021-03-23T21:52:37Z

Microsoft.Azure.Cosmos/src/Tracing/TraceWriter.TraceJsonWriter.cs

+                    jsonWriter.WriteStringValue(stat.Exception.GetType().ToString());
+
+                    jsonWriter.WriteFieldName("ExceptionMessage");
+                    jsonWriter.WriteStringValue(stat.Exception.Message);


If it worth instead capturing the ToString instead of the Message? Or the stack trace?

Is the size a concern? Stack trace and tostring which also prints the stack trace can be very large.

Not the size, but rather, would we have enough troubleshooting information just from the message

This will only include exception thrown by the HttpClient so I think it will be ok to just record the message. The Trace structure will let us know where it was thrown from.

...oft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/GatewayClientSideRequestStats.cs

…wayRequestStats

…om/Azure/azure-cosmos-dotnet-v3 into users/askagarw/GatewayRequestStats

j82w · 2021-04-19T21:10:48Z

Microsoft.Azure.Cosmos/src/Handler/TransportHandler.cs

+
+                Debug.Assert(System.Diagnostics.Trace.CorrelationManager.ActivityId != Guid.Empty, "Trace activity id is missing");
+                CosmosException cosmosException = CosmosExceptionFactory.CreateRequestTimeoutException(
+                                                            message: ex.Data?["Message"].ToString(),


Why not just ex.Message?

kirankumarkolli

Please verify that there is no perf impact in steady state.

kirankumarkolli

Please verify that there is no perf impact in steady state.

j82w · 2021-04-23T11:39:20Z

Please verify that there is no perf impact in steady state.

I verified the end to end perf shows no changes in steady state.

Initial commit for gateway request stats

1cc2ca5

asketagarwal requested review from bchong95, ealsur, FabianMeiswinkel, j82w, khdang, kirankumarkolli, kirillg and sboshra as code owners March 19, 2021 17:03

j82w reviewed Mar 19, 2021

View reviewed changes

Microsoft.Azure.Cosmos/src/HttpClient/CosmosHttpClient.cs Show resolved Hide resolved

asketagarwal added 2 commits March 22, 2021 16:01

Adding jsonwriter implementation

e503dcf

Removes ITrace implementation from HttpClientCore

6e026f1

j82w reviewed Mar 22, 2021

View reviewed changes

Microsoft.Azure.Cosmos/src/GatewayAccountReader.cs Outdated Show resolved Hide resolved

j82w reviewed Mar 22, 2021

View reviewed changes

Microsoft.Azure.Cosmos/src/Tracing/TraceData/ClientSideRequestStatisticsTraceDatum.cs Outdated Show resolved Hide resolved

ealsur reviewed Mar 22, 2021

View reviewed changes

Microsoft.Azure.Cosmos/src/HttpClient/CosmosHttpClientCore.cs Show resolved Hide resolved

asketagarwal added 2 commits March 23, 2021 01:17

catching operationcancelled in transporthandler

b51ed87

Adding locking

b63ce13

j82w reviewed Mar 22, 2021

View reviewed changes

Microsoft.Azure.Cosmos/src/Handler/TransportHandler.cs Outdated Show resolved Hide resolved

asketagarwal added 2 commits March 23, 2021 16:11

Fixing Tests

fb4cb7e

Merge remote-tracking branch 'upstream/master' into users/askagarw/Ga…

d88a0db

…tewayRequestStats

kirankumarkolli reviewed Mar 23, 2021

View reviewed changes

Fixing Tests

b0fde5c

j82w reviewed Mar 23, 2021

View reviewed changes

Microsoft.Azure.Cosmos/src/Routing/ClientCollectionCache.cs Outdated Show resolved Hide resolved

Adding itrace for GetPartitionKeyPathTokensAsync and fixing baseline

8c87e8a

ealsur reviewed Mar 23, 2021

View reviewed changes

asketagarwal added 3 commits March 24, 2021 12:14

changing filename and ordering params

4113d01

Merge remote-tracking branch 'origin/master' into users/askagarw/Gate…

9faf524

…wayRequestStats

Merge remote-tracking branch 'origin/master' into users/askagarw/Gate…

c438755

…wayRequestStats

Merge branch 'master' into users/askagarw/GatewayRequestStats

cbce482

j82w requested a review from neildsh as a code owner April 16, 2021 21:24

j82w previously approved these changes Apr 16, 2021

View reviewed changes

asketagarwal added 4 commits April 20, 2021 00:59

Merge remote-tracking branch 'origin/master' into users/askagarw/Gate…

4194f73

…wayRequestStats

mergin conflicts

2fbbbb4

updating baseline

baa4fbb

Merge branch 'users/askagarw/GatewayRequestStats' of https://github.c…

16be193

…om/Azure/azure-cosmos-dotnet-v3 into users/askagarw/GatewayRequestStats

asketagarwal dismissed j82w’s stale review via 16be193 April 19, 2021 19:45

updating baseline

2c9abc3

j82w reviewed Apr 19, 2021

View reviewed changes

Merge branch 'master' into users/askagarw/GatewayRequestStats

8282cd2

kirankumarkolli requested changes Apr 21, 2021

View reviewed changes

kirankumarkolli approved these changes Apr 21, 2021

View reviewed changes

Merge branch 'master' into users/askagarw/GatewayRequestStats

3624b0c

j82w merged commit 64cbee3 into master Apr 23, 2021

j82w deleted the users/askagarw/GatewayRequestStats branch April 23, 2021 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diagnostics: Adds Gateway Request Stats #2324

Diagnostics: Adds Gateway Request Stats #2324

asketagarwal commented Mar 19, 2021 •

edited by j82w

Loading

kirankumarkolli commented Mar 23, 2021

kirankumarkolli Mar 23, 2021

j82w Mar 23, 2021 •

edited

Loading

ealsur Mar 23, 2021

asketagarwal Mar 24, 2021

ealsur Mar 30, 2021

j82w Apr 16, 2021

j82w Apr 19, 2021

kirankumarkolli left a comment

kirankumarkolli left a comment

j82w commented Apr 23, 2021 •

edited

Loading

Diagnostics: Adds Gateway Request Stats #2324

Diagnostics: Adds Gateway Request Stats #2324

Conversation

asketagarwal commented Mar 19, 2021 • edited by j82w Loading

Pull Request Template

Diagostics Changes

kirankumarkolli commented Mar 23, 2021

kirankumarkolli Mar 23, 2021

Choose a reason for hiding this comment

j82w Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

ealsur Mar 23, 2021

Choose a reason for hiding this comment

asketagarwal Mar 24, 2021

Choose a reason for hiding this comment

ealsur Mar 30, 2021

Choose a reason for hiding this comment

j82w Apr 16, 2021

Choose a reason for hiding this comment

j82w Apr 19, 2021

Choose a reason for hiding this comment

kirankumarkolli left a comment

Choose a reason for hiding this comment

kirankumarkolli left a comment

Choose a reason for hiding this comment

j82w commented Apr 23, 2021 • edited Loading

asketagarwal commented Mar 19, 2021 •

edited by j82w

Loading

j82w Mar 23, 2021 •

edited

Loading

j82w commented Apr 23, 2021 •

edited

Loading