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

Common URL Template path handling for REST requests (server and client) #17676

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Common URL Template path handling for REST requests (server and client) #17676

merged 1 commit into from
Jun 16, 2021

Conversation

ebullient
Copy link
Member

@ebullient ebullient commented Jun 3, 2021

Fixes #16952
Fixes #15231
Fixes #15044

Provide a common way to remember the parameterized path of a REST endpoint.

Both Micrometer and OpenTelemetry use parameterized values to reduce the cardinality of telemetry data, e.g. using /hello/{name} instead of /hello/gary.

RESTEasy classic and RESTEasy Reactive manage creation/registration of templated paths very differently and two extensions need to be able to consume the parameterized version of the path.

  • Resteasy classic server requests: An additional interceptor annotation is added that stores the partly-resolved templated value from the original @Path annotation (taking a configured Rest ApplicationPath value into account). An associated interceptor binding looks up that annotation to retrieve the templated path, and set it on the common vertx context where consumers (both Micrometer and OpenTelemetry) can find it.

  • Resteasy classic client requests:

@ebullient ebullient changed the title Add resteasy-observability module Draft: Add resteasy-observability module Jun 3, 2021
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/metrics area/resteasy-reactive labels Jun 3, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 3, 2021

/cc @kenfinnigan

@gsmet
Copy link
Member

gsmet commented Jun 9, 2021

There's no detail in the description so it's hard to guess what this is for?

Until now, we didn't have specific extension for observability but we enabled it if the ad hoc extension was around and a config property enabled. What is the rationale for creating a specific extension here?

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Jun 9, 2021
@kenfinnigan
Copy link
Member

@gsmet I can add a description, but the idea is we need a way for REST services to capture the parameterized version of a URL path for an endpoint as it's needed for setting on metrics and traces.

Not using the parameterized version results in high cardinality metrics and traces because we get a separate version for each parameter passed to the endpoint

@kenfinnigan kenfinnigan marked this pull request as ready for review June 9, 2021 18:01
kenfinnigan
kenfinnigan previously approved these changes Jun 9, 2021
Copy link
Member

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

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

I can approve, but seems odd given I contributed as well ;-)

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 9, 2021

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building 141a21a

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Persist Maven Repo ⚠️ Check → Logs Raw logs

@ebullient ebullient changed the title Draft: Add resteasy-observability module Add resteasy-observability module Jun 9, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 9, 2021

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building 8b77f0f

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Devtools Tests - JDK 11 Build Test failures Logs Raw logs
Devtools Tests - JDK 11 Windows Build Test failures Logs Raw logs
Gradle Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Gradle Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs
Maven Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs
Native Tests - Amazon Build ⚠️ Check → Logs Raw logs
Native Tests - Cache ⚠️ Check → Logs Raw logs
Native Tests - Data1 Build ⚠️ Check → Logs Raw logs
Native Tests - Data2 Build ⚠️ Check → Logs Raw logs
Native Tests - Data3 Build ⚠️ Check → Logs Raw logs
Native Tests - Data4 Build ⚠️ Check → Logs Raw logs
Native Tests - Data5 Build ⚠️ Check → Logs Raw logs
Native Tests - Data6 Build ⚠️ Check → Logs Raw logs
Native Tests - Data7 Build ⚠️ Check → Logs Raw logs
Native Tests - HTTP ⚠️ Check → Logs Raw logs
Native Tests - Main Build ⚠️ Check → Logs Raw logs
Native Tests - Messaging1 Build ⚠️ Check → Logs Raw logs
Native Tests - Messaging2 Reclaim Disk Space ⚠️ Check → Logs Raw logs
Native Tests - Misc1 ⚠️ Check → Logs Raw logs
Native Tests - Misc2 ⚠️ Check → Logs Raw logs
Native Tests - Misc3 ⚠️ Check → Logs Raw logs
Native Tests - Misc4 ⚠️ Check → Logs Raw logs
Native Tests - Security1 Reclaim Disk Space ⚠️ Check → Logs Raw logs
Native Tests - Security2 ⚠️ Check → Logs Raw logs
Native Tests - Security3 ⚠️ Check → Logs Raw logs
Native Tests - Spring ⚠️ Check → Logs Raw logs
Native Tests - Windows - hibernate-validator ⚠️ Check → Logs Raw logs
Native Tests - gRPC ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Devtools Tests - JDK 11 #

📦 integration-tests/devtools

io.quarkus.devtools.codestarts.quarkus.CustomRESTEasyReactiveCodestartBuildIT.testBuild line 26 - More details - Source on GitHub

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testRunAloneCodestartsJava(String)[2] line 89 - More details - Source on GitHub

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testRunAloneCodestartsScala(String)[2] line 102 - More details - Source on GitHub

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testRunAloneCodestartsKotlin(String)[2] line 96 - More details - Source on GitHub

📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-java-resteasy-reactive-codestart

org.acme.ReactiveGreetingResourceTest.testHelloEndpoint - More details - Source on GitHub

📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-kotlin-resteasy-reactive-codestart

org.acme.ReactiveGreetingResourceTest.testHelloEndpoint - More details - Source on GitHub

📦 integration-tests/devtools/target/quarkus-codestart-test/resteasy-reactive/real-data/java

ilove.quark.us.RESTEasyEndpointTest.testHelloEndpoint - More details - Source on GitHub


⚙️ Devtools Tests - JDK 11 Windows #

📦 integration-tests/devtools

io.quarkus.devtools.codestarts.quarkus.CustomRESTEasyReactiveCodestartBuildIT.testBuild line 26 - More details - Source on GitHub

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testRunAloneCodestartsJava(String)[2] line 89 - More details - Source on GitHub

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testRunAloneCodestartsScala(String)[2] line 102 - More details - Source on GitHub

io.quarkus.devtools.codestarts.quarkus.QuarkusCodestartBuildIT.testRunAloneCodestartsKotlin(String)[2] line 96 - More details - Source on GitHub

📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-java-resteasy-reactive-codestart

org.acme.ReactiveGreetingResourceTest.testHelloEndpoint - More details - Source on GitHub

📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-kotlin-resteasy-reactive-codestart

org.acme.ReactiveGreetingResourceTest.testHelloEndpoint - More details - Source on GitHub

📦 integration-tests/devtools/target/quarkus-codestart-test/resteasy-reactive/real-data/java

ilove.quark.us.RESTEasyEndpointTest.testHelloEndpoint - More details - Source on GitHub

@kenfinnigan kenfinnigan added this to the 2.1 - main milestone Jun 9, 2021
@kenfinnigan kenfinnigan requested a review from geoand June 9, 2021 19:45
@geoand
Copy link
Contributor

geoand commented Jun 10, 2021

I can't really say I fully understand what's going on here, but IIUC you are adding a CDI interceptor everywhere there is a @Path annotation?
Have you benchmarked how much of a slowdown this incurs, if any?
Is the point of doing that is to stuff something in the Vertx context, right? If so, in RESTEasy Reactive there is definitely a way to do that without paying the cost of the the CDI interceptor.

Furthermore, is the new obversability module meant to be a hard dependency? Can't RESTEasy Classic and RESTEasy Reactive extensions just depend on an SPI module for the build items?

@kenfinnigan
Copy link
Member

I can't really say I fully understand what's going on here, but IIUC you are adding a CDI interceptor everywhere there is a @Path annotation?

Yes, but only when Micrometer or OpenTelemetry are also present, otherwise it is not included.

Have you benchmarked how much of a slowdown this incurs, if any?

Not at present, but the interceptor only reads an annotation and sets a value in the Vertx context, so it should be fairly light.

Is the point of doing that is to stuff something in the Vertx context, right? If so, in RESTEasy Reactive there is definitely a way to do that without paying the cost of the the CDI interceptor.

Yes, that's right.

Do you have pointers to examples?

The reason a CDI Interceptor was chosen is we needed something that works for both RESTEasy Classic and RESTEasy Reactive. Maybe having the common annotation transformer is all that needs to be common, and we can have specific ways to retrieve the annotation and add it to the context.

Furthermore, is the new obversability module meant to be a hard dependency? Can't RESTEasy Classic and RESTEasy Reactive extensions just depend on an SPI module for the build items?

Yes, it's meant to be a hard dependency. There originally was an SPI with just the build item, but then it became too complicated as to how to include the actual deployment dependency given the logic of "if (RESTEasy Classic OR RESTEasy Reactive) AND (Micrometer OR OpenTelemetry)". It was decided the best option was to directly include in the REST extensions but have the processor do nothing if Micrometer or OpenTelemetry aren't present.

@geoand
Copy link
Contributor

geoand commented Jun 10, 2021

I can't really say I fully understand what's going on here, but IIUC you are adding a CDI interceptor everywhere there is a @Path annotation?

Yes, but only when Micrometer or OpenTelemetry are also present, otherwise it is not included.

Have you benchmarked how much of a slowdown this incurs, if any?

Not at present, but the interceptor only reads an annotation and sets a value in the Vertx context, so it should be fairly light.

Reading the annotation part is what has me worried.
We have tried very hard in RESTEasy Reactive to avoid reading annotations at runtime, so if this can be avoided, I would much prefer it.

Is the point of doing that is to stuff something in the Vertx context, right? If so, in RESTEasy Reactive there is definitely a way to do that without paying the cost of the the CDI interceptor.

Yes, that's right.

Do you have pointers to examples?

Basically you would need to add a new ServerRestHandler and alter the handler chain with the HandlerChainCustomizer using the BEFORE_METHOD_INVOKE phase.
Any of the usages of HandlerChainCustomizer should give you an idea of what it does, but org.jboss.resteasy.reactive.server.processor.scanning.CacheControlScanner is probably a good place to start

The reason a CDI Interceptor was chosen is we needed something that works for both RESTEasy Classic and RESTEasy Reactive. Maybe having the common annotation transformer is all that needs to be common, and we can have specific ways to retrieve the annotation and add it to the context.

Furthermore, is the new obversability module meant to be a hard dependency? Can't RESTEasy Classic and RESTEasy Reactive extensions just depend on an SPI module for the build items?

Yes, it's meant to be a hard dependency. There originally was an SPI with just the build item, but then it became too complicated as to how to include the actual deployment dependency given the logic of "if (RESTEasy Classic OR RESTEasy Reactive) AND (Micrometer OR OpenTelemetry)". It was decided the best option was to directly include in the REST extensions but have the processor do nothing if Micrometer or OpenTelemetry aren't present.

OK

@kenfinnigan
Copy link
Member

Reading the annotation part is what has me worried.
We have tried very hard in RESTEasy Reactive to avoid reading annotations at runtime, so if this can be avoided, I would much prefer it.

Even when it's a straight read and then set without any computation associated with it?

What alternatives are available to do what is needed?

Basically you would need to add a new ServerRestHandler and alter the handler chain with the HandlerChainCustomizer using the BEFORE_METHOD_INVOKE phase.
Any of the usages of HandlerChainCustomizer should give you an idea of what it does, but org.jboss.resteasy.reactive.server.processor.scanning.CacheControlScanner is probably a good place to start

Thanks

@geoand
Copy link
Contributor

geoand commented Jun 10, 2021

Reading the annotation part is what has me worried.
We have tried very hard in RESTEasy Reactive to avoid reading annotations at runtime, so if this can be avoided, I would much prefer it.

Even when it's a straight read and then set without any computation associated with it?

Yes, our strategy has been to put this kind of thing in it's own ServerRestHandler that is only added to the processing pipeline if necessary

What alternatives are available to do what is needed?

Basically you would need to add a new ServerRestHandler and alter the handler chain with the HandlerChainCustomizer using the BEFORE_METHOD_INVOKE phase.
Any of the usages of HandlerChainCustomizer should give you an idea of what it does, but org.jboss.resteasy.reactive.server.processor.scanning.CacheControlScanner is probably a good place to start

Thanks

👍🏼

@kenfinnigan
Copy link
Member

Where do we store the path value if not on an annotation? We need to store it somewhere between the annotation transformer in the build step and runtime.

@geoand
Copy link
Contributor

geoand commented Jun 10, 2021

You could stuff the value in the implementation of ServerRestHandler you use - much as CacheControlHandler does.

If you like, I can have a go at this on Monday.

@kenfinnigan
Copy link
Member

If you have some time, that would be appreciated

@geoand
Copy link
Contributor

geoand commented Jun 10, 2021

Sure yeah, I'll take care of it on Monday. I would do it tomorrow, but I have a talk to give so it's very unlikely I'll have the time :)

@ebullient
Copy link
Member Author

ebullient commented Jun 10, 2021

That doesn't resolve the issue, Georgios. Doing something special in Resteasy reactive doesn't fix/address the mess that is resteasy classic.
While this is "reading an annotation" .. it is doing it via a lookup in a table (I don't know if that is better or worse, but it is a lookaside because it's the special Arc transformed annotations, and not those that were originally on the method discovered via reflection).

Part of what is nice about this solution is that it is comparatively simple. I've tried doing this 8 different ways, and this is the way that covers resteasy reactive AND resteasy classic (AND the clients in both cases).

@geoand
Copy link
Contributor

geoand commented Jun 11, 2021

I'll fix things on the RESTEasy Reactive side on Monday, but RESTEasy Classic is up you to address the issues based on what Stuart brought up.

@stuartwdouglas
Copy link
Member

As in mentioned in my previous comment the client is broken, it will overwrite the server version, and can't handle concurrent async requests.

@ebullient
Copy link
Member Author

ebullient commented Jun 11, 2021

As I am observing (putting the micrometer side of these tests together), the interceptor does not overwrite the server value because it uses a different context entirely (which the client filter running later doesn't see). So there is something else to fix there, I agree (and I've moved this PR back to draft while I sort that out).

I understand the desire to do something different for RR. The tests in extensions/micrometer/deployment cover resteasy classic (server and client) and integration-tests/micrometer-prometheus will cover resteasy reactive (server and client). The resteasy reactive tests (minimally) will be failing and ready for you to fix on Monday, @geoand =)

@kenfinnigan
Copy link
Member

Previously I'd moved away from CurrentVertxRequest usage because it didn't make the value available at the point the Tracing SPI is invoked at the end, but I think I might have been doing something slightly different than what you've proposed @stuartwdouglas so will try that approach again

@ebullient
Copy link
Member Author

ebullient commented Jun 11, 2021

[Edit] Things have been moved around.
The annotation processing (looking for @Path annotations) is still done, but only in resteasy classic (server) case.

@geoand .. tests have been updated with expected URIs:

  • extensions/micrometer/deployment/ .... UriTag* tests verify resteasy classic behavior (client is still failing)
  • integration-tests/micrometer-prometheus/... tests verify resteasy reactive behavior (both server and client tests are failing)

OpenTelemetry with RR is not ready yet.

@ebullient ebullient changed the title Add resteasy-observability module Common URL Template path handling for REST requests (server and client) Jun 11, 2021
@geoand
Copy link
Contributor

geoand commented Jun 14, 2021

The RESTEasy Reactive server part is done and I am looking into the client part as well (which seems to be more involved).
Not sure if I'll get it done today however since I have an Insights show and then need to step out to run some errands.

@geoand
Copy link
Contributor

geoand commented Jun 14, 2021

I pushed a fix for the Reactive REST Client as well.

There is still one test failing:

PrometheusMetricsRegistryTest.testClientRequests:87 but that fails with a 404 so I think there is something wrong with that test

@ebullient
Copy link
Member Author

PrometheusMetricsRegistryTest.testClientRequests:87 but that fails with a 404 so I think there is something wrong with that test

Yep, there was. All fixed. Found another issue with the classic path, that's also been corrected. Let's see how CI does.

@ebullient ebullient marked this pull request as ready for review June 14, 2021 21:26
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 14, 2021

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

Failing Jobs - Building 61268cf

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 14, 2021

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

Failing Jobs - Building a1e4c61

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 15, 2021

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

Failing Jobs - Building 4d0bbe4

Status Name Step Test failures Logs Raw logs
Native Tests - Misc4 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Native Tests - Misc4 #

📦 integration-tests/opentelemetry

io.quarkus.it.opentelemetry.OpenTelemetryITCase.testClientTracing - More details - Source on GitHub

} finally {
if (annotation != null) {
((HttpServerRequestInternal) request.getCurrent().request()).context().putLocal("UrlPathTemplate",
annotation.value());
Copy link
Member

Choose a reason for hiding this comment

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

Note that this won't deal with subresources, but I don't know how big an issue this is. As sub resources are directly instantiated by the user code you can't guarantee that interceptors will be applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

would a subresource have a path annotation?

Copy link
Member

Choose a reason for hiding this comment

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

Well the initial resource locator method will, however the resource that is returned likely won't be managed by CDI so won't be intercepted.

@stuartwdouglas
Copy link
Member

Test failure looks related, it looks like a META-INF/services entry is not registered in native mode.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 15, 2021

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building 77c361b

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 16 Build ⚠️ Check → Logs Raw logs

 - Add tests for Micrometer and OpenTelemetry to verify server and client uri is parameterized
 - Add UrlTemplatePath handling for RESTEasy Reactive Server
 - Add UrlTemplatePath handling for Reactive REST Client
 - Move Opentelemetry listener registration to relevant extension
 - Use a DynamicFeature to set the UrlPathTemplate
 - Expand existing MethodScanner

Co-authored-by: Erin Schnabel <ebullient@redhat.com>
Co-authored-by: Ken Finnigan <ken@kenfinnigan.me>
Co-authored-by: Georgios Andrianakis <geoand@gmail.com>
Co-authored-by: Stuart Douglas <stuart.w.douglas@gmail.com>
@kenfinnigan kenfinnigan dismissed their stale review June 15, 2021 13:25

I probably shouldn't approve as I was involved in it

@geoand
Copy link
Contributor

geoand commented Jun 15, 2021

I probably shouldn't approve as I was involved in it

@kenfinnigan we have all been involved in it, so that would apply to any of our reviews :)

Copy link
Member Author

@ebullient ebullient left a comment

Choose a reason for hiding this comment

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

The changes that aren't mine LGTM! (but I can't approve as it is my PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/metrics area/resteasy-reactive area/tracing
Projects
None yet
5 participants