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.

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

Update exporter to OpenTelemetry api/sdk v1.15.0 #27913

Merged
merged 1 commit into from
Dec 14, 2022
Merged

Conversation

jabbera
Copy link
Contributor

@jabbera jabbera commented Dec 12, 2022

Description

Opentelemetry 1.15.0 made breaking changes. See: #27900 and open-telemetry/opentelemetry-python#3038

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Dec 12, 2022
@ghost
Copy link

ghost commented Dec 12, 2022

Thank you for your contribution jabbera! We will review the pull request and get back to you soon.

@jabbera
Copy link
Contributor Author

jabbera commented Dec 12, 2022

I didn't test that this still actually works, but at least it imports. This is really annoying to have broken. I get that this is beta but I'm hoping for a quick turnaround on this.

@jabbera jabbera changed the title Updates for opentelemetry==1.0.15 Update to OpenTelemetry api/sdk v1.15.0 Dec 13, 2022
@jabbera
Copy link
Contributor Author

jabbera commented Dec 13, 2022

Builds all pass! Followed the same format as this PR: #25659

@jabbera jabbera changed the title Update to OpenTelemetry api/sdk v1.15.0 Update exporter to OpenTelemetry api/sdk v1.15.0 Dec 13, 2022
Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

@lzchen or @jeremydvoss: can one of you take a look? Monitor pipelines failing due to the severity import path change.

@kaihei777
Copy link

me too...

@jabbera
Copy link
Contributor Author

jabbera commented Dec 13, 2022

@pvaneck Anything we can do to move this along? I don't think I can explain the pain this is causing our CI environment:-)

@pvaneck
Copy link
Member

pvaneck commented Dec 13, 2022

Looks good to me. Not sure why the CI checks aren't reporting back. Mind doing a rebase or push another commit to retry the checks?

@jeremydvoss
Copy link
Member

Hey @jabbera, could you update the PR detaisl to say 1.15.0 instead of 1.0.15? Otherwise, looks good. Approved.

@jabbera
Copy link
Contributor Author

jabbera commented Dec 13, 2022

@jeremydvoss rebased and comment adjusted,

@pvaneck pvaneck merged commit aaf6bbb into Azure:main Dec 14, 2022
@pvaneck
Copy link
Member

pvaneck commented Dec 14, 2022

Thanks @jabbera. Most likely a new patch release will be cut tomorrow after some additional verification by @jeremydvoss.

@jabbera jabbera deleted the fix branch December 14, 2022 14:57
@jeremydvoss
Copy link
Member

Hey @jabbera. I am looking into whether we can do a release before January. It's a bit difficult since many people are on vacation. It may be possible today, but I can't make promises yet. I will keep you posted. In the meantime, can you use OTel 1.14?

@jabbera
Copy link
Contributor Author

jabbera commented Dec 14, 2022

@jeremydvoss Not easily. We depend on this package from about 50 pipelines. Each would have to be updated and the pinned dependency list is ugly because we use a bunch of otel contrib packages that don't have pinned dependencies

@jeremydvoss
Copy link
Member

There's another issue: when using the fixed version of the exporter with OTel 1.15, I consistently get the following warning:
...\site-packages\werkzeug\serving.py:716: ResourceWarning: unclosed <socket.socket fd=1304, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0> self.socket = socket.fromfd(fd, address_family, socket.SOCK_STREAM) ResourceWarning: Enable tracemalloc to get the object allocation traceback

I've determined that no commit in exporter 1.0.0b11 has caused this. It's too early to know. But it seems to be an issue with how opentelemetry-instrumentation-flask==0.36b0 or opentelemetry-instrumentation-wsgi==0.36b0 use Werkzeug. I've confirmed the fixed version of the exporter does still send telemetry correctly. However, since the new version needs to be pinned to 1.15, we'll be encouraging people to use 1.15 seems to have some issue that needs to be addressed.

@jeremydvoss
Copy link
Member

I'll see what I can do.

@jeremydvoss
Copy link
Member

I am not able to release the exporter as is because of the memory issue with OTel 1.15. Instead, we can pin the exporter to 1.12<=x<=1.14 before the module path was changed. That way, we can avoid the memory allocation issue as well as the severity import breaking change.

jeremydvoss added a commit to jeremydvoss/azure-sdk-for-python that referenced this pull request Dec 15, 2022
@jeremydvoss
Copy link
Member

pvaneck pushed a commit that referenced this pull request Dec 16, 2022
…nstead (#27958)

* Release hotfix 1.0.0b11. Reverting #27913. Pinning to < OTel 1.15

* shared_requirements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants