-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Do not prefix URL when URL is already absolute #45300
base: main
Are you sure you want to change the base?
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the package, is this code path for devmode only ? I was reproducing #36510 in production mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
🎊 PR Preview 1ffcee2 has been successfully built and deployed to https://quarkus-pr-main-45300-preview.surge.sh/version/main/guides/
|
Status for workflow
|
Status for workflow
|
@brunobat if you could validate it indeed fixes the whole issue when you're back from PTO, that would be awesome! |
Sure, will do on Thu. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the fix doesn't work.
I see traces for the dev UI but the application spans and the /metrics one are not collected.
Could you clarify what you see and shouldn't see or the opposite? What I was trying to fix here are the If you have other calls to clarify, let me know and I'll have a look on Monday. |
Using the otel QuickStart as an example, After the change I don't see traces for the app's We probably can have a quick chat about this. I feel we need a reproducer test for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just read @brunobat comment - need further investigation.
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 #36510