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 on RestPathAnnotationProcessor to ensure that the correct url template is computed for metrics #32121

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

souchel
Copy link
Contributor

@souchel souchel commented Mar 24, 2023

This is a proposition to fix a bug on the metric system. When using quarkus-micrometer, the metrics contain information on the time taken by a request. One of the tag is the uri and currently the uri is incorrect.
For example we get:

# HELP http_server_requests_seconds  
# TYPE http_server_requests_seconds summary
http_server_requests_seconds_count{method="GET",outcome="SUCCESS",status="200",uri="/{firstname}/{firstname}",} 1.0
http_server_requests_seconds_sum{method="GET",outcome="SUCCESS",status="200",uri="/{firstname}/{firstname}",} 0.1192732

instead of:

# TYPE http_server_requests_seconds summary
http_server_requests_seconds_count{method="GET",outcome="SUCCESS",status="200",uri="/hello/{firstname}",} 1.0
http_server_requests_seconds_sum{method="GET",outcome="SUCCESS",status="200",uri="/hello/{firstname}",} 0.1192732

Here is a reproducer: https://github.com/souchel/quarkus-metrics-reproducer

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 24, 2023

/cc @brunobat (micrometer), @ebullient (micrometer)

gsmet
gsmet previously requested changes Mar 24, 2023
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.

Thanks. Let's see what @ebullient thinks but PRs should target the main branch and are backported to older branches when it makes sense.

@souchel souchel changed the base branch from 2.16 to main March 27, 2023 08:08
@souchel souchel requested a review from gsmet March 27, 2023 11:05
@gsmet
Copy link
Member

gsmet commented Mar 27, 2023

@ebullient any chance you could have a look? You're our path expert (how lucky!) and this one might be a pain if you're affected.

@gsmet gsmet dismissed their stale review March 27, 2023 12:10

It targets the right branch, now, thanks!

@geoand
Copy link
Contributor

geoand commented Mar 28, 2023

Also cc @brunobat @radcortez

@brunobat
Copy link
Contributor

Will take a look

@brunobat
Copy link
Contributor

brunobat commented Mar 29, 2023

@souchel can you please update your PR by moving from the legacy javax to the new jakarta packages on the test?
Quarkus 3.x now uses jakarta.

The reproducer worked for me and I think the fix is good.
Nice one!

@souchel
Copy link
Contributor Author

souchel commented Mar 29, 2023

@souchel can you please update your PR by moving from the legacy javax to the new jakarta packages on the test? Quarkus 3.x now uses jakarta.

The reproducer worked for me and I think the fix is good. Nice one!

Will do, thanks :)

@brunobat
Copy link
Contributor

Approved the CI run to see how it looks.

@souchel souchel marked this pull request as ready for review March 31, 2023 09:10
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 4, 2023

Failing Jobs - Building ec8decd

Status Name Step Failures Logs Raw logs
✔️ Gradle Tests - JDK 11
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

io.quarkus.gradle.devmode.AnnotationProcessorSimpleModuleDevModeTest.main line 13 - 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:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

io.quarkus.gradle.devmode.AvroDevModeTest.main line 15 - 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:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

io.quarkus.gradle.devmode.BasicKotlinApplicationModuleDevModeTest.main line 19 - 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:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

io.quarkus.gradle.devmode.JandexMultiModuleProjectDevModeTest.main line 21 - 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:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

io.quarkus.gradle.devmode.MultiSourceProjectDevModeTest.main line 22 - 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:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - 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:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

The CI errors seem unrelated.
They work on my machine but are quite slow. That's probably why they are failing.

@souchel
Copy link
Contributor Author

souchel commented Apr 12, 2023

@brunobat I also think the failures are unrelated, what's the next step ?

@brunobat
Copy link
Contributor

@gsmet will merge when the right time comes.

@geoand
Copy link
Contributor

geoand commented Apr 12, 2023

There is no reason to wait for @gsmet in this one.

If you are fine with it, you can go ahead and merge it

@brunobat brunobat merged commit c1657cd into quarkusio:main Apr 12, 2023
@quarkus-bot quarkus-bot bot added this to the 3.1 - main milestone Apr 12, 2023
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.

4 participants