Integrating cosmos diagnostics with open telemetry tracer by simplynaveen20 · Pull Request #22202 · Azure/azure-sdk-for-java

simplynaveen20 · 2021-06-10T14:24:43Z

Cosmos public API end to end tracing is already added last year with PR #10265

This PR will add diagnostics information on the span event based in some threshold which can be configured via CosmosItemRequestOptions and CosmosQueryRequestOptions through setThresholdForDiagnosticsOnTracerInMS(). Default is 100 ms(CRUD) and 500ms(Query) . Metadata/Script crud request options does not have the option to set threshold and will take default. If needed we can add the option there as well

Note : -

This is draft PR as we have bug in core tracer event api which is getting worked on ,due to which I have to add opentelemetry dependency explicitly in pom, once issue is fixed we will remove the changes from cosmos pom.
This PR also changed the key of query metrics map back to logical id instead of partition range , see changes DefaultDocumentQueryExecutionContext.java and DocumentProducer.java

Some screen shot from Jaeger UI -
Point operation -

Query Operation -

sdk/cosmos/azure-cosmos/pom.xml

...rc/main/java/com/azure/cosmos/implementation/query/DefaultDocumentQueryExecutionContext.java

...osmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/DocumentProducer.java

moderakh

high level looks good. great work Naveen.

my comments:

why are we introducing back pkRangeId? can't we rely on pkRange/feedRange instead?
on the public api we expose period of time as duration elsewhere. we should try to be consistent.
We should avoid adding more BridgeInternal methods if possible.

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

.../azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientSideRequestStatistics.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosItemRequestOptions.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosQueryRequestOptions.java

...rc/main/java/com/azure/cosmos/implementation/query/DefaultDocumentQueryExecutionContext.java

check-enforcer · 2021-06-21T20:26: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

mbhaskar

Thanks for taking time to discuss and add keeping the feedrange in metrics. LGTM

moderakh

Thanks @simplynaveen20 great work. some minor comments/questions, other than that LGTM.

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/util/CosmosPagedFlux.java

moderakh · 2021-06-23T16:20:36Z

...osmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/DocumentProducer.java

                                Arrays.asList(schedulingTimeSpanMap)
                        ), pageResult.getActivityId());
-                BridgeInternal.putQueryMetricsIntoMap(pageResult, feedRange.getRange().toString(), qm);
+                String pkrId = pageResult.getResponseHeaders().get(HttpConstants.HttpHeaders.PARTITION_KEY_RANGE_ID);


why can't we rely on the targetRange please see here:
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/DocumentProducer.java#L93

is that going be different than this one from the header response?

We need both feedrange and rangeId in query metric key , it will be easier to diagnose issue for On call, discussed with @mbhaskar please check this comment #22202 (comment)

moderakh · 2021-06-23T16:21:36Z

...osmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/DocumentProducer.java

-                BridgeInternal.putQueryMetricsIntoMap(pageResult, feedRange.getRange().toString(), qm);
+                String pkrId = pageResult.getResponseHeaders().get(HttpConstants.HttpHeaders.PARTITION_KEY_RANGE_ID);
+                if (StringUtils.isEmpty(pkrId)) {
+                    pkrId = "0";


on which scenario we come here? is this code reachable?

can't we rely on the targetRage see my above comment for this?

We need both feedrange and rangeId in query metric key , it will be easier to diagnose issue for On call, discussed with @mbhaskar please check this comment #22202 (comment)

moderakh · 2021-06-23T16:21:52Z

...rc/main/java/com/azure/cosmos/implementation/query/DefaultDocumentQueryExecutionContext.java

                                                    Arrays.asList(schedulingTimeSpanMap)),
                                            tFeedResponse.getActivityId());
-                            BridgeInternal.putQueryMetricsIntoMap(tFeedResponse, DEFAULT_PARTITION_RANGE, qm);
+                            String pkrId = tFeedResponse.getResponseHeaders().get(HttpConstants.HttpHeaders.PARTITION_KEY_RANGE_ID);


why can't we rely on the targetRange please see here:
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/query/DocumentProducer.java#L93

is that going be different than this one from the header response?

We need both feedrange and rangeId in query metric key , it will be easier to diagnose issue for On call, discussed with @mbhaskar please check this comment #22202 (comment)

...rc/main/java/com/azure/cosmos/implementation/query/DefaultDocumentQueryExecutionContext.java

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

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosItemRequestOptions.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosQueryRequestOptions.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosItemRequestOptions.java

…ter new core release

simplynaveen20 · 2021-07-07T18:38:14Z

/azp run java - cosmos - tests

azure-pipelines · 2021-07-07T18:38:27Z

Azure Pipelines successfully started running 1 pipeline(s).

simplynaveen20 · 2021-07-13T18:59:52Z

/azp run java - cosmos - tests

azure-pipelines · 2021-07-13T19:00:09Z

Azure Pipelines successfully started running 1 pipeline(s).

simplynaveen20 · 2021-07-13T22:50:23Z

/azp run java - cosmos - tests

azure-pipelines · 2021-07-13T22:50:36Z

Azure Pipelines successfully started running 1 pipeline(s).

simplynaveen20 added 6 commits May 27, 2021 18:13

Diagnostic integration with open telemetry

6600746

adding test case for query metrics in tracer

28e2607

incremental change

cda7afd

Adding api to pass threshhold for diagnostics

a6ecac4

Merge branch 'latest-master' into user/nakumars/diagnoticOnTracer

21b2848

Correcting Api name

e14332d

ghost added the Cosmos label Jun 10, 2021

simplynaveen20 changed the title ~~Integrating cosmos diagnostic with open telemetry tracer~~ Integrating cosmos diagnostics with open telemetry tracer Jun 10, 2021

Documentation improvement

f1edb0e

simplynaveen20 commented Jun 10, 2021

View reviewed changes

simplynaveen20 requested review from kushagraThapar, mbhaskar and moderakh and removed request for kushagraThapar June 10, 2021 14:41

simplynaveen20 self-assigned this Jun 10, 2021

simplynaveen20 requested a review from kushagraThapar June 10, 2021 14:41

moderakh reviewed Jun 16, 2021

View reviewed changes

simplynaveen20 mentioned this pull request Jun 17, 2021

Fixing environment name header for client telemetry for daily ctl #22364

Merged

resolving comments

f249610

Adding pkrange and pkrange id both for querymetrics key

180ce6d

mbhaskar approved these changes Jun 21, 2021

View reviewed changes

moderakh approved these changes Jun 23, 2021

View reviewed changes

simplynaveen20 added 3 commits July 6, 2021 11:32

Merging with master and removing opentelemetry dependency from pom af…

3683faf

…ter new core release

resolving comments

c6c055f

Using new core addEvent api with context param

4f34d2a

simplynaveen20 marked this pull request as ready for review July 6, 2021 15:49

simplynaveen20 requested review from FabianMeiswinkel, aayush3011 and kirankumarkolli as code owners July 6, 2021 15:49

simplynaveen20 requested review from milismsft and xinlian12 as code owners July 6, 2021 15:49

simplynaveen20 added 3 commits July 6, 2021 15:04

build error fix

64e675a

resolving comments

df80247

fixing spot bugs

da48392

Merge with master

29e153d

fixing test case

762b134

fixing test for emulator

d3bdf41

simplynaveen20 merged commit 86e6d62 into Azure:main Jul 15, 2021

simplynaveen20 mentioned this pull request Sep 17, 2021

Removing full exception trace from 404 error #24195

Merged

Comments

Conversation

simplynaveen20 commented Jun 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moderakh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

check-enforcer bot commented Jun 21, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

mbhaskar left a comment

Choose a reason for hiding this comment

Uh oh!

moderakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moderakh Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

simplynaveen20 Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

moderakh Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

simplynaveen20 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

moderakh Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

simplynaveen20 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simplynaveen20 commented Jul 7, 2021

Uh oh!

azure-pipelines bot commented Jul 7, 2021

Uh oh!

simplynaveen20 commented Jul 13, 2021

Uh oh!

azure-pipelines bot commented Jul 13, 2021

Uh oh!

simplynaveen20 commented Jul 13, 2021

Uh oh!

azure-pipelines bot commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moderakh left a comment •

edited

Loading