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

OpenTelemetry extension traces health endpoints when a custom root-path is configured #46040

Closed
jamesnetherton opened this issue Feb 3, 2025 · 5 comments · Fixed by #46181
Closed
Assignees
Labels
Milestone

Comments

@jamesnetherton
Copy link
Contributor

Describe the bug

Configuring a custom path for the health endpoint like:

quarkus.smallrye-health.root-path=/observe/health

And then invoking endpoints such as /observe/health/live & /observe/health/ready results in them being traced if the OpenTelemetry extension is present. It seems to ignore /observe/health but not sub-paths.

Expected behavior

All endpoint paths under quarkus.smallrye-health.root-path are excluded from tracing.

Actual behavior

See details above.

How to Reproduce?

  1. git clone https://github.com/jamesnetherton/otel-non-app-path-trace.git
  2. cd otel-non-app-path-trace
  3. mvn clean test

OtelTest will fail. It's expecting to find only one application endpoint being traced. But it finds 3 as the health endpoints are also traced.

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.18.1

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

Copy link

quarkus-bot bot commented Feb 3, 2025

/cc @brunobat (opentelemetry), @jmartisk (health), @radcortez (config,opentelemetry), @xstefank (health)

@brunobat
Copy link
Contributor

brunobat commented Feb 3, 2025

There is a workaround for this behaviour.
If you configure quarkus.otel.traces.suppress-application-uris with the health endpoints you are defining for health they will be supressed.

@gsmet
Copy link
Member

gsmet commented Feb 4, 2025

I will have a look.

@gsmet
Copy link
Member

gsmet commented Feb 10, 2025

Yeah so the ignore list is all borked:

[/q/observe/health/live, /q/observe/health/group/, /q/health-ui, /q/observe/health/group, /q/observe/health/well, /q/observe/health/ready, /observe/metrics, /q/observe/metrics, /q/observe/health/started, /q/health-ui, /observe/health, /q/observe/metrics/*]

I will have a look at what's going on.

gsmet added a commit to gsmet/quarkus that referenced this issue Feb 10, 2025

Unverified

The email in this signature doesn’t match the committer email.
We already have the full path at some point so there's really no need to
rebuild it once again, especially since the code was incorrect as it was
adding the /q/ prefix always if it wasn't there in the path.

It's perfectly valid to not have /q in the path, for instance if you set
quarkus.smallrye-health.root-path to /something/else.

I added a new test based on the report in addition to the ones we added
recently for related fixes.

Fixes quarkusio#46040
@gsmet
Copy link
Member

gsmet commented Feb 10, 2025

#46181 should fix it.

@gsmet gsmet closed this as completed in d8daa71 Feb 11, 2025
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Feb 11, 2025
@gsmet gsmet modified the milestones: 3.19 - main, 3.18.3 Feb 11, 2025
gsmet added a commit to gsmet/quarkus that referenced this issue Feb 11, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We already have the full path at some point so there's really no need to
rebuild it once again, especially since the code was incorrect as it was
adding the /q/ prefix always if it wasn't there in the path.

It's perfectly valid to not have /q in the path, for instance if you set
quarkus.smallrye-health.root-path to /something/else.

I added a new test based on the report in addition to the ones we added
recently for related fixes.

Fixes quarkusio#46040

(cherry picked from commit d8daa71)
@gsmet gsmet modified the milestones: 3.18.3, 3.15.4 Feb 24, 2025
gsmet added a commit to jmartisk/quarkus that referenced this issue Feb 24, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We already have the full path at some point so there's really no need to
rebuild it once again, especially since the code was incorrect as it was
adding the /q/ prefix always if it wasn't there in the path.

It's perfectly valid to not have /q in the path, for instance if you set
quarkus.smallrye-health.root-path to /something/else.

I added a new test based on the report in addition to the ones we added
recently for related fixes.

Fixes quarkusio#46040

(cherry picked from commit d8daa71)
gsmet added a commit to jmartisk/quarkus that referenced this issue Feb 24, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
We already have the full path at some point so there's really no need to
rebuild it once again, especially since the code was incorrect as it was
adding the /q/ prefix always if it wasn't there in the path.

It's perfectly valid to not have /q in the path, for instance if you set
quarkus.smallrye-health.root-path to /something/else.

I added a new test based on the report in addition to the ones we added
recently for related fixes.

Fixes quarkusio#46040

(cherry picked from commit d8daa71)
jmartisk pushed a commit to jmartisk/quarkus that referenced this issue Feb 27, 2025
We already have the full path at some point so there's really no need to
rebuild it once again, especially since the code was incorrect as it was
adding the /q/ prefix always if it wasn't there in the path.

It's perfectly valid to not have /q in the path, for instance if you set
quarkus.smallrye-health.root-path to /something/else.

I added a new test based on the report in addition to the ones we added
recently for related fixes.

Fixes quarkusio#46040

(cherry picked from commit d8daa71)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants