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

[tracing] updating to opentelemetry 1.0.0-rc.0 #14208

Merged
merged 37 commits into from
Mar 29, 2021
Merged

[tracing] updating to opentelemetry 1.0.0-rc.0 #14208

merged 37 commits into from
Mar 29, 2021

Conversation

richardpark-msft
Copy link
Member

@richardpark-msft richardpark-msft commented Mar 9, 2021 •
edited
Loading

This PR does the final leg of updating our code base to opentelemetry 1.0.0-rc.0, which has been done in pieces with #14094, #14082, #13951, #13949 and #13942.

This PR looks deceptively big but in reality it's just a few similar changes, but applied across a lot of projects. The three areas that you'll care about:

  1. Your rollup.config file now has a new import from dev-tool that will take care of formulating the proper opentelemetry/api rollup configuration. This is setup for everyone at this point, so if future things come up this should be easier (hopefully).
  2. NOBODY references opentelemetry/api directly! The work we did in the previous PRs meant that all packages can just depend on core-tracing and get opentelemetry transitively. This should greatly simplify things currently and in the future!
  3. azure/identity also got updated in all packages devDependencies so we don't have to deal with mixed versions of OTel in every package.
  4. Code changes largely fell into three buckets:
    • CanonicalCode -> SpanStatusCode changes (mostly squashing CanonicalCode into SpanStatusCode.ERROR)
    • Test code that wasn't using createSpan and thus needed slight updates to create and pass a context.

(unresolved, future PR) SpanStatusCode.OK no longer lines up with the 'default' value so spans that do not have span.setSpanStatus(SpanStatusCode.OK) set on them will default to UNSET instead. We should be able to fix this centrally but that is not in this PR.

@check-enforcer
Copy link

check-enforcer bot commented Mar 9, 2021

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

@richardpark-msft richardpark-msft changed the title DRAFT telemetry: updating to 0.18. [tracing] updating to opentelemetry 0.18 Mar 11, 2021
@richardpark-msft richardpark-msft marked this pull request as ready for review March 11, 2021 05:21
@richardpark-msft
Copy link
Member Author

/azp run js - communication-sms - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - storage-file-datalake - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - storage-file-share - tests

@richardpark-msft
Copy link
Member Author

/azp run js - storage-queue - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

2 similar comments
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - attestation - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - container-registry - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

NOTE - the failures that have occurred so far (metricsadvisor and mixedreality) appear to be related to some DNS issues we're having in our agents. The tests have completed, it's the Analyze phase that's failing.

@richardpark-msft
Copy link
Member Author

/azp run js - metricsadvisor - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

/azp run js - mixedreality - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@richardpark-msft
Copy link
Member Author

There are issues with the container-registry pipeline that are unrelated to anything in this PR. Ignoring those failures for now (@jeremymeng is working on it independently of this).

@richardpark-msft
Copy link
Member Author

Okay, to sum up:

  • container-registry failures are being worked on @jeremymeng, independently of this PR.
  • js - storage-file-datalake - tests failed on a single test on Ubuntu+node15, but passed on all others. Unfortunately the test swallows the actual text of the exception but I believe @mikeharder was investigating boundary conditions like this where node 15 increased a limit that can cause this test to no longer fail in the expected way.
  • Other failures are transitive and all appear to be affected by DNS/transitive network issues, and not the contents of this PR.

I'm going to bypass checkenforcer (since it's stuck now on the containre-registry failures) and merge this in. CC: @ramya-rao-a

@richardpark-msft
Copy link
Member Author

/check-enforcer override

@richardpark-msft richardpark-msft merged commit f1054c3 into Azure:master Mar 29, 2021
@richardpark-msft richardpark-msft mentioned this pull request Mar 29, 2021
Closed
@richardpark-msft richardpark-msft deleted the ot-upgrading-0.18 branch March 29, 2021 23:29
@chuanqisun chuanqisun mentioned this pull request Apr 3, 2021
Closed
1 task
@joheredi joheredi mentioned this pull request Apr 5, 2021
Merged
jay-most pushed a commit to jay-most/azure-sdk-for-js that referenced this pull request Apr 26, 2021
@richardpark-msft @vindicatesociety
[tracing] updating to opentelemetry 1.0.0-rc.0 (Azure#14208)
f58b5e4 
* Upgrading to opentelemetry 1.0.0 (rc.0)

Did a few things that made this MUCH easier.

Now that everyone is using the createSpan from @azure/core-tracing we
no longer need _every_ project to reference opentelemetry/api! That has
been removed as part of this PR.

Unfortunately, the leaky nature of JS means that packages still need to
worry about opentelemetry when they build their browser bundle for
testing. To make that simpler I've added a common function to dev-tool
that everyone can call in their rollup that will give them the correct
named exports. This is hooked up for everyone at this point, so the next
time something like this happens I should be able to control it
centrally.

Now for the API breaking changes that I had to fix:
- CanonicalCode is gone and is replaced with SpanStatusCode.
  SpanStatusCode has a much smaller set of codes (literally: ERROR, OK
  or UNSET) so that simplified most of the way we were handling setting
  a span status on error.
- There is a new field (`tracingContext`) that contains `Context`. You
  now pass a context, not a span, to indicate what your "parent" is.
  You'll see this where I've removed `SpanOptions.parentSpan`. Mostly
  it's a simple replacement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chradek chradek chradek left review comments

@jeremymeng jeremymeng jeremymeng left review comments

@deyaaeldeen deyaaeldeen deyaaeldeen approved these changes

@maorleger maorleger maorleger approved these changes

@xirzec xirzec xirzec left review comments

@joheredi joheredi joheredi approved these changes

@mikeharder mikeharder mikeharder approved these changes

@EmmaZhu EmmaZhu EmmaZhu approved these changes

@bterlson bterlson Awaiting requested review from bterlson

@daviwil daviwil Awaiting requested review from daviwil

@ellismg ellismg Awaiting requested review from ellismg

@HarshaNalluru HarshaNalluru Awaiting requested review from HarshaNalluru

@jiacfan jiacfan Awaiting requested review from jiacfan

@KarishmaGhiya KarishmaGhiya Awaiting requested review from KarishmaGhiya

@ljian3377 ljian3377 Awaiting requested review from ljian3377

@praveenkuttappan praveenkuttappan Awaiting requested review from praveenkuttappan

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

@sadasant sadasant Awaiting requested review from sadasant

@sarangan12 sarangan12 Awaiting requested review from sarangan12

@schaabs schaabs Awaiting requested review from schaabs

@southpolesteve southpolesteve Awaiting requested review from southpolesteve

@vinjiang vinjiang Awaiting requested review from vinjiang

@witemple-msft witemple-msft Awaiting requested review from witemple-msft

@XiaoningLiu XiaoningLiu Awaiting requested review from XiaoningLiu

@zfoster zfoster Awaiting requested review from zfoster

@vishnureddy17 vishnureddy17 Awaiting requested review from vishnureddy17

Assignees
No one assigned
Labels
App Configuration Azure.ApplicationModel.Configuration Azure.Core Azure.Identity Cognitive - Form Recognizer Cognitive - Metrics Advisor Cognitive - Text Analytics Communication Digital Twins EngSys This issue is impacting the engineering system. Event Grid Event Hubs KeyVault Search Service Bus Storage Storage Service (Queues, Blobs, Files) Synapse Tables
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.