Improved diagnostics with new models for StoreResponse, StoreResult and CosmosException #28620

kushagraThapar · 2022-05-03T01:42:29Z

Created StoreResultDiagnostics -> contains StoreResponseDiagnostics (diagnostics for store response in case of response and exception)
Added exceptionMessage and exceptionResponseHeaders to StoreResponseDiagnostics.
Added error handling logic in case of java.lang.Error.
Performance improvements related to headers in StoreResponse. Earlier all operations were O(n). Now with this change, they will be O(1).
Currently queryPlanCache is implemented as ThreadSafe LRU cache, with an instance of SynchronizedMap of fixed size 1000. This causes lock on the whole map during reads and writes (reads happen on every query call). To improve performance and removed the locking on the whole map, implemented queryPlanCache as ConcurrentHashMap with a fixed size of 1000. If customers have more than 1000 different queries, then the cache will clear after 1000 size.
Reasoning behind this change is to have queryPlanCache as a very simple cache, without any locking and performance overhead.
Reasoning behind 1000 size is to make sure customers use query spec and query params. If there is still need to increase the size, this can be done in future without any breaking change.

…nd CosmosException

azure-sdk · 2022-05-03T01:55:41Z

API change check for com.azure:azure-cosmos

API changes have been detected in com.azure:azure-cosmos. You can review API changes here

API changes

-         @Warning public static void setGatewayRequestTimelineOnDiagnostics(CosmosDiagnostics cosmosDiagnostics, RequestTimeline requestTimeline) 
-         @Warning public static void recordGatewayResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest rxDocumentServiceRequest, StoreResponse storeResponse, CosmosException exception, GlobalEndpointManager globalEndpointManager) 
+         @Warning public static void recordGatewayResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest rxDocumentServiceRequest, StoreResponse storeResponse, GlobalEndpointManager globalEndpointManager) 
+         @Warning public static void recordGatewayResponse(CosmosDiagnostics cosmosDiagnostics, RxDocumentServiceRequest rxDocumentServiceRequest, CosmosException cosmosException, GlobalEndpointManager globalEndpointManager)

… to start with

kushagraThapar · 2022-05-03T19:26:26Z

/azp run java - cosmos - tests

azure-pipelines · 2022-05-03T19:26:39Z

Azure Pipelines successfully started running 1 pipeline(s).

FabianMeiswinkel · 2022-05-11T17:51:40Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosException.java

        if (responseHeaders != null) {
-            for (Map.Entry<String, String> entry: responseHeaders.entrySet()) {
+            for (Map.Entry<String, String> entry : responseHeaders.entrySet()) {


NIT: shouldn't that extra space be reverted?

FabianMeiswinkel

Love it - much cleaner.

…rors. Also added code for throwing any java.lang.Error

FabianMeiswinkel

Thanks!

kushagraThapar · 2022-05-12T21:17:17Z

/azp run java - cosmos - tests

azure-pipelines · 2022-05-12T21:17:31Z

Azure Pipelines successfully started running 1 pipeline(s).

…sts to fail. Will investigate later

alzimmermsft · 2022-05-13T18:47:47Z

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/StoreResponseBuilder.java

    private String content;

    public static StoreResponseBuilder create() {
        return new StoreResponseBuilder();
    }

    public StoreResponseBuilder() {
-        headerEntries = new ArrayList<>();
+        headers = new HashMap<>();


Was there anything previously depending on header insertion ordering? If so, a LinkedHashMap may be a better option (though less performant)

Nothing was depending on the ordering of the headers.

xinlian12 · 2022-05-16T16:01:01Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxGatewayStoreModel.java

@@ -507,7 +506,8 @@ public Mono<RxDocumentServiceResponse> processMessage(RxDocumentServiceRequest r
                    }

                    if (Exceptions.isThroughputControlRequestRateTooLargeException(dce)) {
-                        BridgeInternal.recordGatewayResponse(request.requestContext.cosmosDiagnostics, request, null, dce, globalEndpointManager);
+                        StoreResponseDiagnostics storeResponseDiagnostics = StoreResponseDiagnostics.createStoreResponseDiagnostics(dce);
+                        BridgeInternal.recordGatewayResponse(request.requestContext.cosmosDiagnostics, request, storeResponseDiagnostics, globalEndpointManager);


do we need to check whether request.requestContext.cosmosDiagnostics != null? (similar to line 410 - 416)

Good catch, worth adding a null check here to avoid any NPEs.

xinlian12 · 2022-05-16T16:08:42Z

...ure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreReader.java

@@ -664,7 +667,8 @@ StoreResult createAndRecordStoreResult(
        StoreResult storeResult = this.createStoreResult(storeResponse, responseException, requiresValidLsn, useLocalLSNBasedHeaders, storePhysicalAddress);

        try {
-            BridgeInternal.recordResponse(request.requestContext.cosmosDiagnostics, request, storeResult, transportClient.getGlobalEndpointManager());
+            StoreResultDiagnostics storeResultDiagnostics = StoreResultDiagnostics.createStoreResultDiagnostics(storeResult);
+            BridgeInternal.recordResponse(request.requestContext.cosmosDiagnostics, request, storeResultDiagnostics, transportClient.getGlobalEndpointManager());


kind feel like createStoreResultDiagnostics can be part of the logic in recordResponse,

Valid point, will be much cleaner, will do that.

xinlian12 · 2022-05-16T16:10:33Z

...e-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResponse.java


-        return null;
+    public String getHeaderValue(String attribute) {
+        return responseHeaders.get(attribute);


the logic here seems changed a little bit compared to original, do we need to allow match by ignoreCase?

if (responseHeaderNames[i].equalsIgnoreCase(attribute)) { return responseHeaderValues[i]; }

Valid point, I think worth checking for both cases to make it case insensitive.

xinlian12 · 2022-05-16T16:18:17Z

...c/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResponseDiagnostics.java

+        this.requestTimeline = storeResponse.getRequestTimeline();
+        this.channelAcquisitionTimeline = storeResponse.getChannelAcquisitionTimeline();
+        this.rntbdChannelTaskQueueSize = storeResponse.getRntbdChannelTaskQueueSize();
+        this.rntbdEndpointStatistics = storeResponse.getEndpointStsts();


Is it a typo or abbreviation ? storeResponse.getEndpointStsts

It looks like a typo, I will fix this API name.

xinlian12 · 2022-05-16T16:25:14Z

...ure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResult.java

+            cosmosException.getResponseHeaders().put(HttpConstants.HttpHeaders.REQUEST_CHARGE, totalRequestChargeString);
+        } else {
+            // Set total charge as final charge for the response.
+            response.getResponseHeaders().put(HttpConstants.HttpHeaders.REQUEST_CHARGE, totalRequestChargeString);


the logic here also seems a little different, also because of ignoreCase check in the original logic.
Do we ever have a mix use of uppercase/lowercase of the header names?

xinlian12 · 2022-05-16T16:31:20Z

...src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResultDiagnostics.java

+                              SerializerProvider serializerProvider) throws IOException {
+            StoreResponseDiagnostics storeResponseDiagnostics = storeResultDiagnostics.getStoreResponseDiagnostics();
+            jsonGenerator.writeStartObject();
+            jsonGenerator.writeObjectField("storePhysicalAddress", storeResultDiagnostics.storePhysicalAddressAsString);


should this be writeStringField?

Valid point, I will change it.

xinlian12 · 2022-05-16T16:33:18Z

...src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResultDiagnostics.java

+            jsonGenerator.writeNumberField("requestCharge", storeResponseDiagnostics.getRequestCharge());
+            jsonGenerator.writeNumberField("itemLSN", storeResultDiagnostics.itemLSN);
+            jsonGenerator.writeStringField("sessionToken", storeResponseDiagnostics.getSessionTokenAsString());
+            jsonGenerator.writeObjectField("backendLatencyInMs", storeResultDiagnostics.backendLatencyInMs);


should this be writeNumberField?

I don't want to change any existing fields, the way they were being written, but will make sure to fix the newly added string fields.

xinlian12 · 2022-05-16T16:34:19Z

...src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreResultDiagnostics.java

+
+            this.writeNonNullObjectField(jsonGenerator,"transportRequestChannelAcquisitionContext", storeResponseDiagnostics.getChannelAcquisitionTimeline());
+
+            jsonGenerator.writeObjectField("rntbdRequestLengthInBytes", storeResponseDiagnostics.getRntbdRequestLength());


same question here: should we use writeNumberField for line 176 - 181?

I don't want to change any existing fields, the way they were being written, but will make sure to fix the newly added string fields.

xinlian12

LGTM, so many improvements 👍

xinlian12 · 2022-05-16T17:02:02Z

Please also update change log

check-enforcer · 2022-05-16T21:05:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

kushagraThapar · 2022-05-16T21:19:38Z

/azp run java - cosmos - tests

azure-pipelines · 2022-05-16T21:19:52Z

Azure Pipelines successfully started running 1 pipeline(s).

…shagraThapar/azure-sdk-for-java into azure_cosmos_diagnostics_improvements

Improved diagnostics with new models for StoreResponse, StoreResult a…

e47acdf

…nd CosmosException

kushagraThapar requested review from FabianMeiswinkel, kirankumarkolli, mbhaskar, simplynaveen20, xinlian12, milismsft and aayush3011 as code owners May 3, 2022 01:42

ghost added the Cosmos label May 3, 2022

kushagraThapar added 2 commits May 2, 2022 20:16

Fixed spot bugs related to storeResult.getException()

5c1c95f

Updated query plan cache to ConcurrentHashMap with fixed size of 1000…

e9d74f1

… to start with

FabianMeiswinkel reviewed May 11, 2022

View reviewed changes

FabianMeiswinkel approved these changes May 11, 2022

View reviewed changes

Added exception response headers and message to direct and gateway er…

b4443d3

…rors. Also added code for throwing any java.lang.Error

FabianMeiswinkel approved these changes May 12, 2022

View reviewed changes

kushagraThapar added 4 commits May 13, 2022 10:00

Added unit tests for StoreReader and ConsistencyWriter

9d46be1

Disabled StoreReader unit test for error since it is causing other te…

76c4fc4

…sts to fail. Will investigate later

Commented out the broken test

0ae604b

Reverted StoreReaderTest

a0ab289

alzimmermsft reviewed May 13, 2022

View reviewed changes

alzimmermsft approved these changes May 13, 2022

View reviewed changes

kushagraThapar added 3 commits May 13, 2022 12:02

Removed mockito-inline

185e6d5

Fixed StoreReaderTest static mocking

d10e363

Fixed ConsistencyWriterTest static mocking

803bc74

xinlian12 reviewed May 16, 2022

View reviewed changes

xinlian12 approved these changes May 16, 2022

View reviewed changes

Code review comments and changelog addition

77bbe3d

Merge branch 'main' into azure_cosmos_diagnostics_improvements

9a8dc72

kushagraThapar added 2 commits May 16, 2022 17:41

Fixed some test cases

6a387c0

Merge branch 'azure_cosmos_diagnostics_improvements' of github.com:ku…

7f18224

…shagraThapar/azure-sdk-for-java into azure_cosmos_diagnostics_improvements

kushagraThapar merged commit 957740d into Azure:main May 17, 2022

kushagraThapar mentioned this pull request Jun 3, 2022

Fixed Store Response headers case insensitivity #29210

Closed

FabianMeiswinkel mentioned this pull request Jun 7, 2022

Fixed Store Response headers case insensitivity (alternative) #29268

Merged

6 tasks

kushagraThapar mentioned this pull request Apr 13, 2023

Some misc fixes in Cosmos Java SDK #34432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved diagnostics with new models for StoreResponse, StoreResult and CosmosException #28620

Improved diagnostics with new models for StoreResponse, StoreResult and CosmosException #28620

kushagraThapar commented May 3, 2022 •

edited

Loading

azure-sdk commented May 3, 2022 •

edited

Loading

kushagraThapar commented May 3, 2022

azure-pipelines bot commented May 3, 2022

FabianMeiswinkel May 11, 2022

FabianMeiswinkel left a comment

FabianMeiswinkel left a comment

kushagraThapar commented May 12, 2022

azure-pipelines bot commented May 12, 2022

alzimmermsft May 13, 2022

kushagraThapar May 13, 2022

xinlian12 May 16, 2022

kushagraThapar May 16, 2022

xinlian12 May 16, 2022 •

edited

Loading

kushagraThapar May 16, 2022

xinlian12 May 16, 2022 •

edited

Loading

kushagraThapar May 16, 2022

xinlian12 May 16, 2022 •

edited

Loading

kushagraThapar May 16, 2022

xinlian12 May 16, 2022 •

edited

Loading

xinlian12 May 16, 2022

kushagraThapar May 16, 2022

xinlian12 May 16, 2022

kushagraThapar May 16, 2022

xinlian12 May 16, 2022 •

edited

Loading

kushagraThapar May 16, 2022

xinlian12 left a comment

xinlian12 commented May 16, 2022

check-enforcer bot commented May 16, 2022

kushagraThapar commented May 16, 2022

azure-pipelines bot commented May 16, 2022


		this.writeNonNullObjectField(jsonGenerator,"transportRequestChannelAcquisitionContext", storeResponseDiagnostics.getChannelAcquisitionTimeline());

		jsonGenerator.writeObjectField("rntbdRequestLengthInBytes", storeResponseDiagnostics.getRntbdRequestLength());

Improved diagnostics with new models for StoreResponse, StoreResult and CosmosException #28620

Improved diagnostics with new models for StoreResponse, StoreResult and CosmosException #28620

Conversation

kushagraThapar commented May 3, 2022 • edited Loading

azure-sdk commented May 3, 2022 • edited Loading

kushagraThapar commented May 3, 2022

azure-pipelines bot commented May 3, 2022

Choose a reason for hiding this comment

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

kushagraThapar commented May 12, 2022

azure-pipelines bot commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinlian12 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinlian12 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinlian12 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinlian12 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinlian12 May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinlian12 left a comment

Choose a reason for hiding this comment

xinlian12 commented May 16, 2022

check-enforcer bot commented May 16, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

kushagraThapar commented May 16, 2022

azure-pipelines bot commented May 16, 2022

kushagraThapar commented May 3, 2022 •

edited

Loading

azure-sdk commented May 3, 2022 •

edited

Loading

xinlian12 May 16, 2022 •

edited

Loading

xinlian12 May 16, 2022 •

edited

Loading

xinlian12 May 16, 2022 •

edited

Loading

xinlian12 May 16, 2022 •

edited

Loading

xinlian12 May 16, 2022 •

edited

Loading