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.

Sign up for GitHub

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

Jump to bottom

Introduce server.port tag into http.server.active.requests metric #46695

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Introduce server.port tag into http.server.active.requests metric #46695

merged 1 commit into from
Mar 11, 2025
+10 −3

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 10, 2025

Relates to: #46539

@geoand geoand mentioned this pull request Mar 10, 2025
Closed
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Mar 10, 2025

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

cescoffier
cescoffier requested changes Mar 10, 2025
View reviewed changes
...runtime/src/main/java/io/quarkus/micrometer/runtime/binder/vertx/VertxHttpServerMetrics.java Outdated
@@ -62,6 +62,7 @@ public class VertxHttpServerMetrics extends VertxTcpServerMetrics
activeRequests = new LongAdder();
Gauge.builder(config.getHttpServerActiveRequestsName(), activeRequests, LongAdder::doubleValue)
.tag("url.scheme", httpServerOptions.isSsl() ? "https" : "http")
.tag("server.port", "" + httpServerOptions.getPort())
Copy link
Member

Choose a reason for hiding this comment

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

It won't work for random port and mapped port (containers).

I think we need to be slightly more careful here. We can have the scheme, but the public port is unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it really matter if it's the public port or not? Essentially this would reflect the configuration values.
The only situation it wouldn't work properly is when all ports are configured to be random (which I doubt is a common scenario)

Copy link
Contributor

Choose a reason for hiding this comment

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

We will only now that properly when we have a service registry with runtime values.
I think we can do this for now because production values will match.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, if we ignore the random ports. I still think it should report the public port and not the private port, but as I said, you cannot have that value (typically on OpenShift it would mean finding the service representing you and extracting the port - way to expensive and specific).

Maybe we should not add this tag if the port is 0, or negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you cannot have that value

Yeah exactly.

Maybe we should not add this tag if the port is 0, or negative.

Makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Mar 11, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 1f96fbf.

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

@geoand geoand requested a review from cescoffier March 11, 2025 07:17
@geoand geoand merged commit 20df18b into quarkusio:main Mar 11, 2025
35 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.21 - main milestone Mar 11, 2025
@geoand geoand deleted the #46539-followup branch March 11, 2025 08:04
@gsmet gsmet modified the milestones: 3.21 - main, 3.19.3 Mar 11, 2025
benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this pull request Mar 14, 2025
chore(deps): update all non-major dependencies to v3.19.3 (mulk/quark…
ca69160 
…us-googlecloud-jsonlogging!27)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [io.quarkus:quarkus-extension-processor](https://github.com/quarkusio/quarkus) |  | patch | `3.19.2` -> `3.19.3` |
| [io.quarkus:quarkus-extension-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.19.2` -> `3.19.3` |
| [io.quarkus:quarkus-bom](https://github.com/quarkusio/quarkus) | import | patch | `3.19.2` -> `3.19.3` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.19.2` -> `3.19.3` |

---

### Release Notes

<details>
<summary>quarkusio/quarkus</summary>

### [`v3.19.3`](https://github.com/quarkusio/quarkus/releases/tag/3.19.3)

[Compare Source](quarkusio/quarkus@3.19.2...3.19.3)

##### Complete changelog

-   [#&#8203;45112](quarkusio/quarkus#45112) - Exception about missing maven classes when opening the dev-ui (gradle based project)
-   [#&#8203;46430](quarkusio/quarkus#46430) - ResponseBuilderImpl NumberFormatException with IPv6
-   [#&#8203;46459](quarkusio/quarkus#46459) - Upgrading from 3.18.2 to 3.18.3 Results in OutOfMemoryError when using `@QuarkusTest` with Quarkus Junit 5
-   [#&#8203;46527](quarkusio/quarkus#46527) - Broken archive in vaadin-webcomponent dependency
-   [#&#8203;46566](quarkusio/quarkus#46566) - Issuer-based OIDC tenant resolver should check `quarkus.oidc.token.required-claims`
-   [#&#8203;46615](quarkusio/quarkus#46615) - OIDC client token requests retry not working
-   [#&#8203;46621](quarkusio/quarkus#46621) - Bump testcontainers.version from 1.20.5 to 1.20.6
-   [#&#8203;46624](quarkusio/quarkus#46624) - Devui Database View can not find tables in different schemas
-   [#&#8203;46632](quarkusio/quarkus#46632) - Make ResponseBuilderImpl more ipv6 aware
-   [#&#8203;46634](quarkusio/quarkus#46634) - Check required claims in OIDC issuer-based resolver
-   [#&#8203;46635](quarkusio/quarkus#46635) - Update some dev-ui libs versions
-   [#&#8203;46638](quarkusio/quarkus#46638) - Bump Keycloak version to 26.1.3
-   [#&#8203;46640](quarkusio/quarkus#46640) - Using SocketException in all of the OIDC retry code
-   [#&#8203;46651](quarkusio/quarkus#46651) - Fix non-public schema in DB Viewer for Dev UI
-   [#&#8203;46653](quarkusio/quarkus#46653) - Add -e to quarkus update commands and improve display
-   [#&#8203;46655](quarkusio/quarkus#46655) - JSON-B link
-   [#&#8203;46659](quarkusio/quarkus#46659) - Correct link to JSON-B API
-   [#&#8203;46660](quarkusio/quarkus#46660) - Correct summary text of config-yaml.adoc
-   [#&#8203;46661](quarkusio/quarkus#46661) - Correct summary text of spring-boot-properties.adoc
-   [#&#8203;46664](quarkusio/quarkus#46664) - ArC: fix disposer resolution in case the disposed parameter declares no qualifiers
-   [#&#8203;46680](quarkusio/quarkus#46680) - Fix gradle devui NoClassDefFound
-   [#&#8203;46684](quarkusio/quarkus#46684) - Revert "Execute simple JUnit tests and `@QuarkusComponentTest` first"
-   [#&#8203;46685](quarkusio/quarkus#46685) - Micrometer docs moved - fix links
-   [#&#8203;46695](quarkusio/quarkus#46695) - Introduce `server.port` tag into `http.server.active.requests` metric
-   [#&#8203;46700](quarkusio/quarkus#46700) - Exclude `.github/project.yml` from triggering workflows on push event
-   [#&#8203;46706](quarkusio/quarkus#46706) - Fix wording in quarkus-rest jsonview support
-   [#&#8203;46709](quarkusio/quarkus#46709) - Fix true-false typo
-   [#&#8203;46712](quarkusio/quarkus#46712) - Bump resteasy.version from 6.2.11.Final to 6.2.12.Final
-   [#&#8203;46713](quarkusio/quarkus#46713) - Bump hibernate-orm.version from 6.6.9.Final to 6.6.10.Final
-   [#&#8203;46714](quarkusio/quarkus#46714) - Bump io.micrometer:micrometer-bom from 1.14.4 to 1.14.5

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunobat brunobat brunobat left review comments

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
area/metrics
Projects
None yet
Milestone
3.19.3
Development

Successfully merging this pull request may close these issues.
4 participants
@geoand @cescoffier @brunobat @gsmet