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

Ensure there are no jdbc spans if otel sdk is disabled #45359

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

brunobat
Copy link
Contributor

@brunobat brunobat commented Jan 3, 2025

During the OTel performance work I noticed that the OTel jdbc wrapper was being run even if the OTel SDK was disabled at runtime.
The PR fixes the unnecessary execution of the span creation, therefore improving performance.

Also found that if the OTel extension was disabled at build time, quarkus would fail to start if the wrapper was present.
Could not create a test because NoNettyDataSourceConfigTest doesn't like the OTel extension.

@brunobat brunobat requested review from michalvavrik and gsmet and removed request for michalvavrik January 3, 2025 16:12
@brunobat brunobat changed the title No jdbc spans if otel sdk is disabled Ensure there are no jdbc spans if otel sdk is disabled Jan 3, 2025

This comment has been minimized.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I think it looks good, thanks for fixing it @brunobat . I will leave @gsmet to decide if using quarkus.otel.sdk.disabled inside Agroal doesn't violate separation of concerns, I remember Guillaume helped with original impl.

@brunobat
Copy link
Contributor Author

brunobat commented Jan 6, 2025

Will have to submit another commit on this PR.
Turns out Quarkus fails to start when quarkus.otel.enabled=false

@brunobat
Copy link
Contributor Author

brunobat commented Jan 6, 2025

Updated the code and the PR description.

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added two comments.

Would like to get @yrodiere 's opinion too.

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

I have doubts about how config is retrieved, but if tests pass... LGTM.

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jan 21, 2025
@brunobat brunobat requested a review from yrodiere January 21, 2025 16:35

This comment has been minimized.

This comment has been minimized.

Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit a978058.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/observability-lgtm

io.quarkus.observability.test.LgtmReloadTest.testReload - History

  • io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.observability.deployment.ObservabilityDevServiceProcessor\#startContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/grafana/otel-lgtm:0.8.2 at io.quarkus.observability.deployment.ObservabilityDevServiceProcessor.lambda$startContainers$3(ObservabilityDevServiceProcessor.java:170) at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762) - java.lang.RuntimeException
java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.observability.deployment.ObservabilityDevServiceProcessor#startContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/grafana/otel-lgtm:0.8.2
	at io.quarkus.observability.deployment.ObservabilityDevServiceProcessor.lambda$startContainers$3(ObservabilityDevServiceProcessor.java:170)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
	at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:762)
	at io.quarkus.observability.deployment.ObservabilityDevServiceProcessor.startContainers(ObservabilityDevServiceProcessor.java:113)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)

@gsmet gsmet merged commit e38b5d9 into quarkusio:main Jan 23, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants