Expose /v1/integrations/gateway/metrics #26548

andythsu · 2025-09-03T21:51:25Z

Description

Expose cpu usage and memory from Trino cluster on an endpoint

Additional context and related issues

Resolves #26549

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

## Section
* Expose metrics for Trino Gateway on /v1/integrations/gateway/metrics (issue #26549)

andythsu · 2025-09-03T22:04:25Z

core/trino-main/src/main/java/io/trino/memory/LocalMemoryManager.java

+        double systemCpuLoad = 0.0;
+        if (OPERATING_SYSTEM_MX_BEAN instanceof com.sun.management.OperatingSystemMXBean) {
+            systemCpuLoad = ((com.sun.management.OperatingSystemMXBean) OPERATING_SYSTEM_MX_BEAN).getCpuLoad();
+        }


looking for suggestions here. This could lead to false metrics if OPERATING_SYSTEM_MX_BEAN instanceof com.sun.management.OperatingSystemMXBean is always false. The systemCpuLoad will always be 0.0, and trino-gateway will think there's no load on this cluster

We could log a warning

Suggested change

}

} else {

// Log a warning, or throw if this metric is critical

log.warn("Could not retrieve system CPU load: OperatingSystemMXBean is not an instance of com.sun.management.OperatingSystemMXBean");

}

We could consider making this a JVM requirement we verify in TrinoSystemRequirements. @electrum what do you think?

Alternatively, we could make this Optional everywhere, and then the client of this endpoint would need to decide what to do when the statistic is not available.

I think it's better to make it a jvm requirement if possible. It's also how it's used here:

trino/core/trino-main/src/main/java/io/trino/server/StatusResource.java

Line 62 in 17e7b42

if (ManagementFactory.getOperatingSystemMXBean() instanceof OperatingSystemMXBean) {

That seems fine to me. We already require com.sun.management.UnixOperatingSystemMXBean and Trino has other implicit requirements on OpenJDK.

wendigo · 2025-09-04T13:42:42Z

Why another endpoint if public v1/status contains CPU load and available memory?

wendigo · 2025-09-04T13:43:30Z

{
  "nodeId": "presto-master",
  "nodeVersion": {
    "version": "477-test-1-7-g515c8c0-dirty"
  },
  "environment": "test",
  "coordinator": true,
  "uptime": "8.73s",
  "externalAddress": "presto-master.docker.cluster",
  "internalAddress": "presto-master.docker.cluster",
  "memoryInfo": {
    "availableProcessors": 16,
    "pool": {
      "maxBytes": 1503238554,
      "reservedBytes": 0,
      "reservedRevocableBytes": 0,
      "queryMemoryReservations": {

      },
      "queryMemoryAllocations": {

      },
      "queryMemoryRevocableReservations": {

      },
      "taskMemoryReservations": {

      },
      "taskMemoryRevocableReservations": {

      },
      "freeBytes": 1503238554
    }
  },
  "processors": 16,
  "processCpuLoad": 0.000903614457831325,
  "systemCpuLoad": 0.142035287566504,
  "heapUsed": 244729616,
  "heapAvailable": 2147483648,
  "nonHeapUsed": 306181120
}

andythsu · 2025-09-04T14:31:09Z

@wendigo as far as I understand /v1/status only retrieves the status "per node". It doesn't give the entire cluster's metrics. This PR aggregates the metrics so it returns the metrics "per cluster"

wendigo · 2025-09-04T14:32:58Z

@andythsu how this PR addresses this need?

andythsu · 2025-09-04T15:49:19Z

@wendigo all nodes are available on the coordinator node. We can easily sum all node's free bytes up

        long totalFreeBytes = clusterMemoryManager.getAllNodesMemoryInfo()
                .values()
                .stream()
                .flatMap(Optional::stream)
                .map(MemoryInfo::getPool)
                .mapToLong(MemoryPoolInfo::getFreeBytes)
                .sum();

andythsu · 2025-09-11T18:56:16Z

core/trino-main/src/main/java/io/trino/server/GatewayResource.java

+
+    @GET
+    @Path("metrics")
+    @ResourceSecurity(MANAGEMENT_READ)


I'm not sure if we want to use MANAGEMENT_READ because this endpoint will be hit by a dummy user from gateway. Most likely it'll be a role account. We may not want this role account to have management privileges.

Since this endpoint is for integrations, maybe we can have something like user A needs to have <integration> privilege in order to call this endpoint. We can easily use this access type for future integrations as well. Not sure if any of the existing access types can achieve such thing.

In my opinion the 'MANAGEMENT_READ' is totally fine :)

We added management read for just this kind of thing. I allows the caller to read data that some may find sensitive, like the IPs of workers. Honestly I would have make this stuff public, but people get concerned.

RoeyoOgen · 2025-09-14T06:02:44Z

core/trino-main/src/main/java/io/trino/server/GatewayResource.java

+
+    @GET
+    @Path("metrics")
+    @ResourceSecurity(MANAGEMENT_READ)


In my opinion the 'MANAGEMENT_READ' is totally fine :)

RoeyoOgen · 2025-09-14T06:08:35Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+
+import static io.trino.server.security.ResourceSecurity.AccessType.MANAGEMENT_READ;
+
+@Path("/v1/integrations/trinoGateway")


maybe use kebab-case or snake_case for paths ?

Actually we should use a path that is more "generic" as this endpoint could be used for other user cases as well. These kind of endpoints over time become a de facto part of the protocol so it's design and shape should be considered carefully.

other endpoints in trino codebase use camel case

@wendigo there was a discussion on slack about making a special endpoint for the gateway like we have for the UI. The implication being that changes to the endpoint would be decided by the gateway team, and could be backwards incompatible if they decided, which they likely will not as they need compatiblity, but it would be their choice.

let's make the endpont call just gateway

RoeyoOgen · 2025-09-14T06:09:44Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+    @ResourceSecurity(MANAGEMENT_READ)
+    public ClusterMetrics clusterMetrics()
+    {
+        long totalFreeBytes = clusterMemoryManager.getAllNodesMemoryInfo()


You're calling clusterMemoryManager.getAllNodesMemoryInfo() twice, which results in unnecessary overhead and potential inconsistency if the state changes between calls.
PLS cache the result in a variable

RoeyoOgen · 2025-09-14T06:10:18Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+                .map(MemoryInfo::getPool)
+                .mapToLong(MemoryPoolInfo::getFreeBytes)
+                .sum();
+        double totalSystemLoad = clusterMemoryManager.getAllNodesMemoryInfo()


Suggested change

double totalSystemLoad = clusterMemoryManager.getAllNodesMemoryInfo()

double aggregatedSystemLoad = clusterMemoryManager.getAllNodesMemoryInfo()

RoeyoOgen · 2025-09-14T06:11:13Z

core/trino-main/src/main/java/io/trino/server/GatewayResource.java

+                .values()
+                .stream()
+                .flatMap(Optional::stream)
+                .map(MemoryInfo::getPool)


can getPool() be null? if so: wrap with a null check to avoid potential NullPointerException

no, pool won't be null.

trino/core/trino-main/src/main/java/io/trino/memory/MemoryInfo.java

Line 34 in 77af5ad

this.pool = requireNonNull(pool, "pool is null");

RoeyoOgen · 2025-09-14T06:11:49Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+        return new ClusterMetrics(totalFreeBytes, totalSystemLoad);
+    }
+
+    public record ClusterMetrics(long totalFreeBytes, double totalSystemLoad)


perhaps add documentation to clarify what each field means

RoeyoOgen · 2025-09-14T06:13:07Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+    @GET
+    @Path("metrics")
+    @ResourceSecurity(MANAGEMENT_READ)
+    public ClusterMetrics clusterMetrics()


rename to getClusterMetrics (as it is actionable)

dain

This all seems fine to me, but @wendigo should signoff.

dain · 2025-09-15T20:01:48Z

core/trino-main/src/main/java/io/trino/memory/LocalMemoryManager.java

+        double systemCpuLoad = 0.0;
+        if (OPERATING_SYSTEM_MX_BEAN instanceof com.sun.management.OperatingSystemMXBean) {
+            systemCpuLoad = ((com.sun.management.OperatingSystemMXBean) OPERATING_SYSTEM_MX_BEAN).getCpuLoad();
+        }


We could consider making this a JVM requirement we verify in TrinoSystemRequirements. @electrum what do you think?

dain · 2025-09-15T20:04:03Z

core/trino-main/src/main/java/io/trino/memory/LocalMemoryManager.java

+        double systemCpuLoad = 0.0;
+        if (OPERATING_SYSTEM_MX_BEAN instanceof com.sun.management.OperatingSystemMXBean) {
+            systemCpuLoad = ((com.sun.management.OperatingSystemMXBean) OPERATING_SYSTEM_MX_BEAN).getCpuLoad();
+        }


Alternatively, we could make this Optional everywhere, and then the client of this endpoint would need to decide what to do when the statistic is not available.

dain · 2025-09-15T20:07:13Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+
+import static io.trino.server.security.ResourceSecurity.AccessType.MANAGEMENT_READ;
+
+@Path("/v1/integrations/trinoGateway")


@wendigo there was a discussion on slack about making a special endpoint for the gateway like we have for the UI. The implication being that changes to the endpoint would be decided by the gateway team, and could be backwards incompatible if they decided, which they likely will not as they need compatiblity, but it would be their choice.

dain · 2025-09-15T20:10:13Z

core/trino-main/src/main/java/io/trino/server/GatewayResource.java

+
+    @GET
+    @Path("metrics")
+    @ResourceSecurity(MANAGEMENT_READ)


We added management read for just this kind of thing. I allows the caller to read data that some may find sensitive, like the IPs of workers. Honestly I would have make this stuff public, but people get concerned.

dain · 2025-09-15T20:10:59Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+    public record ClusterMetrics(long totalFreeBytes, double aggregatedSystemLoad)
+    {}


This can be on one line

andythsu · 2025-09-25T14:40:47Z

@wendigo Could you take another look at this PR?

wendigo · 2025-09-25T16:07:30Z

core/trino-main/src/main/java/io/trino/node/DnsNodeInventoryModule.java

        boolean coordinator = buildConfigObject(ServerConfig.class).isCoordinator();
        if (coordinator) {
            jaxrsBinder(binder).bind(AnnounceNodeResource.class);
-


hmm.. not sure what changed here because it looks fine in my local

wendigo · 2025-09-25T16:07:50Z

core/trino-main/src/main/java/io/trino/server/CoordinatorModule.java

        newExporter(binder).export(ClusterMemoryManager.class).withGeneratedName();

+        // metrics used by Trino Gateway
+        jaxrsBinder(binder).bind(TrinoGatewayResource.class);


drop comment. rename class to GatewayResource.class

wendigo · 2025-09-25T16:09:06Z

core/trino-main/src/main/java/io/trino/server/TrinoGatewayResource.java

+    @Inject
+    public TrinoGatewayResource(ClusterMemoryManager clusterMemoryManager)
+    {
+        this.clusterMemoryManager = clusterMemoryManager;


requireNonNull(clusterMemoryManager, "clusterMemoryManager is null")

wendigo · 2025-09-25T16:10:09Z

core/trino-main/src/main/java/io/trino/server/GatewayResource.java

+     *
+     * @param totalFreeBytes the sum of free memory from each node in the cluster
+     * @param aggregatedSystemLoad the sum of system load from each node in the cluster
+     */


record definition doesn't return anything so it should be rather "represents"

core/trino-main/src/main/java/io/trino/memory/LocalMemoryManager.java

andythsu · 2025-10-09T15:45:16Z

@wendigo this is ready for another review!

core/trino-main/src/main/java/io/trino/server/GatewayResource.java

wendigo · 2025-10-16T14:25:37Z

@andythsu I applied some changes, take a look and if you are ok with it, I can merge this PR

andythsu · 2025-10-16T14:32:19Z

@wendigo is this the only change you made?

    private static final Supplier<Double> SYSTEM_CPU_LOAD = Suppliers
            .memoizeWithExpiration(() -> clamp(((UnixOperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean()).getCpuLoad(), 0.0, 1.0), 5, TimeUnit.SECONDS);

if so, I'm happy with this change. TIL clamp is cool

wendigo · 2025-10-16T14:39:37Z

@andythsu https://github.com/trinodb/trino/compare/58537883e8797e3a95987aa4b6a93810f5203c85..aae18d323dba7f8e522cd5c80bf8833a5c535bd4

andythsu · 2025-10-16T14:44:59Z

@wendigo thanks! huh weird I couldn't find that new commit. The diff LGTM!

ebyhr · 2025-10-16T23:56:38Z

The PR title and description are stale - the actual endpoint is different. Please update it.

cla-bot bot added the cla-signed label Sep 3, 2025

andythsu force-pushed the gateway_metrics branch from 8f50337 to 9d43271 Compare September 3, 2025 22:01

andythsu requested review from Chaho12, dain, mosabua, vishalya and xkrogen September 3, 2025 22:02

andythsu commented Sep 3, 2025

View reviewed changes

andythsu force-pushed the gateway_metrics branch 2 times, most recently from b9a76ec to 11317f9 Compare September 4, 2025 13:27

andythsu force-pushed the gateway_metrics branch from 11317f9 to 5e13008 Compare September 4, 2025 14:28

andythsu force-pushed the gateway_metrics branch from 5e13008 to 0aa7d25 Compare September 4, 2025 15:48

andythsu commented Sep 11, 2025

View reviewed changes

RoeyoOgen reviewed Sep 14, 2025

View reviewed changes

andythsu force-pushed the gateway_metrics branch 2 times, most recently from e3e042f to c4121d7 Compare September 15, 2025 03:46

dain reviewed Sep 15, 2025

View reviewed changes

andythsu force-pushed the gateway_metrics branch from c4121d7 to b0cadb6 Compare September 15, 2025 23:52

wendigo mentioned this pull request Sep 18, 2025

Verify that Trino has access to UnixOperatingSystemMXBean #26657

Merged

andythsu force-pushed the gateway_metrics branch 3 times, most recently from 1e354ce to 4481544 Compare September 24, 2025 13:53

andythsu force-pushed the gateway_metrics branch from 4481544 to 20371e6 Compare September 25, 2025 14:41

wendigo reviewed Sep 25, 2025

View reviewed changes

core/trino-main/src/main/java/io/trino/memory/LocalMemoryManager.java Outdated Show resolved Hide resolved

andythsu force-pushed the gateway_metrics branch 2 times, most recently from 4530594 to 5853788 Compare September 30, 2025 14:45

chenjian2664 reviewed Oct 12, 2025

View reviewed changes

core/trino-main/src/main/java/io/trino/server/GatewayResource.java Show resolved Hide resolved

Expose /v1/integrations/gateway/metrics for Trino Gateway

aae18d3

wendigo force-pushed the gateway_metrics branch from 5853788 to aae18d3 Compare October 16, 2025 11:21

wendigo approved these changes Oct 16, 2025

View reviewed changes

wendigo merged commit 1ca42b4 into trinodb:master Oct 16, 2025
192 of 193 checks passed

github-actions bot added this to the 478 milestone Oct 16, 2025

ebyhr mentioned this pull request Oct 16, 2025

Add Trino 478 release note #26726

Closed

wendigo changed the title ~~Expose /v1/integrations/trinoGateway/metrics~~ Expose /v1/integrations/gateway/metrics Oct 17, 2025

electrum mentioned this pull request Oct 25, 2025

Reapply "Expose /v1/integrations/gateway/metrics for Trino Gateway" #27101

Merged

-        }
+        } else {
+            // Log a warning, or throw if this metric is critical
+            log.warn("Could not retrieve system CPU load: OperatingSystemMXBean is not an instance of com.sun.management.OperatingSystemMXBean");
+        }


		import static io.trino.server.security.ResourceSecurity.AccessType.MANAGEMENT_READ;

		@Path("/v1/integrations/trinoGateway")

	double totalSystemLoad = clusterMemoryManager.getAllNodesMemoryInfo()
	double aggregatedSystemLoad = clusterMemoryManager.getAllNodesMemoryInfo()

		public record ClusterMetrics(long totalFreeBytes, double aggregatedSystemLoad)
		{}

Expose /v1/integrations/gateway/metrics #26548

Expose /v1/integrations/gateway/metrics #26548

Uh oh!

Conversation

andythsu commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

andythsu Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wendigo commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wendigo commented Sep 4, 2025

Uh oh!

andythsu commented Sep 4, 2025

Uh oh!

wendigo commented Sep 4, 2025

Uh oh!

andythsu commented Sep 4, 2025

Uh oh!

andythsu Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andythsu commented Sep 3, 2025 •

edited

Loading

andythsu Sep 3, 2025 •

edited

Loading

wendigo commented Sep 4, 2025 •

edited

Loading

andythsu Sep 11, 2025 •

edited

Loading

wendigo Sep 25, 2025 •

edited

Loading