Do not prefix URL when URL is already absolute #45300

gsmet · 2024-12-27T15:40:34Z

Management URLs were prefixed twice when absolute: http://localhost:9000http://localhost:9000/q/health

Which was defeating the logic removing the host when collecting suppressed URIs.

Management URLs were prefixed twice when absolute: http://localhost:9000http://localhost:9000/q/health Which was defeating the logic removing the host when collecting suppressed URIs. Fixes quarkusio#36510

rquinio1A · 2024-12-27T16:03:08Z

...tp/deployment/src/main/java/io/quarkus/vertx/http/deployment/devmode/ConfiguredPathInfo.java

Based on the package, is this code path for devmode only ? I was reproducing #36510 in production mode.

Nope, it's used when building the application itself in this particular case (and yes, it probably shouldn't!).

I'm not a big fan of how things are handled here and I might have a look at improving how these paths are handled at some point (what is extremely problematic is that we resolve the path at build time while they can only be resolved at runtime).

I'm interested in having someone test it because I actually used the debugger and didn't set up a Prometheus. my debugging session seemed in line with what people observed though.

github-actions · 2024-12-27T16:09:44Z

🎊 PR Preview 1ffcee2 has been successfully built and deployed to https://quarkus-pr-main-45300-preview.surge.sh/version/main/guides/

Images of blog posts older than 3 months are not available.
Newsletters older than 3 months are not available.

quarkus-bot · 2024-12-27T16:10:55Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit 8d81d78.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot · 2024-12-27T20:02:54Z

Status for workflow `Quarkus CI`

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

✅ 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/oidc-code-flow

✖ io.quarkus.it.keycloak.CodeFlowTest.testTokenRefresh - History

expected: <true> but was: <false> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at io.quarkus.it.keycloak.CodeFlowTest$3.call(CodeFlowTest.java:734)
	at io.quarkus.it.keycloak.CodeFlowTest$3.call(CodeFlowTest.java:721)
	at org.awaitility.core.CallableCondition$ConditionEvaluationWrapper.eval(CallableCondition.java:99)
	at org.awaitility.core.ConditionAwaiter$ConditionPoller.call(ConditionAwaiter.java:248)
	at org.awaitility.core.ConditionAwaiter$ConditionPoller.call(ConditionAwaiter.java:235)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/opentelemetry-quickstart

✖ io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled - History

Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled(OpenTelemetryDisabledTest.java:29)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

gsmet · 2024-12-28T09:57:46Z

@brunobat if you could validate it indeed fixes the whole issue when you're back from PTO, that would be awesome!

brunobat · 2024-12-31T11:01:39Z

Sure, will do on Thu.

brunobat

Unfortunately the fix doesn't work.
I see traces for the dev UI but the application spans and the /metrics one are not collected.

gsmet · 2025-01-03T15:28:56Z

Could you clarify what you see and shouldn't see or the opposite?

What I was trying to fix here are the /health calls being collected (and in any case, we need to get this PR in, even if the fix is incomplete).

If you have other calls to clarify, let me know and I'll have a look on Monday.

brunobat · 2025-01-03T15:55:45Z

Using the otel QuickStart as an example, After the change I don't see traces for the app's /hello endpoint.
Also, I see no metrics for the /metrics endpoint, after adding the Micrometer extension.

We probably can have a quick chat about this. I feel we need a reproducer test for this.

cescoffier

Just read @brunobat comment - need further investigation.

gsmet · 2025-01-09T12:07:51Z

FWIW, I agree the issue is broader than this particular fix but this fix actually fixes one of the issues.

Let's say you configure the OpenTelemetry quickstart with:

quarkus.management.enabled=true
quarkus.otel.traces.suppress-non-application-uris=true

You end up with TracerRecorder.dropNonApplicationUriTargets:

TracerRecorder.dropNonApplicationUriTargets: [/q/arc/removed-beans, /q/dev-ui/endpoints/, /q/arc/beans, /q/dev-ui/io.quarkus.quarkus-arc/, /q/health-ui*, http://localhost:9000http://localhost:9000/q/health-ui, /q/dev-ui/io.quarkus.quarkus-micrometer/, /q/health/started, /q/dev-ui/io.quarkus.quarkus-rest-client/, /q/arc/observers, /q/open-in-ide/:fileName/:lang/:lineNumber, /q/dev-ui/json-rpc-ws, /q/health/live, http://localhost:9000http://localhost:9000/q/health, /q/dev-ui/io.quarkus.quarkus-observability-devservices/, /q/health/group, /q/health/well, /q/arc, /q/dev, /q/dev-ui/io.quarkus.quarkus-rest/, /q/health/group/, /q/health/ready, /q/dev-ui/, /q/dev-ui, /q/_static/, /q/dev-ui/io.quarkus.quarkus-smallrye-health/]

As you can see, all the URLs are prefixed with /q/. Also you can see the /q/health URL being http://localhost:9000http://localhost:9000/q/health, which is what this PR fixes.

The thing is that if you have a look the generated spans, you will quickly understand why things are not filtered:

As you can see, requests to /q/dev-ui are recorded as /dev-ui, requests to /q/_static/ as /_static (you can't see it in this screenshot but I have the static one in another screenshot) and same for a bunch of other technical URIs.

Thus I suspect these URIs can't be properly dropped.

brunobat · 2025-01-09T12:40:39Z

Me and @gsmet meet and we agree there are 3 issues at least:

host and port should never be included in the FrameworkEndpointsBuildItem or at least only for Dev Mode, not in prod code. We need to understand why some URLs must be absolute.
The fix about the double URL that was originally removed
Pattern marching with "bla/:filename/" are being added.
Spans are being reported without /q and this is independent from the management console is on or off and the suppress-non-application-uris=true. Reported in the previous comment. This is the more concerning point!

gsmet · 2025-01-09T15:22:40Z

We agreed on me digging further and come up with a good idea of what's going on.

For me, we should include what's in this PR + a fix for the span not having the correct route path. This would be backportable.

And then take a step back and fix the reason why we are having absolute URL with host:port at build time as that's not something we want (especially since the port is set at runtime). But this is for later as I'm pretty sure whatever we do to fix that won't be backportable.

Do not prefix URL when URL is already absolute

8d81d78

Management URLs were prefixed twice when absolute: http://localhost:9000http://localhost:9000/q/health Which was defeating the logic removing the host when collecting suppressed URIs. Fixes quarkusio#36510

quarkus-bot bot added the area/vertx label Dec 27, 2024

gsmet mentioned this pull request Dec 27, 2024

quarkus.otel.traces.suppress-non-application-uris not working with quarkus.management.enabled #36510

Open

rquinio1A reviewed Dec 27, 2024

View reviewed changes

quarkus-bot bot added the triage/flaky-test label Dec 27, 2024

gsmet requested a review from brunobat December 28, 2024 09:57

gsmet added triage/backport-3.15 triage/backport labels Dec 28, 2024

brunobat requested changes Jan 2, 2025

View reviewed changes

cescoffier approved these changes Jan 6, 2025

View reviewed changes

cescoffier requested changes Jan 6, 2025

View reviewed changes

brunobat added area/tracing area/metrics labels Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not prefix URL when URL is already absolute #45300

Do not prefix URL when URL is already absolute #45300

gsmet commented Dec 27, 2024

rquinio1A Dec 27, 2024

gsmet Dec 27, 2024

github-actions bot commented Dec 27, 2024

quarkus-bot bot commented Dec 27, 2024

quarkus-bot bot commented Dec 27, 2024 •

edited by github-actions bot

Loading

gsmet commented Dec 28, 2024

brunobat commented Dec 31, 2024

brunobat left a comment

gsmet commented Jan 3, 2025

brunobat commented Jan 3, 2025

cescoffier left a comment

gsmet commented Jan 9, 2025

brunobat commented Jan 9, 2025 •

edited

Loading

gsmet commented Jan 9, 2025

Do not prefix URL when URL is already absolute #45300

Are you sure you want to change the base?

Do not prefix URL when URL is already absolute #45300

Conversation

gsmet commented Dec 27, 2024

rquinio1A Dec 27, 2024

Choose a reason for hiding this comment

gsmet Dec 27, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 27, 2024

quarkus-bot bot commented Dec 27, 2024

Status for workflow Quarkus Documentation CI

quarkus-bot bot commented Dec 27, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/oidc-code-flow

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/opentelemetry-quickstart

gsmet commented Dec 28, 2024

brunobat commented Dec 31, 2024

brunobat left a comment

Choose a reason for hiding this comment

gsmet commented Jan 3, 2025

brunobat commented Jan 3, 2025

cescoffier left a comment

Choose a reason for hiding this comment

gsmet commented Jan 9, 2025

brunobat commented Jan 9, 2025 • edited Loading

gsmet commented Jan 9, 2025

Status for workflow `Quarkus Documentation CI`

quarkus-bot bot commented Dec 27, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

brunobat commented Jan 9, 2025 •

edited

Loading