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

Fix OpenTelemetry substitution #25158

Merged
merged 1 commit into from
Apr 26, 2022
Merged

Fix OpenTelemetry substitution #25158

merged 1 commit into from
Apr 26, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 26, 2022

Fixes: #25143

@geoand geoand requested a review from radcortez April 26, 2022 06:35
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 26, 2022

/cc @radcortez

@gsmet
Copy link
Member

gsmet commented Apr 26, 2022

Any chance we could test this somehow? Not necessarily the whole thing but at least that this thing compiles?

@geoand
Copy link
Contributor Author

geoand commented Apr 26, 2022

We could add the extension to one of the integration tests. @radcortez anything more we can do?

@radcortez
Copy link
Member

We could add the extension to one of the integration tests. @radcortez anything more we can do?

We can also add some integrations tests, set up Jaeger, and check if the Telemetry data is getting correctly populated. It is going to require some effort. Not sure if it will be worth it since the plan is to move the Jaeger extension to Quarkiverse and deprecate it at some point. The recommended way to push Telemetry data is to use the OTel Exporter (which can push data to Jaeger on its own, so we don't need a dedicated extension for Jaeger).

@geoand
Copy link
Contributor Author

geoand commented Apr 26, 2022

So do you propose we just leave it as is for now?

@radcortez
Copy link
Member

So do you propose we just leave it as is for now?

Yes. We can just add an integration-test to make sure that the compile works.

@geoand
Copy link
Contributor Author

geoand commented Apr 26, 2022

Done

Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

App with quarkus-opentelemetry-exporter-jaeger builds now

@rsvoboda rsvoboda merged commit 316e5a0 into quarkusio:main Apr 26, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 26, 2022
@geoand
Copy link
Contributor Author

geoand commented Apr 26, 2022

Thanks for checking!

@geoand geoand deleted the #25143 branch April 26, 2022 12:22
@gsmet gsmet modified the milestones: 2.9.0.CR1, 2.8.3.Final May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App with quarkus-opentelemetry-exporter-jaeger fails to compile into native
4 participants