Add GetClientElapsedTime to CosmosDiagnostics #1268

j82w · 2020-03-10T14:54:21Z

Pull Request Template

Description

This is adding GetElapsedClientLatency to CosmosDiagnostics.

The elapsed client latency is useful for users to determine if they need to log the diagnostics.
Fixed bug where request options was not being sent in Database.ReadStreamAsync() and Database.DeleteStreamAsync()

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Closing issues

closes #1218

public abstract class CosmosDiagnostics
{
        public abstract TimeSpan GetClientElapsedTime();

        public abstract override string ToString();
    }	  
}

…nostics/properties

…t option not getting set for database read and delete stream.

kirankumarkolli · 2020-03-11T14:39:47Z

#458 update description to the scope of the change.

kirankumarkolli · 2020-03-11T14:42:14Z

Batch diagnostics: Diagnostics duplication should solve it no?

Microsoft.Azure.Cosmos/src/Batch/BatchAsyncContainerExecutor.cs

Microsoft.Azure.Cosmos/src/RequestOptions/RequestOptions.cs

kirankumarkolli · 2020-03-11T14:47:43Z

Pulling in ClientReqeustId inside might actually confuse CX that its actually sent uses E2E and might be source of future CRI's.

If its for application tracing/correlation, then application can trace it explicitly (yes its more work but straightforward).

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnostics.cs

kirankumarkolli · 2020-03-11T17:25:57Z

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnostics.cs

    /// <summary>
    ///  Contains the cosmos diagnostic information for the current request to Azure Cosmos DB service.
    /// </summary>
    public abstract class CosmosDiagnostics
    {
+        /// <summary>
+        /// This represent the current elapsed latency of the request. 


request [](start = 62, length = 7)

request: qualify it very explicitly that its E2E elapsed time.

Please check the updated text

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContextCore.cs

Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/DotNetSDKAPI.json

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticScope.cs

ealsur · 2020-03-12T16:04:37Z

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContext.cs

        public abstract string UserAgent { get; protected set; }

+        internal abstract CosmosDiagnostics Diagnostics { get; }


Why internal if the class in internal?

It makes it easier to read. You don't have to scroll to the top to see if the class is internal.

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContextCore.cs

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsCore.cs

Microsoft.Azure.Cosmos/src/Diagnostics/EmptyCosmosDiagnosticsContext.cs

...re.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/Diagnostics/CosmosDiagnosticsSerializerTests.cs

…pe to track overall time.

…nostics/properties

… the SDK.

FabianMeiswinkel · 2020-03-16T12:17:44Z

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnostics.cs

 namespace Microsoft.Azure.Cosmos
 {
+    using System;
+
    /// <summary>
    ///  Contains the cosmos diagnostic information for the current request to Azure Cosmos DB service.
    /// </summary>
    public abstract class CosmosDiagnostics


I assume this is the envelope meant to expose additional diagnostics information for customers as well that are currently only available in the opaque diagnostics string? Like ActivityIds related to the current request etc.?

Yes, this is the public contract. Then we have a separate implementation class.

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnostics.cs

…ture it is easier to add GetNetworkElapsedTime or other time factors users may want.

Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs

Microsoft.Azure.Cosmos/src/FeedIteratorCore.cs

Jake Willey added 4 commits February 27, 2020 11:58

Adding user client request id, and get contact host uris.

1134708

Add initial objects

ef54109

Merge remote-tracking branch 'origin/master' into users/jawilley/diag…

516263e

…nostics/properties

Adding elapsed time and user client request id. Fixed bug with reques…

5b85557

…t option not getting set for database read and delete stream.

j82w added bug Something isn't working feature-request New feature or request Diagnostics Issues around diagnostics and troubleshooting labels Mar 10, 2020

j82w requested review from bchong95, khdang, kirankumarkolli and kirillg as code owners March 10, 2020 14:54

j82w self-assigned this Mar 10, 2020

Jake Willey added 6 commits March 10, 2020 07:55

Removed unused code

171360b

Fixed tests

bee3545

Updated changelog

2373c64

Merged to latest

3491639

Refactored test to use visitor pattern.

d664b97

Updated tests

59c4f44

kirankumarkolli reviewed Mar 11, 2020

View reviewed changes

Microsoft.Azure.Cosmos/src/Batch/BatchAsyncContainerExecutor.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Mar 11, 2020

View reviewed changes

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

kirankumarkolli reviewed Mar 11, 2020

View reviewed changes

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

Jake Willey added 3 commits March 11, 2020 08:40

Removed UserClientSideRequestId

6f8d829

Reverted more UserClientSideRequestId changes

5377a52

Updated changelog

912b784

j82w changed the title ~~Add UserClientRequestId and GetElapsedClientLatency to CosmosDiagnostics~~ Add GetElapsedClientLatency to CosmosDiagnostics Mar 11, 2020

Updated contract changes

693fcc6

kirankumarkolli reviewed Mar 11, 2020

View reviewed changes

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnostics.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Mar 11, 2020

View reviewed changes

Jake Willey added 2 commits March 11, 2020 11:41

Renames based on feedback

b2b23db

Update contract change

347c1e5

kirankumarkolli reviewed Mar 12, 2020

View reviewed changes

Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContextCore.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Mar 12, 2020

View reviewed changes

Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/DotNetSDKAPI.json Outdated Show resolved Hide resolved

ealsur reviewed Mar 12, 2020

View reviewed changes

Jake Willey added 2 commits March 13, 2020 06:58

Made changes based on feedback. DiagnosticContext now just uses a sco…

dbff1c8

…pe to track overall time.

Merge remote-tracking branch 'origin/master' into users/jawilley/diag…

6c98165

…nostics/properties

j82w force-pushed the users/jawilley/diagnostics/properties branch from 39c8c5d to 6c98165 Compare March 13, 2020 14:08

Jake Willey added 4 commits March 13, 2020 10:36

Adding default behavior to avoid breaking change for user's upgrading…

bb43fa4

… the SDK.

Removed usings statements

b91d69a

Merge to latest

afe8994

Fixed merge conflicts

4ef3e82

FabianMeiswinkel previously approved these changes Mar 16, 2020

View reviewed changes

Renamed to GetElapsedTime to GetClientElapsedTime. This way in the fu…

ece4a12

…ture it is easier to add GetNetworkElapsedTime or other time factors users may want.

j82w dismissed FabianMeiswinkel’s stale review via ece4a12 March 16, 2020 13:45

j82w changed the title ~~Add GetElapsedClientLatency to CosmosDiagnostics~~ Add GetClientElapsedTime to CosmosDiagnostics Mar 16, 2020

Updated contract

45e26ab

ealsur reviewed Mar 16, 2020

View reviewed changes

Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs Show resolved Hide resolved

Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs Show resolved Hide resolved

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

Fixed diagnostics for read feed from bad merge

f727d43

ealsur approved these changes Mar 16, 2020

View reviewed changes

kirankumarkolli approved these changes Mar 16, 2020

View reviewed changes

j82w merged commit 4bcc82d into master Mar 16, 2020

j82w deleted the users/jawilley/diagnostics/properties branch March 16, 2020 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetClientElapsedTime to CosmosDiagnostics #1268

Add GetClientElapsedTime to CosmosDiagnostics #1268

j82w commented Mar 10, 2020 •

edited

Loading

kirankumarkolli commented Mar 11, 2020

kirankumarkolli commented Mar 11, 2020

kirankumarkolli commented Mar 11, 2020

kirankumarkolli Mar 11, 2020

j82w Mar 11, 2020

ealsur Mar 12, 2020

j82w Mar 12, 2020

FabianMeiswinkel Mar 16, 2020

j82w Mar 16, 2020

		public abstract string UserAgent { get; protected set; }

		internal abstract CosmosDiagnostics Diagnostics { get; }

Add GetClientElapsedTime to CosmosDiagnostics #1268

Add GetClientElapsedTime to CosmosDiagnostics #1268

Conversation

j82w commented Mar 10, 2020 • edited Loading

Pull Request Template

Description

Type of change

Closing issues

kirankumarkolli commented Mar 11, 2020

kirankumarkolli commented Mar 11, 2020

kirankumarkolli commented Mar 11, 2020

kirankumarkolli Mar 11, 2020

Choose a reason for hiding this comment

j82w Mar 11, 2020

Choose a reason for hiding this comment

ealsur Mar 12, 2020

Choose a reason for hiding this comment

j82w Mar 12, 2020

Choose a reason for hiding this comment

FabianMeiswinkel Mar 16, 2020

Choose a reason for hiding this comment

j82w Mar 16, 2020

Choose a reason for hiding this comment

j82w commented Mar 10, 2020 •

edited

Loading