-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Cosmos: Diagnostic improvements #33209
Cosmos: Diagnostic improvements #33209
Conversation
Initial DRAFT for public API review
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosDiagnosticsLogger.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosDiagnosticsLogger.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosDiagnosticsLogger.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosDiagnosticsLoggerConfig.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosDiagnosticsLoggerConfig.java
Outdated
Show resolved
Hide resolved
…to users/fabianm/DiagnosticsProcessor
API change check APIView has identified API level changes in this PR and created following API reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @FabianMeiswinkel , all changes look good to me except for few minor questions and one revert ask on getDuration() API on CosmosResponse. Thanks for the awesome work.
sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/ClientMetricsTest.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncClient.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncClient.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncContainer.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosAsyncContainer.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/DiagnosticsProvider.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/DiagnosticsProvider.java
Outdated
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/HttpConstants.java
Show resolved
Hide resolved
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosClientTelemetryConfig.java
Show resolved
Hide resolved
@@ -127,6 +156,10 @@ public CosmosDiagnostics getDiagnostics() { | |||
* @return end-to-end request latency for the current request to Azure Cosmos DB service. | |||
*/ | |||
public Duration getDuration() { | |||
if (resourceResponseWrapper == null) { | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. Currently if the response is null, we return Duration.ZERO. Can we please revert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss on Monday - I wont' change it right now because I don't agree that it is a breaking change - it would have thrown a NullPointerException here - I only added this to avoid the NullPointerException. If you still think this is considered breaking I can add an internal property - but seems ok to do that in a separate PR or within the retriable write PR after we discussed on Monday.
/azp run java - cosmos - tests |
/azp run java - cosmos - spark |
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - cosmos - spark |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - cosmos - tests |
/azp run java - cosmos - spark |
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - cosmos - spark |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run java - cosmos - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
Test failure readFeedDocumentsAfterSplit has known flakiness - will override and merge the PR |
/check-enforcer override |
Description
We are planning to make a few improvements to the diagnostics capabilities in the Cosmos DB SDK in this PR
CosmosDiagnosticsHandler
that customers can use to build their own diagnostics handlersCosmosDiagnosticsContext
to represent metdata of an SDK operation that is relevant for diagnostics handlers - it could span multipleCosmosDiagnostics
instances - for example in the case of cross-regional retries or speculative processingCosmosDiagnosticsHandler
interface that will emit logs to log4j when operations hit certain conditions (certain status codes, latency exceeding certain threshold) - the configuration of the conditions can be done viaCosmosDiagnosticsThresholds
. This loggerCosmosDiagnosticsLogger
is exposed viaCosmosDiagnosticsHandler.DEFAULT_LOGGING_HANDLER
CosmosDiagnosticsHandler
interface.Code snippets showing the usage of the new API
Enabling default logging to log4j
Enabling logging to log4j customizing conditions when to log
Enabling a custom diagnostics handler
Enabling a custom diagnostics handler (in this case custom logger)
Capturing CosmosDiagnosticsContext from CosmosDiagnostics (for example to get the CosmosDiagnosticsContext from one of the FeedResponse instances all mapping to the same query operation/CosmosDiagnosticsContext instance
Other design considerations
Sample OpenTelemetry traces (captured from custom Tracer serializing in json)
Sample meta-data operation (read container in this case)
Sample point operation (create item in this case) with request tracing enabled
Sample query with request traces - and also violating latency threshold (emitting diagnostics)
Sample point read with request traces - and also having retries for 429
Perf regression tests
Summary
Results of baseline (main branch)
Results of candidate (this PR)
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines
Parent feature: #33425