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

Exposes OpenTelemetryConcurrency target #592

Merged

Conversation

NeedleInAJayStack
Copy link
Contributor

Thank you @semicoleon for the excellent changes in #546! This MR simply exposes the new OpenTelemetryConcurrency target through the OpenTelemetryApi product so that it is available for usage by downstream packages. This enables usage as shown in /Examples/ConcurrencyContext/main.swift.

If you'd prefer for OpenTelemetryConcurrency to get its own product, just let me know and I'm happy to adjust. Thanks!

Copy link

linux-foundation-easycla bot commented Aug 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@bryce-b bryce-b self-requested a review August 20, 2024 21:52
@NeedleInAJayStack
Copy link
Contributor Author

Hmm, kinda strange that the Linux CI appears to be stuck. I'm able to successfully run these tests on a swift:5.10 docker image locally. Any ideas?

@semicoleon
Copy link
Contributor

It may just be an intermittent failure, I had quite a few when working on the original PR. There were also a couple of issues with tests never finishing on Linux even when they worked fine on other platforms. I fixed those issues that I encountered but it's possible another one has popped up now.

@bryce-b
Copy link
Member

bryce-b commented Aug 22, 2024

Hi @NeedleInAJayStack the concurrency target should be its own product. Adding it to the OpenTelemetryApi product would create a circular dependency.

Copy link
Member

@nachoBonafonte nachoBonafonte left a comment

Choose a reason for hiding this comment

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

Please make it a separate product

@NeedleInAJayStack
Copy link
Contributor Author

Sure, I've adjusted it to be a separate product. Thanks!

@NeedleInAJayStack
Copy link
Contributor Author

@nachoBonafonte Just a gentle reminder on this PR - any thoughts on my changes? Thanks!

Copy link
Member

@nachoBonafonte nachoBonafonte 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 the reminder, looks good to me now.

@nachoBonafonte nachoBonafonte merged commit 0b7ca90 into open-telemetry:main Aug 29, 2024
6 checks passed
nachoBonafonte added a commit to nachoBonafonte/opentelemetry-swift that referenced this pull request Aug 29, 2024
* main:
  Exposes OpenTelemetryConcurrency target (open-telemetry#592)

# Conflicts:
#	Package@swift-5.9.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants