Skip to content

Commit

Permalink
Added Changelog and bug fixes (#27072)
Browse files Browse the repository at this point in the history
### Packages impacted by this PR
@azure/cosmos

### Describe the problem that is addressed by this PR
- Added changelog entries for v4.0.0 release.
- Moved logic for addition of `getClientConfig` from `ClientContext` to
`DiagnosticNodeInternal`, so that all such logic is at one place.
- In case of exception, the logic to add request payload to diagnostic
(with appropriate diagnostic level) was missing. added that.
- Added test case for verifying scope of diagnostic level is limited to
`CosmosClient` instance.

### What are the possible designs available to address the problem? If
there are more than one possible design, why was the one in this PR
chosen?


### Are there test cases added in this PR? _(If not, why?)_
Yes

### Provide a list of related PRs _(if any)_


### Command used to generate this PR:**_(Applicable only to SDK release
request PRs)_

### Checklists
- [ ] Added impacted package name to the issue description
- [ ] Does this PR needs any fixes in the SDK Generator?** _(If so,
create an Issue in the
[Autorest/typescript](https://github.com/Azure/autorest.typescript)
repository and link it here)_
- [ ] Added a changelog (if necessary)
  • Loading branch information
v1k1 authored Sep 11, 2023
1 parent 8c6a412 commit 0237f04
Show file tree
Hide file tree
Showing 10 changed files with 90 additions and 34 deletions.
19 changes: 12 additions & 7 deletions sdk/cosmosdb/cosmos/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,20 @@
## 4.0.0 (Unreleased)

### Features Added
- Retries for system timeouts (Timeout error) and service unavailability (503). [#26039] (https://github.com/Azure/azure-sdk-for-js/pull/26039)
- Added support for hierarchical partitions.
- Added Diagnostics to all response objects, i.e. ResourceResponse (parent class for ItemRespone, ContainerResponse etc.), FeedResponse, ChangeFeedIteratorResponse, ErrorResponse, BulkOperationResponse.

### Breaking Changes

- Added Changefeed support for partition keys, feed ranges, and entire container. [#18062](https://github.com/Azure/azure-sdk-for-js/issues/18062)
- Added Diagnostics to all response objects, i.e. ResourceResponse (parent class for ItemRespone, ContainerResponse etc.), FeedResponse, ChangeFeedIteratorResponse,
ErrorResponse, BulkOperationResponse. [#21177](https://github.com/Azure/azure-sdk-for-js/issues/21177)
- Added support for hierarchical partitions. [#23416](https://github.com/Azure/azure-sdk-for-js/issues/23416)
- Added support of index metrics. [#20194](https://github.com/Azure/azure-sdk-for-js/issues/20194)
- Improved the retry utility to align with other language SDKs. Now, it automatically retries requests on the next available region when encountering HTTP 503 errors (Service Unavailable)
and handles HTTP timeouts more effectively, enhancing the SDK's reliability. [#23475](https://github.com/Azure/azure-sdk-for-js/issues/23475)
- Added priority based throttling. [docs](https://devblogs.microsoft.com/cosmosdb/introducing-priority-based-execution-in-azure-cosmos-db-preview/) [#26393](https://github.com/Azure/azure-sdk-for-js/pull/26393/files)
### Bugs Fixed
- Updated response codes for the getDatabase() method. [#25932](https://github.com/Azure/azure-sdk-for-js/issues/25932)
- Fix Upsert operation failing when partition key of container is `/id` and `/id` is missing in the document. [#21383](https://github.com/Azure/azure-sdk-for-js/issues/21383)

### Other Changes
### Breaking Changes
- The definition of PartitionKey is changed, PartitionKeyDefinition is now a independent type. [#23416](https://github.com/Azure/azure-sdk-for-js/issues/23416)

## 3.17.3 (2023-02-13)

Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmosdb/cosmos/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ There are 3 Cosmos Diagnostic levels, info, debug and debug-unsafe. Where only i
```js
const client = new CosmosClient({ endpoint, key, diagnosticLevel: CosmosDbDiagnosticLevel.debug });
```
- Using environment variables
- Using environment variables. (Diagnostic level set by Environment variable has higher priority over setting it through client options.)
```bash
export AZURE_COSMOSDB_DIAGNOSTICS_LEVEL="debug"
```
Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmosdb/cosmos/review/cosmos.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export class ClientContext {
diagnosticNode: DiagnosticNodeInternal;
}): Promise<Response_2<T>>;
// (undocumented)
getClientConfig(): ClientConfigDiagnostic | undefined;
getClientConfig(): ClientConfigDiagnostic;
getDatabaseAccount(diagnosticNode: DiagnosticNodeInternal, options?: RequestOptions): Promise<Response_2<DatabaseAccount>>;
// (undocumented)
getQueryPlan(path: string, resourceType: ResourceType, resourceId: string, query: SqlQuerySpec | string, options: FeedOptions, diagnosticNode: DiagnosticNodeInternal): Promise<Response_2<PartitionedQueryExecutionInfo>>;
Expand Down
9 changes: 2 additions & 7 deletions sdk/cosmosdb/cosmos/src/ClientContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import {
} from "./diagnostics/DiagnosticWriter";
import { DefaultDiagnosticFormatter, DiagnosticFormatter } from "./diagnostics/DiagnosticFormatter";
import { CosmosDbDiagnosticLevel } from "./diagnostics/CosmosDbDiagnosticLevel";
import { allowTracing } from "./diagnostics/diagnosticLevelComparator";

const logger: AzureLogger = createClientLogger("ClientContext");

Expand Down Expand Up @@ -971,11 +970,7 @@ export class ClientContext {
};
}

public getClientConfig(): ClientConfigDiagnostic | undefined {
if (allowTracing(CosmosDbDiagnosticLevel.debug, this.diagnosticLevel)) {
return this.clientConfig;
} else {
return undefined;
}
public getClientConfig(): ClientConfigDiagnostic {
return this.clientConfig;
}
}
7 changes: 7 additions & 0 deletions sdk/cosmosdb/cosmos/src/common/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,13 @@ export function getContainerLink(link: string): string {
return link.split("/").slice(0, 4).join("/");
}

/**
* @hidden
*/
export function prepareURL(endpoint: string, path: string): string {
return trimSlashes(endpoint) + path;
}

/**
* @hidden
*/
Expand Down
50 changes: 36 additions & 14 deletions sdk/cosmosdb/cosmos/src/diagnostics/DiagnosticNodeInternal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { getCurrentTimestampInMs } from "../utils/time";
import { CosmosDbDiagnosticLevel } from "./CosmosDbDiagnosticLevel";
import { CosmosHeaders } from "../queryExecutionContext/CosmosHeaders";
import { HttpHeaders, PipelineResponse } from "@azure/core-rest-pipeline";
import { Constants, OperationType, ResourceType } from "../common";
import { Constants, OperationType, ResourceType, prepareURL } from "../common";
import { allowTracing } from "./diagnosticLevelComparator";

/**
Expand Down Expand Up @@ -110,12 +110,6 @@ export class DiagnosticNodeInternal implements DiagnosticNode {
resourceType: gatewayRequest.resourceType,
requestPayloadLengthInBytes: gatewayRequest.requestPayloadLengthInBytes,
};
this.addData({
requestPayloadLengthInBytes: gatewayRequest.requestPayloadLengthInBytes,
responsePayloadLengthInBytes: gatewayRequest.responsePayloadLengthInBytes,
startTimeUTCInMs: gatewayRequest.startTimeUTCInMs,
durationInMs: gatewayRequest.durationInMs,
});

if (allowTracing(CosmosDbDiagnosticLevel.debugUnsafe, this.diagnosticLevel)) {
requestData = {
Expand All @@ -125,10 +119,14 @@ export class DiagnosticNodeInternal implements DiagnosticNode {
responseBody: pipelineResponse.bodyAsText,
url: url,
};
this.addData({
requestData,
});
}
this.addData({
requestPayloadLengthInBytes: gatewayRequest.requestPayloadLengthInBytes,
responsePayloadLengthInBytes: gatewayRequest.responsePayloadLengthInBytes,
startTimeUTCInMs: gatewayRequest.startTimeUTCInMs,
durationInMs: gatewayRequest.durationInMs,
requestData,
});
this.diagnosticCtx.recordNetworkCall(gatewayRequest);
}

Expand All @@ -144,20 +142,38 @@ export class DiagnosticNodeInternal implements DiagnosticNode {
responseHeaders: CosmosHeaders
): void {
this.addData({ failedAttempty: true });
const requestPayloadLengthInBytes = calculateRequestPayloadLength(requestContext);
this.diagnosticCtx.recordFailedAttempt(
{
activityId: responseHeaders[Constants.HttpHeaders.ActivityId] as string,
startTimeUTCInMs,
durationInMs: getCurrentTimestampInMs() - startTimeUTCInMs,
statusCode,
subStatusCode: substatusCode,
requestPayloadLengthInBytes: calculateRequestPayloadLength(requestContext),
requestPayloadLengthInBytes,
responsePayloadLengthInBytes: 0,
operationType: requestContext.operationType,
resourceType: requestContext.resourceType,
},
retryAttemptNumber
);
let requestData: any = {
OperationType: requestContext.operationType,
resourceType: requestContext.resourceType,
requestPayloadLengthInBytes,
};
if (allowTracing(CosmosDbDiagnosticLevel.debugUnsafe, this.diagnosticLevel)) {
requestData = {
...requestData,
headers: this.sanitizeHeaders(requestContext.headers),
requestBody: requestContext.body,
url: prepareURL(requestContext.endpoint, requestContext.path),
};
}
this.addData({
failedAttempty: true,
requestData,
});
}

/**
Expand Down Expand Up @@ -191,11 +207,14 @@ export class DiagnosticNodeInternal implements DiagnosticNode {
*/
public addChildNode(
child: DiagnosticNodeInternal,
level: CosmosDbDiagnosticLevel,
metadataType?: MetadataLookUpType
): DiagnosticNodeInternal {
this.diagnosticCtx.mergeDiagnostics(child.diagnosticCtx, metadataType);
child.parent = this;
this.children.push(child);
if (allowTracing(level, this.diagnosticLevel)) {
child.parent = this;
this.children.push(child);
}
return child;
}

Expand Down Expand Up @@ -254,11 +273,14 @@ export class DiagnosticNodeInternal implements DiagnosticNode {
* Convert to CosmosDiagnostics
* @internal
*/
public toDiagnostic(clientConfig?: ClientConfigDiagnostic): CosmosDiagnostics {
public toDiagnostic(clientConfigDiagnostic: ClientConfigDiagnostic): CosmosDiagnostics {
const rootNode = getRootNode(this);
const diagnostiNode = allowTracing(CosmosDbDiagnosticLevel.debug, this.diagnosticLevel)
? rootNode.toDiagnosticNode()
: undefined;
const clientConfig = allowTracing(CosmosDbDiagnosticLevel.debug, this.diagnosticLevel)
? clientConfigDiagnostic
: undefined;
const cosmosDiagnostic = new CosmosDiagnostics(
this.diagnosticCtx.getClientSideStats(),
diagnostiNode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { SqlQuerySpec } from "./SqlQuerySpec";
import { DiagnosticNodeInternal, DiagnosticNodeType } from "../diagnostics/DiagnosticNodeInternal";
import { addDignosticChild } from "../utils/diagnostics";
import { MetadataLookUpType } from "../CosmosDiagnostics";
import { CosmosDbDiagnosticLevel } from "../diagnostics/CosmosDbDiagnosticLevel";

/** @hidden */
const logger: AzureLogger = createClientLogger("parallelQueryExecutionContextBase");
Expand Down Expand Up @@ -354,6 +355,7 @@ export abstract class ParallelQueryExecutionContextBase implements ExecutionCont
if (!this.diagnosticNodeWrapper.consumed) {
diagnosticNode.addChildNode(
this.diagnosticNodeWrapper.diagnosticNode,
CosmosDbDiagnosticLevel.debug,
MetadataLookUpType.QueryPlanLookUp
);
this.diagnosticNodeWrapper.diagnosticNode = undefined;
Expand Down
4 changes: 2 additions & 2 deletions sdk/cosmosdb/cosmos/src/request/RequestHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
createHttpHeaders,
PipelineResponse,
} from "@azure/core-rest-pipeline";
import { trimSlashes } from "../common";
import { prepareURL } from "../common";
import { Constants } from "../common/constants";
import { executePlugins, PluginOn } from "../plugins/Plugin";
import * as RetryUtility from "../retry/retryUtility";
Expand Down Expand Up @@ -69,7 +69,7 @@ async function httpRequest(
}

const httpsClient = getCachedDefaultHttpClient();
const url = trimSlashes(requestContext.endpoint) + requestContext.path;
const url = prepareURL(requestContext.endpoint, requestContext.path);
const reqHeaders = createHttpHeaders(requestContext.headers as any);
const pipelineRequest = createPipelineRequest({
url,
Expand Down
4 changes: 2 additions & 2 deletions sdk/cosmosdb/cosmos/src/utils/diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ export async function withMetadataDiagnostics<
);
try {
const response: any = await callback(diagnosticNodeForMetadataCall);
node.addChildNode(diagnosticNodeForMetadataCall, type);
node.addChildNode(diagnosticNodeForMetadataCall, CosmosDbDiagnosticLevel.debug, type);
return response;
} catch (e) {
node.addChildNode(diagnosticNodeForMetadataCall, type);
node.addChildNode(diagnosticNodeForMetadataCall, CosmosDbDiagnosticLevel.debug, type);
throw e;
}
}
Expand Down
25 changes: 25 additions & 0 deletions sdk/cosmosdb/cosmos/test/internal/unit/diagnostics.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,31 @@ describe("Diagnostic Unit Tests", function (this: Suite) {
expect(determineDiagnosticLevel(level, undefined)).to.eql(level);
});
});
it("Check setting of diagnostic level", async function () {
// Testing scope of diagnostic level is limited to an instance of CosmosDB client.
const clientInfo = new CosmosClient({
endpoint: "https://localhost",
diagnosticLevel: CosmosDbDiagnosticLevel.info,
});
const clientDebug = new CosmosClient({
endpoint: "https://localhost",
diagnosticLevel: CosmosDbDiagnosticLevel.debug,
});
const clientDebugUnsafe = new CosmosClient({
endpoint: "https://localhost",
diagnosticLevel: CosmosDbDiagnosticLevel.debugUnsafe,
});

expect((clientInfo as any).clientContext.diagnosticLevel).to.be.eql(
CosmosDbDiagnosticLevel.info
);
expect((clientDebug as any).clientContext.diagnosticLevel).to.be.eql(
CosmosDbDiagnosticLevel.debug
);
expect((clientDebugUnsafe as any).clientContext.diagnosticLevel).to.be.eql(
CosmosDbDiagnosticLevel.debugUnsafe
);
});
});

it("Test Ordering of Diagnostic Level", function () {
Expand Down

0 comments on commit 0237f04

Please sign in to comment.