Make copying of Vertx context data conditional #22766

geoand · 2022-01-10T13:13:00Z

In order to avoid negatively affecting RESTEasy Reactive performance
just to serve very specific use cases, we introduce a way to
conditionally control when the current Vertx request context data needs
to be copied into the connection context

Follow up of: #22671

quarkus-bot · 2022-01-10T13:13:19Z

/cc @radcortez

quarkus-bot · 2022-01-10T13:30:16Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `71a7f81`

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/opentelemetry/opentelemetry/deployment 
! Skipped: docs extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment and 4 more

📦 extensions/opentelemetry/opentelemetry/deployment

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.6.2:check (check-imports) on project quarkus-opentelemetry-deployment: Imports are not sorted in /home/runner/work/quarkus/quarkus/extensions/opentelemetry/opentelemetry/deployment/src/main/java/io/quarkus/opentelemetry/deployment/OpenTelemetryProcessor.java

...ent/src/main/java/io/quarkus/resteasy/reactive/server/spi/CopyVertxContextDataBuildItem.java

gsmet · 2022-01-10T14:21:02Z

Note that this shouldn't be seen as a fix for the potential regression performance and we still need to check it. Because, if we end up doing this a lot, we will have users with an app that is working fine and that has degraded performance when adding telemetry.

geoand · 2022-01-10T14:23:47Z

Still need to check what? I am not following

gsmet · 2022-01-10T14:27:03Z

The penalty of what @radcortez introduced and if it should be done this way. I don't want us to have a very optimized thing that falls apart when you add telemetry :).

BTW, I'm not against this PR, just that it doesn't prevent us from making sure the penalty is not too big.

geoand · 2022-01-10T14:31:50Z

Sure, checking further should be a next step, but it's entirely possible that we won't be able to avoid this type of copying for the case of telemetry.

geoand · 2022-01-10T14:34:24Z

Also, it's a given that any sort of telemetry will add some overhead.

What we don't want is to pay that price if no telemetry is needed

radcortez · 2022-01-10T14:40:22Z

We can probably make it even smarter, by only copying the context if the trace is sampled. Anyway, it would be interesting to measure the performance hit, not only for this case but for telemetry overall.

Let me add that to my TODO list :)

Sanne · 2022-01-10T15:10:26Z

It concerns me that such a "condition" might be yet-another source of things that breaks or magically get fixed when an extension is added [removed]. We need to be careful in making things nicely composeable for Quarkus to be a viable platform for non-trivial applications; maybe best to explore such options only if we see there is a performance problem?

Also wondering: does telemetry need the whole map or is it needing some specific element? If we could "copy" only what it needs, at least it wouldn't have unexpected side-effects. In which case, perhaps what we need is a similar BuildItem, but which allows registering what exactly needs to be copied.

quarkus-bot · 2022-01-10T15:49:52Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `d5bc289`

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs
✖	MicroProfile TCKs Tests	`Verify`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.CompositeBuildWithDependenciesDevModeTest.main line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/opentelemetry-vertx

📦 integration-tests/opentelemetry-vertx

✖ io.quarkus.it.opentelemetry.vertx.HelloRouterTest.span - More details - Source on GitHub

java.lang.RuntimeException: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:593)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:666)

⚙️ JVM Tests - JDK 11 Windows #

- Failing: integration-tests/opentelemetry-vertx

📦 integration-tests/opentelemetry-vertx

✖ io.quarkus.it.opentelemetry.vertx.HelloRouterTest.span - More details - Source on GitHub

java.lang.RuntimeException: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:593)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:666)

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/opentelemetry-vertx

📦 integration-tests/opentelemetry-vertx

✖ io.quarkus.it.opentelemetry.vertx.HelloRouterTest.span - More details - Source on GitHub

java.lang.RuntimeException: java.util.ServiceConfigurationError: io.smallrye.config.SmallRyeConfigFactory: io.quarkus.runtime.configuration.QuarkusConfigFactory not a subtype
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:593)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:666)

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-fault-tolerance

📦 tcks/microprofile-fault-tolerance

✖ org.eclipse.microprofile.fault.tolerance.tck.TimeoutUninterruptableTest.testTimeoutAsyncBulkhead line 190 - More details - Source on GitHub

java.lang.AssertionError: Unexpected exception thrown from Future
	at org.testng.Assert.fail(Assert.java:85)
	at org.eclipse.microprofile.fault.tolerance.tck.util.Exceptions.expect(Exceptions.java:98)

radcortez · 2022-01-10T16:03:26Z

Also wondering: does telemetry need the whole map or is it needing some specific element? If we could "copy" only what it needs, at least it wouldn't have unexpected side-effects. In which case, perhaps what we need is a similar BuildItem, but which allows registering what exactly needs to be copied.

Yes, the entire map is not required. We can just copy the specific element that carries the OTel context. When I wrote the fix, I thought about it, but I didn't want to be specific about OTel, so a build item to copy specific things sounds better.

geoand · 2022-01-10T17:32:17Z

I updated the PR to use the named approach

quarkus-bot · 2022-01-10T17:53:42Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `71eebdd`

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/opentelemetry/opentelemetry/deployment 
! Skipped: docs extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment and 4 more

📦 extensions/opentelemetry/opentelemetry/deployment

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.6.2:check (check-imports) on project quarkus-opentelemetry-deployment: Imports are not sorted in /home/runner/work/quarkus/quarkus/extensions/opentelemetry/opentelemetry/deployment/src/main/java/io/quarkus/opentelemetry/deployment/OpenTelemetryProcessor.java

In order to avoid negatively affecting RESTEasy Reactive performance just to serve very specific use cases, we introduce a way to conditionally control when the current Vertx request context data needs to be copied into the connection context Follow up off: quarkusio#22671

geoand requested a review from radcortez January 10, 2022 13:13

geoand mentioned this pull request Jan 10, 2022

Copy duplicated local context to the event loop context #22671

Merged

quarkus-bot bot added area/rest area/tracing labels Jan 10, 2022

geoand force-pushed the #22671-follow-up branch from 71a7f81 to d5bc289 Compare January 10, 2022 13:36

radcortez reviewed Jan 10, 2022

View reviewed changes

...ent/src/main/java/io/quarkus/resteasy/reactive/server/spi/CopyVertxContextDataBuildItem.java Outdated Show resolved Hide resolved

geoand force-pushed the #22671-follow-up branch from d5bc289 to 0189dc3 Compare January 10, 2022 17:32

quarkus-bot bot added the area/vertx label Jan 10, 2022

geoand force-pushed the #22671-follow-up branch 2 times, most recently from 3c06f34 to 71eebdd Compare January 10, 2022 17:37

geoand force-pushed the #22671-follow-up branch from 71eebdd to 8735ad1 Compare January 10, 2022 17:59

geoand requested a review from radcortez January 11, 2022 14:17

radcortez approved these changes Jan 11, 2022

View reviewed changes

geoand merged commit bf06169 into quarkusio:main Jan 11, 2022

geoand deleted the #22671-follow-up branch January 11, 2022 17:37

quarkus-bot bot added this to the 2.7 - main milestone Jan 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make copying of Vertx context data conditional #22766

Make copying of Vertx context data conditional #22766

geoand commented Jan 10, 2022 •

edited

Loading

quarkus-bot bot commented Jan 10, 2022

quarkus-bot bot commented Jan 10, 2022 •

edited

Loading

Failing Jobs - Building `71a7f81`

Failures

⚙️ Initial JDK 11 Build #

📦 extensions/opentelemetry/opentelemetry/deployment

gsmet commented Jan 10, 2022

geoand commented Jan 10, 2022

gsmet commented Jan 10, 2022

geoand commented Jan 10, 2022 •

edited

Loading

geoand commented Jan 10, 2022

radcortez commented Jan 10, 2022

Sanne commented Jan 10, 2022

quarkus-bot bot commented Jan 10, 2022 •

edited

Loading

Failing Jobs - Building `d5bc289`

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/opentelemetry-vertx

⚙️ JVM Tests - JDK 11 Windows #

📦 integration-tests/opentelemetry-vertx

⚙️ JVM Tests - JDK 17 #

📦 integration-tests/opentelemetry-vertx

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-fault-tolerance

radcortez commented Jan 10, 2022 •

edited

Loading

geoand commented Jan 10, 2022

quarkus-bot bot commented Jan 10, 2022 •

edited

Loading

Failing Jobs - Building `71eebdd`

Failures

⚙️ Initial JDK 11 Build #

📦 extensions/opentelemetry/opentelemetry/deployment

Make copying of Vertx context data conditional #22766

Make copying of Vertx context data conditional #22766

Conversation

geoand commented Jan 10, 2022 • edited Loading

quarkus-bot bot commented Jan 10, 2022

quarkus-bot bot commented Jan 10, 2022 • edited Loading

Failing Jobs - Building 71a7f81

Failures

⚙️ Initial JDK 11 Build #

📦 extensions/opentelemetry/opentelemetry/deployment

gsmet commented Jan 10, 2022

geoand commented Jan 10, 2022

gsmet commented Jan 10, 2022

geoand commented Jan 10, 2022 • edited Loading

geoand commented Jan 10, 2022

radcortez commented Jan 10, 2022

Sanne commented Jan 10, 2022

quarkus-bot bot commented Jan 10, 2022 • edited Loading

Failing Jobs - Building d5bc289

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/opentelemetry-vertx

⚙️ JVM Tests - JDK 11 Windows #

📦 integration-tests/opentelemetry-vertx

⚙️ JVM Tests - JDK 17 #

📦 integration-tests/opentelemetry-vertx

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-fault-tolerance

radcortez commented Jan 10, 2022 • edited Loading

geoand commented Jan 10, 2022

quarkus-bot bot commented Jan 10, 2022 • edited Loading

Failing Jobs - Building 71eebdd

Failures

⚙️ Initial JDK 11 Build #

📦 extensions/opentelemetry/opentelemetry/deployment

geoand commented Jan 10, 2022 •

edited

Loading

quarkus-bot bot commented Jan 10, 2022 •

edited

Loading

Failing Jobs - Building `71a7f81`

geoand commented Jan 10, 2022 •

edited

Loading

quarkus-bot bot commented Jan 10, 2022 •

edited

Loading

Failing Jobs - Building `d5bc289`

radcortez commented Jan 10, 2022 •

edited

Loading

quarkus-bot bot commented Jan 10, 2022 •

edited

Loading

Failing Jobs - Building `71eebdd`