Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed resource address in CosmosException. #17279

Merged
merged 4 commits into from
Nov 17, 2020
Merged

Fixed resource address in CosmosException. #17279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d22411
Fixed resource address in CosmosException. Added new API to expose re…
kushagraThapar Nov 6, 2020
23ea640
Fixed resource address in GATEWAY mode to have full physical address
kushagraThapar Nov 10, 2020
caf53fb
Setting physical resource address in tests
kushagraThapar Nov 11, 2020
9381062
Merge branch 'master' into fix_resource_address_diagnostics_string
kushagraThapar Nov 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/BridgeInternal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,14 +387,8 @@ public static CosmosException createCosmosException(int statusCode, String error
}

@Warning(value = INTERNAL_USE_ONLY_WARNING)
public static CosmosException createCosmosException(int statusCode, Exception innerException) {
return new CosmosException(statusCode, null, null, innerException);
}

@Warning(value = INTERNAL_USE_ONLY_WARNING)
public static CosmosException createCosmosException(int statusCode, CosmosError cosmosErrorResource,
Map<String, String> responseHeaders) {
return new CosmosException(/* resourceAddress */ null, statusCode, cosmosErrorResource, responseHeaders);
public static CosmosException createCosmosException(String resourceAddress, int statusCode, Exception innerException) {
return new CosmosException(resourceAddress, statusCode, null, null, innerException);
}

@Warning(value = INTERNAL_USE_ONLY_WARNING)
Expand Down
10 changes: 10 additions & 0 deletions sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosDiagnostics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.net.URI;
import java.time.Duration;
import java.util.Set;

/**
* This class represents response diagnostic statistics associated with a request to Azure Cosmos DB
Expand Down Expand Up @@ -77,6 +79,14 @@ public Duration getDuration() {
return this.clientSideRequestStatistics.getDuration();
}

/**
* Regions contacted for this request
* @return set of regions contacted for this request
*/
public Set<URI> getRegionsContacted() {
return this.clientSideRequestStatistics.getRegionsContacted();
}

FeedResponseDiagnostics getFeedResponseDiagnostics() {
return feedResponseDiagnostics;
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ CosmosException setDiagnostics(CosmosDiagnostics cosmosDiagnostics) {
@Override
public String toString() {
return getClass().getSimpleName() + "{" + "userAgent=" + USER_AGENT + ", error=" + cosmosError + ", resourceAddress='"
+ resourceAddress + "', requestUri='" + (requestUri != null ? requestUri.getURIAsString() : null) + "', statusCode=" + statusCode + ", message=" + getMessage()
+ resourceAddress + ", statusCode=" + statusCode + ", message=" + getMessage()
+ ", causeInfo=" + causeInfo() + ", responseHeaders=" + responseHeaders + ", requestHeaders="
Copy link
Member

@xinlian12 xinlian12 Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we remove requestUri?
And I think currently we are mixing using resourceAddress and requestUri:
for example in RxGatewayStoreModel, we are passing resourceId to resourceAddress of CosmosException
But in RntbdRequestManager, we are passing physical address to resourceAddress of CosmosException

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed requestUri from CosmosException string representation, as it is never set.
In RxGatewayStoreModel, we are passing request.getResourceAddress()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it looks for gateway -> resourceAddress='dbs/RxJava.SDKTest.SharedDatabase_20201109T182237_GSh/colls/3d2be199-e23b-4af2-a755-510d7068ff97/docs/18b9d280-e899-4300-ae0c-5fe56411aa18,

And for direct -> resourceAddress='rntbd://cdb-ms-prod-westus2-fd26.documents.azure.com:14376/apps/db2a149b-bdb4-4755-b26e-56d1a86cdd4e/services/8ce44d24-fe68-49ae-9784-94b322fa46a3/partitions/003a20cb-a187-4b3a-9bc9-306be0217c36/replicas/132494067072942319s/,

which both are resource addresses.

Copy link
Member

@xinlian12 xinlian12 Nov 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kushagra~
I just wonder should we also capture the server info for gateway mode: (like following)
https://127.0.0.1:8081/dbs/RxJava.SDKTest.SharedDatabase_20201109T193921_erZ/colls/7d5337a3-30f7-46bb-b318-104a5dcfb721

Currently, the resourceAddress for gateway mode only contains the resourceId or full name, but not a complete uri. I think that probably why there is requestResource and requestUri there previously? (not sure though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, not sure how we can capture that, any ideas ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will try to do that, so we can re-use it, will update this thread once I have reached a conclusion :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the direct mode? isn't that set in the direct mode? @kushagraThapar could you please check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moderakh - It is not used anywhere, other than at this code piece - https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/StoreReader.java#L808
Which I am not even sure, if that is correct or not, but don't plan to touch it :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xinlian12 @FabianMeiswinkel - I have added the full resource address in gateway mode too, please check.
Also updated the PR with sample.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kushagra, looks good to me~

+ filterSensitiveData(requestHeaders) + '}';
}
Expand Down
4 changes: 2 additions & 2 deletions ...osmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxGatewayStoreModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ private Mono<RxDocumentServiceResponse> toDocumentServiceResponse(Mono<HttpRespo
if (!(exception instanceof CosmosException)) {
// wrap in CosmosException
logger.error("Network failure", exception);
dce = BridgeInternal.createCosmosException(0, exception);
dce = BridgeInternal.createCosmosException(request.getResourceAddress(), 0, exception);
BridgeInternal.setRequestHeaders(dce, request.getHeaders());
} else {
dce = (CosmosException) exception;
Expand Down Expand Up @@ -342,7 +342,7 @@ private void validateOrThrow(RxDocumentServiceRequest request,
String.format("%s, StatusCode: %s", cosmosError.getMessage(), statusCodeString),
cosmosError.getPartitionedQueryExecutionInfo());

CosmosException dce = BridgeInternal.createCosmosException(statusCode, cosmosError, headers.toMap());
CosmosException dce = BridgeInternal.createCosmosException(request.getResourceAddress(), statusCode, cosmosError, headers.toMap());
BridgeInternal.setRequestHeaders(dce, request.getHeaders());
throw dce;
}
Expand Down
8 changes: 4 additions & 4 deletions ...src/main/java/com/azure/cosmos/implementation/directconnectivity/GatewayAddressCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ public Mono<List<Address>> getServerAddressesViaGatewayAsync(
Duration.ofSeconds(Configs.getAddressRefreshResponseTimeoutInSeconds())));
}

Mono<RxDocumentServiceResponse> dsrObs = HttpClientUtils.parseResponseAsync(clientContext, httpResponseMono, httpRequest);
Mono<RxDocumentServiceResponse> dsrObs = HttpClientUtils.parseResponseAsync(request, clientContext, httpResponseMono, httpRequest);
return dsrObs.map(
dsr -> {
MetadataDiagnosticsContext metadataDiagnosticsContext =
Expand Down Expand Up @@ -386,7 +386,7 @@ public Mono<List<Address>> getServerAddressesViaGatewayAsync(
if (!(exception instanceof CosmosException)) {
// wrap in CosmosException
logger.error("Network failure", exception);
dce = BridgeInternal.createCosmosException(0, exception);
dce = BridgeInternal.createCosmosException(request.getResourceAddress(), 0, exception);
BridgeInternal.setRequestHeaders(dce, request.getHeaders());
} else {
dce = (CosmosException) exception;
Expand Down Expand Up @@ -588,7 +588,7 @@ public Mono<List<Address>> getMasterAddressesViaGatewayAsync(
Duration.ofSeconds(Configs.getAddressRefreshResponseTimeoutInSeconds())));
}

Mono<RxDocumentServiceResponse> dsrObs = HttpClientUtils.parseResponseAsync(this.clientContext, httpResponseMono, httpRequest);
Mono<RxDocumentServiceResponse> dsrObs = HttpClientUtils.parseResponseAsync(request, this.clientContext, httpResponseMono, httpRequest);

return dsrObs.map(
dsr -> {
Expand Down Expand Up @@ -618,7 +618,7 @@ public Mono<List<Address>> getMasterAddressesViaGatewayAsync(
if (!(exception instanceof CosmosException)) {
// wrap in CosmosException
logger.error("Network failure", exception);
dce = BridgeInternal.createCosmosException(0, exception);
dce = BridgeInternal.createCosmosException(request.getResourceAddress(), 0, exception);
BridgeInternal.setRequestHeaders(dce, request.getHeaders());
} else {
dce = (CosmosException) exception;
Expand Down
15 changes: 9 additions & 6 deletions ...mos/src/main/java/com/azure/cosmos/implementation/directconnectivity/HttpClientUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.azure.cosmos.implementation.CosmosError;
import com.azure.cosmos.implementation.DiagnosticsClientContext;
import com.azure.cosmos.implementation.HttpConstants;
import com.azure.cosmos.implementation.RxDocumentServiceRequest;
import com.azure.cosmos.implementation.RxDocumentServiceResponse;
import com.azure.cosmos.implementation.apachecommons.lang.StringUtils;
import com.azure.cosmos.implementation.http.HttpRequest;
Expand All @@ -16,7 +17,10 @@

public class HttpClientUtils {

static Mono<RxDocumentServiceResponse> parseResponseAsync(DiagnosticsClientContext diagnosticsClientContext, Mono<HttpResponse> httpResponse, HttpRequest httpRequest) {
static Mono<RxDocumentServiceResponse> parseResponseAsync(RxDocumentServiceRequest request,
DiagnosticsClientContext diagnosticsClientContext,
Mono<HttpResponse> httpResponse,
HttpRequest httpRequest) {
return httpResponse.flatMap(response -> {
if (response.statusCode() < HttpConstants.StatusCodes.MINIMUM_STATUSCODE_AS_ERROR_GATEWAY) {

Expand All @@ -27,20 +31,19 @@ static Mono<RxDocumentServiceResponse> parseResponseAsync(DiagnosticsClientConte

} else {
return HttpClientUtils
.createDocumentClientException(response).flatMap(Mono::error);
.createDocumentClientException(request, response).flatMap(Mono::error);
}
});
}

private static Mono<CosmosException> createDocumentClientException(HttpResponse httpResponse) {
private static Mono<CosmosException> createDocumentClientException(RxDocumentServiceRequest request, HttpResponse httpResponse) {
Mono<String> readStream = httpResponse.bodyAsString().switchIfEmpty(Mono.just(StringUtils.EMPTY));

return readStream.map(body -> {
CosmosError cosmosError = new CosmosError(body);

// TODO: we should set resource address in the Document Client Exception
return BridgeInternal.createCosmosException(httpResponse.statusCode(), cosmosError,
httpResponse.headers().toMap());
return BridgeInternal.createCosmosException(request.getResourceAddress(), httpResponse.statusCode(),
cosmosError, httpResponse.headers().toMap());
});
}
}
9 changes: 5 additions & 4 deletions ...in/java/com/azure/cosmos/implementation/directconnectivity/rntbd/RntbdRequestManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,9 @@ private void messageReceived(final ChannelHandlerContext context, final RntbdRes

// ..Create CosmosException based on status and sub-status codes

final String resourceAddress = requestRecord.args().physicalAddress() != null ?
requestRecord.args().physicalAddress().toString() : null;

switch (status.code()) {

case StatusCodes.BADREQUEST:
Expand Down Expand Up @@ -825,9 +828,6 @@ private void messageReceived(final ChannelHandlerContext context, final RntbdRes

case StatusCodes.REQUEST_TIMEOUT:
Exception inner = new RequestTimeoutException(error, lsn, partitionKeyRangeId, responseHeaders);
String resourceAddress = requestRecord.args().physicalAddress() != null ?
requestRecord.args().physicalAddress().toString() : null;

cause = new GoneException(resourceAddress, error, lsn, partitionKeyRangeId, responseHeaders, inner);
break;

Expand All @@ -848,9 +848,10 @@ private void messageReceived(final ChannelHandlerContext context, final RntbdRes
break;

default:
cause = BridgeInternal.createCosmosException(status.code(), error, responseHeaders);
cause = BridgeInternal.createCosmosException(resourceAddress, status.code(), error, responseHeaders);
break;
}
BridgeInternal.setResourceAddress(cause, resourceAddress);

requestRecord.completeExceptionally(cause);
}
Expand Down
10 changes: 10 additions & 0 deletions sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosDiagnosticsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ public void gatewayDiagnostics() {
assertThat(diagnostics).contains("\"serializationType\":\"PARTITION_KEY_FETCH_SERIALIZATION\"");
assertThat(diagnostics).contains("\"userAgent\":\"" + Utils.getUserAgent() + "\"");
assertThat(createResponse.getDiagnostics().getDuration()).isNotNull();
assertThat(createResponse.getDiagnostics().getRegionsContacted()).isNotNull();
// TODO: (nakumars) - Uncomment the following line after your client telemetry fix
// assertThat(createResponse.getDiagnostics().getRegionsContacted()).isNotEmpty();
validateTransportRequestTimelineGateway(diagnostics);
validateJson(diagnostics);
} finally {
Expand Down Expand Up @@ -176,6 +179,10 @@ public void gatewayDiagnosticsOnException() {
assertThat(diagnostics).contains("\"statusCode\":404");
assertThat(diagnostics).contains("\"operationType\":\"Read\"");
assertThat(diagnostics).contains("\"userAgent\":\"" + Utils.getUserAgent() + "\"");
assertThat(diagnostics).doesNotContain(("\"resourceAddress\":null"));
assertThat(createResponse.getDiagnostics().getRegionsContacted()).isNotNull();
// TODO: (nakumars) - Uncomment the following line after your client telemetry fix
// assertThat(createResponse.getDiagnostics().getRegionsContacted()).isNotEmpty();
assertThat(exception.getDiagnostics().getDuration()).isNotNull();
validateTransportRequestTimelineGateway(diagnostics);
validateJson(diagnostics);
Expand Down Expand Up @@ -219,6 +226,7 @@ public void directDiagnostics() {
assertThat(diagnostics).contains("\"metaDataName\":\"SERVER_ADDRESS_LOOKUP\"");
assertThat(diagnostics).contains("\"serializationType\":\"PARTITION_KEY_FETCH_SERIALIZATION\"");
assertThat(diagnostics).contains("\"userAgent\":\"" + Utils.getUserAgent() + "\"");
assertThat(createResponse.getDiagnostics().getRegionsContacted()).isNotEmpty();
assertThat(createResponse.getDiagnostics().getDuration()).isNotNull();
validateTransportRequestTimelineDirect(diagnostics);
validateJson(diagnostics);
Expand Down Expand Up @@ -417,6 +425,8 @@ public void directDiagnosticsOnException() {
String diagnostics = exception.getDiagnostics().toString();
assertThat(exception.getStatusCode()).isEqualTo(HttpConstants.StatusCodes.NOTFOUND);
assertThat(diagnostics).contains("\"connectionMode\":\"DIRECT\"");
assertThat(diagnostics).doesNotContain(("\"resourceAddress\":null"));
assertThat(exception.getDiagnostics().getRegionsContacted()).isNotEmpty();
assertThat(exception.getDiagnostics().getDuration()).isNotNull();
validateJson(diagnostics);
// TODO https://github.com/Azure/azure-sdk-for-java/issues/8035
Expand Down
4 changes: 2 additions & 2 deletions sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,14 @@ public void headerNotNull2() {

@Test(groups = { "unit" })
public void headerNotNull3() {
CosmosException dce = BridgeInternal.createCosmosException(0, new RuntimeException());
CosmosException dce = BridgeInternal.createCosmosException(null, 0, new RuntimeException());
assertThat(dce.getResponseHeaders()).isNotNull();
assertThat(dce.getResponseHeaders()).isEmpty();
}

@Test(groups = { "unit" })
public void headerNotNull4() {
CosmosException dce = BridgeInternal.createCosmosException(0, (CosmosError) null, (Map<String, String>) null);
CosmosException dce = BridgeInternal.createCosmosException(null, 0, (CosmosError) null, (Map<String, String>) null);
assertThat(dce.getResponseHeaders()).isNotNull();
assertThat(dce.getResponseHeaders()).isEmpty();
}
Expand Down
Loading