[Core] - Move core-tracing to @opentelemetry/api 0.20.0 #15672

maorleger · 2021-06-10T18:54:12Z

What

Bump @opentelemetry/api to 0.20.0 in @azure/core-tracing
Move all packages that are on core-http to the next core-tracing version
Remove version collision check from the tracer cache

Why

This is part of our effort to move everyone to OTel 0.20.0 - but we have to stage it due to a transitive dependency and a breaking change in OTel. This PR updates core-tracing to use the latest OTel, fixes any breaking changes, and moves packages that we can move to the latest version of core-tracing.

Once core-rest-pipeline 1.1.0 is GA'd we'll be able to move the rest of the packages over to the latest core-tracing as well.

Removing the version collision came out of an offline discussion after these changes were reviewed - since it's a common source of pain and hasn't added much benefit (tracer conflicts incompatibility was never a problem) we decided to remove that logic
and always grab a unique symbol per OT compatibility.

Callouts

The packages that are already on core-v2 will not be upgraded at this time - we'll coordinate that with the GA of core-rest-pipeline 1.1.0

check-enforcer · 2021-06-10T19:04: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 js - [service] - ci

maorleger · 2021-06-10T19:38:51Z

/check-enforcer evaluate

common/config/rush/common-versions.json

sdk/appconfiguration/app-configuration/package.json

sdk/core/core-tracing/review/core-tracing.api.md

sdk/core/core-tracing/src/interfaces.ts

richardpark-msft · 2021-06-11T17:42:37Z

Looks good (and thankfully the source level changes are minimal, unlike last time).

One other thing is to update for the core-tracing/cache.ts.

You probably want to do this sooner, rather than later, so it'll be in place when you run some of the live test pipelines.

CC: @xirzec

maorleger · 2021-06-11T21:45:09Z

@xirzec @richardpark-msft thanks Richard for the reminder!

I made a few changes there to make it easier to catch early:

match the GLOBAL_TRACER_VERSION to @opentelemetry/api version (made it a string, assuming it can be as long as it's unique but let me know if otherwise!).
Inferred the GLOBAL_TRACER_SYMBOL from GLOBAL_TRACER_VERSION just to keep to one place to change.
added a test to ensure we can catch this in the future.

I'm hoping these changes are ok to make, but please let me know if that's not quite right or if you have suggestions

deyaaeldeen

I reviewed the PR and it looks good to me, thanks for upgrading us!

I will defer to @richardpark-msft for approval.

xirzec

I'm glad the actual changes are pretty minor!

This also reminded me to file #15732

sdk/core/core-tracing/src/utils/cache.ts

sdk/core/core-tracing/test/cache.spec.ts

sdk/core/core-tracing/src/utils/cache.ts

xirzec

Left a code change comment, but also just made an interesting discovery...

xirzec · 2021-06-15T00:50:21Z

sdk/core/core-tracing/test/cache.spec.ts

+import * as fs from "fs";
+import * as path from "path";
+
+const validOpenTelemetryVersions = ["1.0.0-rc1", "0.20.0"];


I just noticed that it looks like 1.0 has shipped: https://www.npmjs.com/package/@opentelemetry/api/v/1.0.0

Should we just update to that and GA this package instead? :)

Oh wow! Oh wow! Oh wow!

But 1.0.0 has the next tag... and 0.20.0 has the latest tag

maorleger · 2021-06-18T16:23:11Z

@xirzec I addressed the feedback and implemented the changes we discussed offline (removal of the version collision runtime check and adding a sanity check in test to ensure future us remembers to think about compatibility) - any concerns with the latest changes? If not, should we merge this in?

Especially wanted to call out the cache.ts changes which, while simplifying the code, could use some extra scrutiny 😄

xirzec

Bravo! 👏

xirzec · 2021-06-18T21:06:56Z

sdk/core/core-http/test/policies/tracingPolicyTests.ts

 } from "@azure/core-tracing";
 import { tracingPolicy } from "../../src/policies/tracingPolicy";

-class MockSpan extends NoOpSpan {
+class MockSpan implements Span {


much simpler! Good discovery here

This reverts commit 153e4f7.

ghost added Azure.Core App Configuration Azure.ApplicationModel.Configuration Azure.Identity KeyVault Monitor Monitor, Monitor Ingestion, Monitor Query labels Jun 10, 2021

maorleger force-pushed the core-tracing-0-20-partial branch from b730eb2 to 4724727 Compare June 10, 2021 19:55

maorleger commented Jun 11, 2021

View reviewed changes

common/config/rush/common-versions.json Outdated Show resolved Hide resolved

sdk/appconfiguration/app-configuration/package.json Show resolved Hide resolved

sdk/core/core-tracing/review/core-tracing.api.md Outdated Show resolved Hide resolved

richardpark-msft reviewed Jun 11, 2021

View reviewed changes

sdk/core/core-tracing/src/interfaces.ts Show resolved Hide resolved

maorleger force-pushed the core-tracing-0-20-partial branch 2 times, most recently from ff81d06 to c73087e Compare June 11, 2021 23:23

maorleger changed the title ~~WIP~~ [Core] - Move core-tracing to @opentelemetry/api 0.20.0 Jun 11, 2021

maorleger marked this pull request as ready for review June 11, 2021 23:53

maorleger requested review from 0rland0Wats0n, ankitarorabit, bterlson, chradek, deyaaeldeen, DominikMe, EmmaZhu, HarshaNalluru, jeremymeng, jiacfan, joheredi, KarishmaGhiya, ljian3377 and mikeharder as code owners June 11, 2021 23:53

maorleger requested review from XiaoningLiu, xirzec and a team as code owners June 11, 2021 23:53

deyaaeldeen reviewed Jun 14, 2021

View reviewed changes

xirzec requested changes Jun 14, 2021

View reviewed changes

sdk/core/core-tracing/src/utils/cache.ts Outdated Show resolved Hide resolved

sdk/core/core-tracing/test/cache.spec.ts Outdated Show resolved Hide resolved

This was referenced Jun 14, 2021

Circular dependency open-telemetry/opentelemetry-js-api#87

Closed

Circular dependency #15584

Closed

xirzec reviewed Jun 15, 2021

View reviewed changes

sdk/core/core-tracing/src/utils/cache.ts Show resolved Hide resolved

xirzec reviewed Jun 15, 2021

View reviewed changes

maorleger force-pushed the core-tracing-0-20-partial branch from 1bda8cd to 27f588c Compare June 15, 2021 15:33

EmmaZhu approved these changes Jun 16, 2021

View reviewed changes

maorleger force-pushed the core-tracing-0-20-partial branch from 27f588c to 7bdad9b Compare June 17, 2021 16:14

maorleger requested a review from xirzec June 17, 2021 16:22

maorleger force-pushed the core-tracing-0-20-partial branch 3 times, most recently from 0389954 to 6a29b3a Compare June 17, 2021 23:32

xirzec approved these changes Jun 18, 2021

View reviewed changes

maorleger added 8 commits June 18, 2021 14:19

Update opentelemetry/api for 0.20.0

7feab19

pnpm and review file post rebase

f974108

1.0.0-rc1 is incompatible

04f9a4a

test removing target

3475f4a

Revert "test removing target"

6ac259b

This reverts commit 153e4f7.

implement Span directly in test

eb8e3a0

skip test if in node

e1954a8

update pnpm-lock post rebase

885d670

maorleger force-pushed the core-tracing-0-20-partial branch from d032f34 to 885d670 Compare June 18, 2021 21:21

maorleger merged commit d023204 into Azure:main Jun 18, 2021

maorleger deleted the core-tracing-0-20-partial branch June 18, 2021 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] - Move core-tracing to @opentelemetry/api 0.20.0 #15672

[Core] - Move core-tracing to @opentelemetry/api 0.20.0 #15672

maorleger commented Jun 10, 2021 •

edited

Loading

check-enforcer bot commented Jun 10, 2021

maorleger commented Jun 10, 2021

richardpark-msft commented Jun 11, 2021 •

edited

Loading

maorleger commented Jun 11, 2021 •

edited

Loading

deyaaeldeen left a comment

xirzec left a comment

xirzec left a comment

xirzec Jun 15, 2021

ramya-rao-a Jun 15, 2021

ramya-rao-a Jun 15, 2021

maorleger commented Jun 18, 2021

xirzec left a comment

xirzec Jun 18, 2021

[Core] - Move core-tracing to @opentelemetry/api 0.20.0 #15672

[Core] - Move core-tracing to @opentelemetry/api 0.20.0 #15672

Conversation

maorleger commented Jun 10, 2021 • edited Loading

What

Why

Callouts

check-enforcer bot commented Jun 10, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

maorleger commented Jun 10, 2021

richardpark-msft commented Jun 11, 2021 • edited Loading

maorleger commented Jun 11, 2021 • edited Loading

deyaaeldeen left a comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

xirzec Jun 15, 2021

Choose a reason for hiding this comment

ramya-rao-a Jun 15, 2021

Choose a reason for hiding this comment

ramya-rao-a Jun 15, 2021

Choose a reason for hiding this comment

maorleger commented Jun 18, 2021

xirzec left a comment

Choose a reason for hiding this comment

xirzec Jun 18, 2021

Choose a reason for hiding this comment

maorleger commented Jun 10, 2021 •

edited

Loading

richardpark-msft commented Jun 11, 2021 •

edited

Loading

maorleger commented Jun 11, 2021 •

edited

Loading