This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new f5060c2359 [#13936] fix(server): expose metrics only on main server
(#13938)
f5060c2359 is described below
commit f5060c235984b715d410d1236afde367a3db2190
Author: roryqi <[email protected]>
AuthorDate: Tue Oct 20 09:47:56 2026 +0800
[#13936] fix(server): expose metrics only on main server (#13938)
### What changes were proposed in this pull request?
Make metrics servlet registration opt-in. Iceberg and Lance auxiliary
listeners never expose `/metrics` or `/prometheus/metrics`; their
metrics remain available through the Gravitino server endpoint.
Standalone services can expose the endpoints only when their
service-specific `metrics.enabled` setting is explicitly enabled.
### Why are the changes needed?
The Iceberg REST and Lance REST listeners may be reachable by external
query engines. Serving unauthenticated metrics endpoints on those ports
can expose operational metrics and metric names. The standalone opt-in
preserves monitoring for trusted deployments without reopening these
endpoints in auxiliary mode.
Fixes: #13936
### Does this PR introduce any user-facing change?
Standalone Iceberg and Lance REST deployments no longer serve metrics
endpoints by default. Set `gravitino.iceberg-rest.metrics.enabled=true`
or `gravitino.lance-rest.metrics.enabled=true` to enable them. Auxiliary
mode ignores these settings and keeps metrics on the Gravitino server
endpoint.
### How was this patch tested?
- `./gradlew :server-common:test --tests
org.apache.gravitino.server.web.TestJettyServer`
- `./gradlew :server:test --tests
org.apache.gravitino.server.TestGravitinoServer
:iceberg:iceberg-rest-server:compileJava
:lance:lance-rest-server:compileJava`
- `./gradlew :iceberg:iceberg-rest-server:test --tests
org.apache.gravitino.iceberg.TestRESTService
:lance:lance-rest-server:test --tests
org.apache.gravitino.lance.TestLanceRESTService`
---
.../resources/gravitino-iceberg-rest-server.conf | 3 +-
.../gravitino-iceberg-rest-server/values.yaml | 2 +
.../resources/gravitino-lance-rest-server.conf | 1 +
dev/charts/gravitino-lance-rest-server/values.yaml | 2 +
dev/docker/iceberg-rest-server/rewrite_config.py | 1 +
dev/docker/lance-rest-server/rewrite_config.py | 3 +-
docs/iceberg-rest-service.md | 3 +-
docs/lance-rest-service.md | 17 +++---
docs/metrics.md | 6 +++
.../gravitino/iceberg/common/IcebergConfig.java | 10 ++++
.../org/apache/gravitino/iceberg/RESTService.java | 60 +++++++++++-----------
.../apache/gravitino/iceberg/TestRESTService.java | 60 +++++-----------------
.../test/util/IcebergRESTServerManager.java | 3 +-
.../gravitino/lance/common/config/LanceConfig.java | 10 ++++
.../apache/gravitino/lance/LanceRESTService.java | 58 ++++++++++-----------
.../gravitino/lance/TestLanceRESTService.java | 60 +++++-----------------
.../apache/gravitino/server/web/JettyServer.java | 41 ++++++++-------
.../gravitino/server/web/TestJettyServer.java | 47 +++++++++++++++++
.../apache/gravitino/server/GravitinoServer.java | 5 +-
.../gravitino/server/TestGravitinoServer.java | 18 +++++++
20 files changed, 222 insertions(+), 188 deletions(-)
diff --git
a/dev/charts/gravitino-iceberg-rest-server/resources/gravitino-iceberg-rest-server.conf
b/dev/charts/gravitino-iceberg-rest-server/resources/gravitino-iceberg-rest-server.conf
index c4f5a7c680..73affadfc9 100755
---
a/dev/charts/gravitino-iceberg-rest-server/resources/gravitino-iceberg-rest-server.conf
+++
b/dev/charts/gravitino-iceberg-rest-server/resources/gravitino-iceberg-rest-server.conf
@@ -25,6 +25,7 @@ gravitino.iceberg-rest.shutdown.timeout = {{
.Values.icebergRest.shutdownTimeout
gravitino.iceberg-rest.host = {{ .Values.icebergRest.host | default "0.0.0.0"
}}
# The http port number of the built-in web server
gravitino.iceberg-rest.httpPort = {{ .Values.icebergRest.httpPort | default
9001 }}
+gravitino.iceberg-rest.metrics.enabled = {{ .Values.icebergRest.metricsEnabled
| default false }}
# The min thread size of the built-in web server
gravitino.iceberg-rest.minThreads = {{ .Values.icebergRest.minThreads |
default 24 }}
# The max thread size of the built-in web server
@@ -135,4 +136,4 @@ gravitino.iceberg-rest.default-catalog-name = {{
.Values.icebergRest.dynamicConf
{{- range $key, $val := .Values.additionalConfigItems }}
{{ $key }} = {{ tpl $val $ }}
-{{- end }}
\ No newline at end of file
+{{- end }}
diff --git a/dev/charts/gravitino-iceberg-rest-server/values.yaml
b/dev/charts/gravitino-iceberg-rest-server/values.yaml
index 9550b1f60b..6e71127a93 100644
--- a/dev/charts/gravitino-iceberg-rest-server/values.yaml
+++ b/dev/charts/gravitino-iceberg-rest-server/values.yaml
@@ -42,6 +42,8 @@ icebergRest:
host: 0.0.0.0
# The http port number of the built-in web server
httpPort: 9001
+ # Enable unauthenticated metrics endpoints only on a trusted network.
+ metricsEnabled: false
# The min thread size of the built-in web server
minThreads: 24
# The max thread size of the built-in web server
diff --git
a/dev/charts/gravitino-lance-rest-server/resources/gravitino-lance-rest-server.conf
b/dev/charts/gravitino-lance-rest-server/resources/gravitino-lance-rest-server.conf
index e65f31cb38..62fe5439ba 100644
---
a/dev/charts/gravitino-lance-rest-server/resources/gravitino-lance-rest-server.conf
+++
b/dev/charts/gravitino-lance-rest-server/resources/gravitino-lance-rest-server.conf
@@ -24,6 +24,7 @@ gravitino.lance-rest.shutdown.timeout = {{
.Values.lanceRest.shutdownTimeout | d
# The host name of the built-in web server
gravitino.lance-rest.host = {{ .Values.lanceRest.host | default "0.0.0.0" }}
gravitino.lance-rest.httpPort = {{ .Values.lanceRest.httpPort | default 9101 }}
+gravitino.lance-rest.metrics.enabled = {{ .Values.lanceRest.metricsEnabled |
default false }}
gravitino.lance-rest.minThreads = {{ .Values.lanceRest.minThreads | default 24
}}
gravitino.lance-rest.maxThreads = {{ .Values.lanceRest.maxThreads | default
200 }}
gravitino.lance-rest.stopTimeout = {{ .Values.lanceRest.stopTimeout | default
30000 }}
diff --git a/dev/charts/gravitino-lance-rest-server/values.yaml
b/dev/charts/gravitino-lance-rest-server/values.yaml
index 42bc2cf9d9..4efdcd16d1 100644
--- a/dev/charts/gravitino-lance-rest-server/values.yaml
+++ b/dev/charts/gravitino-lance-rest-server/values.yaml
@@ -40,6 +40,8 @@ lanceRest:
shutdownTimeout: 3000
host: 0.0.0.0
httpPort: 9101
+ # Enable unauthenticated metrics endpoints only on a trusted network.
+ metricsEnabled: false
minThreads: 24
maxThreads: 200
stopTimeout: 30000
diff --git a/dev/docker/iceberg-rest-server/rewrite_config.py
b/dev/docker/iceberg-rest-server/rewrite_config.py
index d6ffcf7ec9..84e4cc4ae7 100755
--- a/dev/docker/iceberg-rest-server/rewrite_config.py
+++ b/dev/docker/iceberg-rest-server/rewrite_config.py
@@ -22,6 +22,7 @@ env_map = {
# Catalog
"GRAVITINO_ICEBERG_REST_HOST": "host",
"GRAVITINO_ICEBERG_REST_HTTP_PORT": "httpPort",
+ "GRAVITINO_ICEBERG_REST_METRICS_ENABLED": "metrics.enabled",
"GRAVITINO_ICEBERG_REST_URI": "uri",
"GRAVITINO_ICEBERG_REST_IO_IMPL": "io-impl",
"GRAVITINO_ICEBERG_REST_CATALOG_BACKEND": "catalog-backend",
diff --git a/dev/docker/lance-rest-server/rewrite_config.py
b/dev/docker/lance-rest-server/rewrite_config.py
index 4bc26e8796..bbe9c6ef0d 100755
--- a/dev/docker/lance-rest-server/rewrite_config.py
+++ b/dev/docker/lance-rest-server/rewrite_config.py
@@ -23,7 +23,8 @@ env_map = {
"LANCE_REST_GRAVITINO_URI": "gravitino-uri",
"LANCE_REST_GRAVITINO_METALAKE_NAME": "gravitino-metalake",
"LANCE_REST_HOST": "host",
- "LANCE_REST_PORT": "httpPort"
+ "LANCE_REST_PORT": "httpPort",
+ "LANCE_REST_METRICS_ENABLED": "metrics.enabled",
}
init_config = {
diff --git a/docs/iceberg-rest-service.md b/docs/iceberg-rest-service.md
index d259b71ff6..72feb3bd52 100644
--- a/docs/iceberg-rest-service.md
+++ b/docs/iceberg-rest-service.md
@@ -139,7 +139,8 @@ distribution published under the `com.datastrato` Maven
group. Its version is pi
| `gravitino.iceberg-rest.idleTimeout` | The timeout in ms of idle
connections.
| `30000`
| No |
| `gravitino.iceberg-rest.requestHeaderSize` | The maximum size of an
HTTP request.
| `131072`
| No |
| `gravitino.iceberg-rest.responseHeaderSize` | The maximum size of an
HTTP response.
| `131072`
| No |
-| `gravitino.iceberg-rest.includeErrorStackTrace` | Whether error responses
include server-side stack traces. Set this to `false` in new deployments
because responses can expose internal implementation details. | `true`
| No |
+| `gravitino.iceberg-rest.includeErrorStackTrace` | Whether error responses
include server-side stack traces. Set this to `false` in new deployments
because responses can expose internal implementation details.
| `true`
| No |
+| `gravitino.iceberg-rest.metrics.enabled` | Whether standalone mode
serves metrics endpoints. Ignored in auxiliary mode; use the Gravitino server
endpoint there. Enable only on a trusted network.
| `false`
| No |
| `gravitino.iceberg-rest.customFilters` | Comma-separated list of
filter class names to apply to the APIs.
| (none)
| No |
| `gravitino.iceberg-rest.advertised-uri` | The public endpoint
reported to clients that discover the service through the Gravitino server.
Must be an absolute `http`/`https` URI with a host and no query or fragment.
| (none)
| No |
diff --git a/docs/lance-rest-service.md b/docs/lance-rest-service.md
index 7ca123d86e..92567a6aed 100644
--- a/docs/lance-rest-service.md
+++ b/docs/lance-rest-service.md
@@ -184,14 +184,15 @@ To run Lance REST service independently without Gravitino
server (You need to st
Configure the service by editing
`{GRAVITINO_HOME}/conf/gravitino-lance-rest-server.conf` or passing
command-line arguments:
-| Configuration Property | Description |
Default Value | Required |
-| ----------------------------------------- | -------------------------- |
--------------------- | -------- |
-| `gravitino.lance-rest.namespace-backend` | Namespace metadata backend |
gravitino | Yes |
-| `gravitino.lance-rest.gravitino-uri` | Gravitino server URI |
http://localhost:8090 | Yes |
-| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name |
(none) | Yes |
-| `gravitino.lance-rest.httpPort` | Service port number |
9101 | No |
-| `gravitino.lance-rest.host` | Service hostname |
0.0.0.0 | No |
-| `gravitino.lance-rest.includeErrorStackTrace` | Whether error responses
include stack traces | true | No |
+| Configuration Property | Description
| Default Value | Required |
+|-----------------------------------------------|----------------------------------------------|------------------------|----------|
+| `gravitino.lance-rest.namespace-backend` | Namespace metadata backend
| gravitino | Yes |
+| `gravitino.lance-rest.gravitino-uri` | Gravitino server URI
| http://localhost:8090 | Yes |
+| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name
| (none) | Yes |
+| `gravitino.lance-rest.httpPort` | Service port number
| 9101 | No |
+| `gravitino.lance-rest.host` | Service hostname
| 0.0.0.0 | No |
+| `gravitino.lance-rest.includeErrorStackTrace` | Whether error responses
include stack traces | true | No |
+| `gravitino.lance-rest.metrics.enabled` | Enable standalone metrics
endpoints | false | No |
:::tip
In standalone deployments, you only need to configure
`gravitino.lance-rest.gravitino-metalake`,
diff --git a/docs/metrics.md b/docs/metrics.md
index 7265278257..deb4a43a38 100644
--- a/docs/metrics.md
+++ b/docs/metrics.md
@@ -10,6 +10,12 @@ license: "This software is licensed under the Apache License
version 2."
Apache Gravitino Metrics builds upon the [Dropwizard
Metrics](https://metrics.dropwizard.io/). It exports these metrics through both
JMX and an HTTP server, supporting JSON and Prometheus formats. Retrieve them
via HTTP requests, as illustrated below:
+The Gravitino server exposes both HTTP endpoints. Standalone Iceberg REST and
Lance REST servers
+disable these endpoints by default; set
`gravitino.iceberg-rest.metrics.enabled=true` or
+`gravitino.lance-rest.metrics.enabled=true` to enable them. These settings
apply only when the
+service runs standalone. In auxiliary mode, scrape metrics from the Gravitino
server endpoint.
+Standalone metrics endpoints are unauthenticated, so enable them only on a
trusted network.
+
```shell
// Use Gravitino Server address or Iceberg REST server address to replace
127.0.0.1:8090
// Get metrics in JSON format
diff --git
a/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java
b/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java
index 305bf95242..17d0679c19 100644
---
a/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java
+++
b/iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/IcebergConfig.java
@@ -45,6 +45,16 @@ public class IcebergConfig extends Config implements
OverwriteDefaultConfig {
public static final int DEFAULT_ICEBERG_REST_SERVICE_HTTP_PORT = 9001;
public static final int DEFAULT_ICEBERG_REST_SERVICE_HTTPS_PORT = 9433;
+ /** Whether to expose metrics endpoints when the Iceberg REST service runs
standalone. */
+ public static final ConfigEntry<Boolean> METRICS_ENABLED =
+ new ConfigBuilder("metrics.enabled")
+ .doc(
+ "Whether to expose /metrics and /prometheus/metrics in
standalone mode. "
+ + "This setting is ignored when running as an auxiliary
service.")
+ .version(ConfigConstants.VERSION_2_0_0)
+ .booleanConf()
+ .createWithDefault(false);
+
public static final ConfigEntry<String> CATALOG_BACKEND =
new ConfigBuilder(IcebergConstants.CATALOG_BACKEND)
.doc("Catalog backend of Gravitino Iceberg catalog")
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
index 99077475d9..b54434b3e0 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
@@ -20,7 +20,6 @@ package org.apache.gravitino.iceberg;
import com.google.common.collect.Lists;
import java.security.CodeSource;
-import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -107,6 +106,10 @@ public class RESTService implements
GravitinoAuxiliaryService {
};
MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();
server.initialize(serverConfig, SERVICE_NAME, false /* shouldEnableUI */);
+ boolean metricsEnabled = shouldEnableMetrics(auxMode, icebergConfig);
+ if (metricsEnabled) {
+ server.addMetricsServlets(metricsSystem);
+ }
ResourceConfig config = new ResourceConfig();
config.packages(getIcebergRESTPackages(icebergConfig));
@@ -221,45 +224,44 @@ public class RESTService implements
GravitinoAuxiliaryService {
EventSource.GRAVITINO_ICEBERG_REST_SERVER,
new IcebergHealthCheckPathMatcher()),
ICEBERG_SPEC);
+ if (metricsEnabled) {
+ server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(0));
+ server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(1));
+ server.addFilter(
+ new HttpAuditFilter(
+ eventBus,
+ EventSource.GRAVITINO_ICEBERG_REST_SERVER,
+ new IcebergHealthCheckPathMatcher()),
+ JettyServer.METRICS_PATH_SPECS.get(0));
+ server.addFilter(
+ new HttpAuditFilter(
+ eventBus,
+ EventSource.GRAVITINO_ICEBERG_REST_SERVER,
+ new IcebergHealthCheckPathMatcher()),
+ JettyServer.METRICS_PATH_SPECS.get(1));
+ }
server.addSystemFilters(ICEBERG_SPEC);
// Root-level aliases for health checks to improve compatibility with
various monitoring
- // systems that expect a /health endpoint. Not part of
JettyServer.METRICS_PATH_SPECS below:
+ // systems that expect a /health endpoint.
// HealthAliasServlet forwards every request into /iceberg/health*, which
ICEBERG_SPEC already
// covers via the servlet container's FORWARD dispatcher type, so binding
the filter again
// here would double-log every probe.
server.addServlet(new HealthAliasServlet("/iceberg"), "/health/*");
server.addServlet(new HealthAliasServlet("/iceberg"), "/health.html");
- registerMetricsPathFilters(server, eventBus);
-
- // Custom filters are registered once, across every filtered path in a
single call, so a
- // filter whose init() isn't safe to run more than once per JVM only runs
it once rather than
- // once per pathSpec.
- List<String> customFilterPaths = new
ArrayList<>(JettyServer.METRICS_PATH_SPECS);
- customFilterPaths.add(ICEBERG_SPEC);
- server.addCustomFilters(customFilterPaths.toArray(new String[0]));
+ if (metricsEnabled) {
+ server.addCustomFilters(
+ ICEBERG_SPEC,
+ JettyServer.METRICS_PATH_SPECS.get(0),
+ JettyServer.METRICS_PATH_SPECS.get(1));
+ } else {
+ server.addCustomFilters(ICEBERG_SPEC);
+ }
}
- /**
- * Registers request-context tracking and audit-on-failure coverage on {@link
- * JettyServer#METRICS_PATH_SPECS}. {@code /metrics} and {@code
/prometheus/metrics} used to
- * receive no such coverage at all, with nothing in the build catching it;
{@code
- * RequestContextFilter} is included too so query-parameter capture applies
uniformly, matching
- * {@link #ICEBERG_SPEC}. Package-private and static so a unit test can
exercise it directly
- * against a plain {@link JettyServer}, without booting the rest of {@link
#initServer}. See
- * GH-12760.
- *
- * @param server the Jetty server whose {@link
JettyServer#METRICS_PATH_SPECS} need filter
- * coverage
- * @param eventBus the event bus audit events are dispatched through
- */
- static void registerMetricsPathFilters(JettyServer server, EventBus
eventBus) {
- for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
- server.addFilter(new RequestContextFilter(eventBus), pathSpec);
- server.addFilter(
- new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_ICEBERG_REST_SERVER), pathSpec);
- }
+ static boolean shouldEnableMetrics(boolean auxiliaryMode, IcebergConfig
icebergConfig) {
+ return !auxiliaryMode && icebergConfig.get(IcebergConfig.METRICS_ENABLED);
}
@Override
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
index 51f4a1b8c3..1fb39c4b9c 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/TestRESTService.java
@@ -22,60 +22,12 @@ import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Collections;
-import java.util.Set;
import org.apache.gravitino.iceberg.common.IcebergConfig;
-import org.apache.gravitino.listener.EventBus;
-import org.apache.gravitino.server.web.HttpAuditFilter;
-import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
-import org.apache.gravitino.server.web.JettyServerTestUtils;
-import org.apache.gravitino.server.web.RequestContextFilter;
-import org.eclipse.jetty.servlet.ServletHandler;
import org.junit.jupiter.api.Test;
public class TestRESTService {
- /**
- * RESTService.initServer() previously registered /metrics and
/prometheus/metrics (added by
- * JettyServer#initialize() itself, outside ICEBERG_SPEC) with no audit
coverage at all, and
- * nothing in the build caught it. Rather than parse source text, this
exercises the extracted
- * RESTService#registerMetricsPathFilters against a plain JettyServer and
inspects the real
- * ServletHandler filter mappings it produces, so a broken
JettyServer.METRICS_PATH_SPECS list or
- * a filter that merely appears in a comment cannot pass. See GH-12760.
- */
- @Test
- public void testMetricsPathsHaveAuditFilterCoverage() throws Exception {
- JettyServer server = new JettyServer();
- JettyServerConfig jettyServerConfig = JettyServerConfig.fromConfig(new
IcebergConfig());
- server.initialize(jettyServerConfig, "test-iceberg-rest", false);
- EventBus eventBus = new EventBus(Collections.emptyList());
-
- try {
- RESTService.registerMetricsPathFilters(server, eventBus);
-
- ServletHandler servletHandler =
-
JettyServerTestUtils.getServletContextHandler(server).getServletHandler();
- Set<String> auditedPathSpecs =
- JettyServerTestUtils.filterPathSpecsFor(servletHandler,
HttpAuditFilter.class);
- Set<String> requestContextPathSpecs =
- JettyServerTestUtils.filterPathSpecsFor(servletHandler,
RequestContextFilter.class);
-
- for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
- assertTrue(
- auditedPathSpecs.contains(pathSpec),
- "'" + pathSpec + "' must be covered by HttpAuditFilter, see
GH-12760");
- assertTrue(
- requestContextPathSpecs.contains(pathSpec),
- "'"
- + pathSpec
- + "' must be covered by RequestContextFilter for
query-parameter "
- + "capture, see GH-12760");
- }
- } finally {
- server.stop();
- }
- }
-
/** The documented Iceberg REST key reaches the value the service passes to
its error builder. */
@Test
public void testIncludeErrorStackTraceReadFromServiceConfig() {
@@ -85,4 +37,16 @@ public class TestRESTService {
new
IcebergConfig(Collections.singletonMap("includeErrorStackTrace", "false")))
.isIncludeErrorStackTrace());
}
+
+ @Test
+ public void testMetricsAreEnabledOnlyForConfiguredStandaloneMode() {
+ IcebergConfig disabledConfig = new IcebergConfig();
+ assertFalse(RESTService.shouldEnableMetrics(false, disabledConfig));
+ assertFalse(RESTService.shouldEnableMetrics(true, disabledConfig));
+
+ IcebergConfig enabledConfig =
+ new
IcebergConfig(Collections.singletonMap(IcebergConfig.METRICS_ENABLED.getKey(),
"true"));
+ assertTrue(RESTService.shouldEnableMetrics(false, enabledConfig));
+ assertFalse(RESTService.shouldEnableMetrics(true, enabledConfig));
+ }
}
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java
index f3c0ad5218..c60a619b4c 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/util/IcebergRESTServerManager.java
@@ -145,8 +145,9 @@ public abstract class IcebergRESTServerManager {
JettyServerConfig jettyServerConfig =
JettyServerConfig.fromConfig(serverConfig,
IcebergConfig.ICEBERG_CONFIG_PREFIX);
String host = jettyServerConfig.getHost();
+ String checkHost = "0.0.0.0".equals(host) ? "127.0.0.1" : host;
int port = jettyServerConfig.getHttpPort();
- this.checkUri = String.format("http://%s:%d/metrics";, host, port);
+ this.checkUri = String.format("http://%s:%d/iceberg/health/live";,
checkHost, port);
LOG.info("Check uri:{}.", checkUri);
}
}
diff --git
a/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/config/LanceConfig.java
b/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/config/LanceConfig.java
index d7579383bf..0222928ae4 100644
---
a/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/config/LanceConfig.java
+++
b/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/config/LanceConfig.java
@@ -42,6 +42,16 @@ public class LanceConfig extends Config implements
OverwriteDefaultConfig {
public static final String GRAVITINO_NAMESPACE_BACKEND = "gravitino";
public static final String GRAVITINO_URI = "http://localhost:8090";;
+ /** Whether to expose metrics endpoints when the Lance REST service runs
standalone. */
+ public static final ConfigEntry<Boolean> METRICS_ENABLED =
+ new ConfigBuilder("metrics.enabled")
+ .doc(
+ "Whether to expose /metrics and /prometheus/metrics in
standalone mode. "
+ + "This setting is ignored when running as an auxiliary
service.")
+ .version(ConfigConstants.VERSION_2_0_0)
+ .booleanConf()
+ .createWithDefault(false);
+
public static final ConfigEntry<String> NAMESPACE_BACKEND =
new ConfigBuilder(CONFIG_NAMESPACE_BACKEND)
.doc("The backend implementation for namespace operations")
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
index 59b8a3afbd..c977835286 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
@@ -22,9 +22,7 @@ import static
org.apache.gravitino.lance.common.config.LanceConfig.NAMESPACE_BAC
import static
org.apache.gravitino.lance.service.authorization.LanceRESTAuthInterceptionService.METALAKE_BINDING;
import java.lang.reflect.Constructor;
-import java.util.ArrayList;
import java.util.HashMap;
-import java.util.List;
import java.util.Map;
import javax.inject.Singleton;
import javax.servlet.Servlet;
@@ -87,6 +85,10 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();
EventBus eventBus = GravitinoEnv.getInstance().eventBus();
server.initialize(serverConfig, SERVICE_NAME, false);
+ boolean metricsEnabled = shouldEnableMetrics(auxMode, lanceConfig);
+ if (metricsEnabled) {
+ server.addMetricsServlets(metricsSystem);
+ }
this.lanceNamespace = loadNamespaceImpl(lanceConfig, auxMode);
@@ -144,6 +146,18 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
new HttpAuditFilter(
eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
LANCE_SPEC);
+ if (metricsEnabled) {
+ server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(0));
+ server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(1));
+ server.addFilter(
+ new HttpAuditFilter(
+ eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
+ JettyServer.METRICS_PATH_SPECS.get(0));
+ server.addFilter(
+ new HttpAuditFilter(
+ eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
+ JettyServer.METRICS_PATH_SPECS.get(1));
+ }
server.addSystemFilters(LANCE_SPEC);
if (auxMode) {
server.addFilter(
@@ -152,21 +166,19 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
}
// Root-level aliases for health checks to improve compatibility with
various monitoring
- // systems that expect a /health endpoint. Not part of
JettyServer.METRICS_PATH_SPECS below:
+ // systems that expect a /health endpoint.
// HealthAliasServlet forwards every request into /lance/health*, which
LANCE_SPEC already
// covers via the servlet container's FORWARD dispatcher type, so binding
the filter again
// here would double-log every probe.
server.addServlet(new HealthAliasServlet("/lance"), "/health/*");
server.addServlet(new HealthAliasServlet("/lance"), "/health.html");
- registerMetricsPathFilters(server, eventBus);
-
- // Custom filters are registered once, across every filtered path in a
single call, so a
- // filter whose init() isn't safe to run more than once per JVM only runs
it once rather than
- // once per pathSpec.
- List<String> customFilterPaths = new
ArrayList<>(JettyServer.METRICS_PATH_SPECS);
- customFilterPaths.add(LANCE_SPEC);
- server.addCustomFilters(customFilterPaths.toArray(new String[0]));
+ if (metricsEnabled) {
+ server.addCustomFilters(
+ LANCE_SPEC, JettyServer.METRICS_PATH_SPECS.get(0),
JettyServer.METRICS_PATH_SPECS.get(1));
+ } else {
+ server.addCustomFilters(LANCE_SPEC);
+ }
LOG.info(
"Initialized Lance REST service for backend {} in {} mode, metadata
authorization {}",
@@ -175,6 +187,10 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
enableMetadataAuthorization ? "enabled" : "disabled");
}
+ static boolean shouldEnableMetrics(boolean auxiliaryMode, LanceConfig
lanceConfig) {
+ return !auxiliaryMode && lanceConfig.get(LanceConfig.METRICS_ENABLED);
+ }
+
@Override
public void serviceStart() {
if (server != null) {
@@ -200,26 +216,6 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
}
}
- /**
- * Registers request-context tracking and audit-on-failure coverage on {@link
- * JettyServer#METRICS_PATH_SPECS}. {@code /metrics} and {@code
/prometheus/metrics} used to
- * receive no such coverage at all, with nothing in the build catching it;
{@code
- * RequestContextFilter} is included too so query-parameter capture applies
uniformly, matching
- * {@link #LANCE_SPEC}. Package-private and static so a unit test can
exercise it directly against
- * a plain {@link JettyServer}, without booting the rest of {@link
#serviceInit}. See GH-12760.
- *
- * @param server the Jetty server whose {@link
JettyServer#METRICS_PATH_SPECS} need filter
- * coverage
- * @param eventBus the event bus audit events are dispatched through
- */
- static void registerMetricsPathFilters(JettyServer server, EventBus
eventBus) {
- for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
- server.addFilter(new RequestContextFilter(eventBus), pathSpec);
- server.addFilter(
- new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_LANCE_REST_SERVER), pathSpec);
- }
- }
-
private NamespaceWrapper loadNamespaceImpl(LanceConfig lanceConfig, boolean
auxMode) {
String backendType = lanceConfig.get(NAMESPACE_BACKEND);
LanceNamespaceBackend lanceNamespaceBackend =
LanceNamespaceBackend.fromType(backendType);
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
index 4798ea1f55..aa2d6ad0ca 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/TestLanceRESTService.java
@@ -22,60 +22,12 @@ import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.util.Collections;
-import java.util.Set;
import org.apache.gravitino.lance.common.config.LanceConfig;
-import org.apache.gravitino.listener.EventBus;
-import org.apache.gravitino.server.web.HttpAuditFilter;
-import org.apache.gravitino.server.web.JettyServer;
import org.apache.gravitino.server.web.JettyServerConfig;
-import org.apache.gravitino.server.web.JettyServerTestUtils;
-import org.apache.gravitino.server.web.RequestContextFilter;
-import org.eclipse.jetty.servlet.ServletHandler;
import org.junit.jupiter.api.Test;
public class TestLanceRESTService {
- /**
- * LanceRESTService.serviceInit() previously registered /metrics and
/prometheus/metrics (added by
- * JettyServer#initialize() itself, outside LANCE_SPEC) with no audit
coverage at all, and nothing
- * in the build caught it. Rather than parse source text, this exercises the
extracted
- * LanceRESTService#registerMetricsPathFilters against a plain JettyServer
and inspects the real
- * ServletHandler filter mappings it produces, so a broken
JettyServer.METRICS_PATH_SPECS list or
- * a filter that merely appears in a comment cannot pass. See GH-12760.
- */
- @Test
- public void testMetricsPathsHaveAuditFilterCoverage() throws Exception {
- JettyServer server = new JettyServer();
- JettyServerConfig jettyServerConfig = JettyServerConfig.fromConfig(new
LanceConfig());
- server.initialize(jettyServerConfig, "test-lance-rest", false);
- EventBus eventBus = new EventBus(Collections.emptyList());
-
- try {
- LanceRESTService.registerMetricsPathFilters(server, eventBus);
-
- ServletHandler servletHandler =
-
JettyServerTestUtils.getServletContextHandler(server).getServletHandler();
- Set<String> auditedPathSpecs =
- JettyServerTestUtils.filterPathSpecsFor(servletHandler,
HttpAuditFilter.class);
- Set<String> requestContextPathSpecs =
- JettyServerTestUtils.filterPathSpecsFor(servletHandler,
RequestContextFilter.class);
-
- for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
- assertTrue(
- auditedPathSpecs.contains(pathSpec),
- "'" + pathSpec + "' must be covered by HttpAuditFilter, see
GH-12760");
- assertTrue(
- requestContextPathSpecs.contains(pathSpec),
- "'"
- + pathSpec
- + "' must be covered by RequestContextFilter for
query-parameter "
- + "capture, see GH-12760");
- }
- } finally {
- server.stop();
- }
- }
-
/** The documented Lance REST key reaches the value the service passes to
its error builder. */
@Test
public void testIncludeErrorStackTraceReadFromServiceConfig() {
@@ -85,4 +37,16 @@ public class TestLanceRESTService {
new
LanceConfig(Collections.singletonMap("includeErrorStackTrace", "false")))
.isIncludeErrorStackTrace());
}
+
+ @Test
+ public void testMetricsAreEnabledOnlyForConfiguredStandaloneMode() {
+ LanceConfig disabledConfig = new LanceConfig();
+ assertFalse(LanceRESTService.shouldEnableMetrics(false, disabledConfig));
+ assertFalse(LanceRESTService.shouldEnableMetrics(true, disabledConfig));
+
+ LanceConfig enabledConfig =
+ new
LanceConfig(Collections.singletonMap(LanceConfig.METRICS_ENABLED.getKey(),
"true"));
+ assertTrue(LanceRESTService.shouldEnableMetrics(false, enabledConfig));
+ assertFalse(LanceRESTService.shouldEnableMetrics(true, enabledConfig));
+ }
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
index 7f0dea29e1..ee53532a39 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
@@ -39,7 +39,6 @@ import javax.servlet.DispatcherType;
import javax.servlet.Filter;
import javax.servlet.Servlet;
import org.apache.commons.lang3.StringUtils;
-import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.metrics.MetricsSystem;
import org.apache.gravitino.server.authentication.AuthenticationFilter;
import org.eclipse.jetty.server.ConnectionFactory;
@@ -73,11 +72,11 @@ public class JettyServer {
private static final String HTTP_PROTOCOL = "http/1.1";
/**
- * The pathSpecs {@link #initialize} registers directly on the shared
servlet context when a
- * {@link MetricsSystem} is available, outside of whatever pathspec the
caller itself filters.
- * Callers that need request-context tracking or audit coverage on these
paths (see GH-12760) must
- * apply their own filters to them explicitly, using this constant rather
than re-declaring the
- * literal path strings, so the two can never drift apart.
+ * The pathSpecs {@link #addMetricsServlets} registers directly on the
shared servlet context,
+ * outside of whatever pathspec the caller itself filters. Callers that need
request-context
+ * tracking or audit coverage on these paths (see GH-12760) must apply their
own filters to them
+ * explicitly, using this constant rather than re-declaring the literal path
strings, so the two
+ * can never drift apart.
*/
public static final ImmutableList<String> METRICS_PATH_SPECS =
ImmutableList.of("/metrics", "/prometheus/metrics");
@@ -182,18 +181,6 @@ public class JettyServer {
// Install before authentication, custom filters, and servlet mappings on
every service.
addFilter(new OutOfMemoryErrorFilter(), "/*");
- MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();
- // Metrics System could be null in UT.
- if (metricsSystem != null) {
- MetricRegistry metricRegistry = metricsSystem.getMetricRegistry();
- servletContextHandler.setAttribute(
- "com.codahale.metrics.servlets.MetricsServlet.registry",
metricRegistry);
- servletContextHandler.addServlet(MetricsServlet.class,
METRICS_PATH_SPECS.get(0));
-
- servletContextHandler.addServlet(
- new ServletHolder(metricsSystem.getPrometheusServlet()),
METRICS_PATH_SPECS.get(1));
- }
-
HandlerCollection handlers = new HandlerCollection();
handlers.addHandler(servletContextHandler);
server.setHandler(handlers);
@@ -271,6 +258,24 @@ public class JettyServer {
new FilterHolder(filter), pathSpec,
EnumSet.allOf(DispatcherType.class));
}
+ /**
+ * Adds Dropwizard and Prometheus metrics endpoints to this server.
+ *
+ * <p>Metrics endpoints are opt-in because some {@code JettyServer}
instances listen on ports
+ * exposed directly to external query engines. Call this only for a listener
intended for metrics
+ * scraping.
+ *
+ * @param metricsSystem the metrics system exported by the endpoints
+ */
+ public void addMetricsServlets(MetricsSystem metricsSystem) {
+ MetricRegistry metricRegistry = metricsSystem.getMetricRegistry();
+ servletContextHandler.setAttribute(
+ "com.codahale.metrics.servlets.MetricsServlet.registry",
metricRegistry);
+ servletContextHandler.addServlet(MetricsServlet.class,
METRICS_PATH_SPECS.get(0));
+ servletContextHandler.addServlet(
+ new ServletHolder(metricsSystem.getPrometheusServlet()),
METRICS_PATH_SPECS.get(1));
+ }
+
public boolean isWebUiEnabled() {
return webUiEnabled;
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
index 909666c523..fab893990d 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
@@ -39,6 +39,7 @@ import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.apache.gravitino.Config;
+import org.apache.gravitino.metrics.MetricsSystem;
import org.apache.gravitino.rest.RESTUtils;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.junit.jupiter.api.AfterEach;
@@ -105,6 +106,32 @@ public class TestJettyServer {
jettyServer.stop();
}
+ @Test
+ public void testMetricsEndpointsAreNotRegisteredByDefault() throws
IOException {
+ int port = startServer();
+
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ assertEquals(HttpServletResponse.SC_NOT_FOUND, requestStatus(port,
pathSpec));
+ }
+ }
+
+ @Test
+ public void testMetricsEndpointsCanBeRegisteredExplicitly() throws
IOException {
+ int port = RESTUtils.findAvailablePort(5000, 6000);
+ Config config = new Config(false) {};
+ config.set(JettyServerConfig.WEBSERVER_HOST, "127.0.0.1");
+ config.set(JettyServerConfig.WEBSERVER_HTTP_PORT, port);
+ jettyServer.initialize(JettyServerConfig.fromConfig(config), "test",
false);
+ try (MetricsSystem metricsSystem = new MetricsSystem()) {
+ jettyServer.addMetricsServlets(metricsSystem);
+ jettyServer.start();
+
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ assertEquals(HttpServletResponse.SC_OK, requestStatus(port, pathSpec));
+ }
+ }
+ }
+
@Test
public void testErrorPageIncludesStackTraceByDefault() throws IOException {
String errorPage = requestFailingServlet(new Config(false) {});
@@ -169,6 +196,26 @@ public class TestJettyServer {
}
}
+ private int startServer() throws IOException {
+ int port = RESTUtils.findAvailablePort(5000, 6000);
+ Config config = new Config(false) {};
+ config.set(JettyServerConfig.WEBSERVER_HOST, "127.0.0.1");
+ config.set(JettyServerConfig.WEBSERVER_HTTP_PORT, port);
+ jettyServer.initialize(JettyServerConfig.fromConfig(config), "test",
false);
+ jettyServer.start();
+ return port;
+ }
+
+ private int requestStatus(int port, String path) throws IOException {
+ HttpURLConnection connection =
+ (HttpURLConnection) new URL("http://127.0.0.1:"; + port +
path).openConnection();
+ try {
+ return connection.getResponseCode();
+ } finally {
+ connection.disconnect();
+ }
+ }
+
private static class FailingServlet extends HttpServlet {
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse
response) {
diff --git
a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
index 019e26563b..406e1c7074 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -96,8 +96,8 @@ public class GravitinoServer extends ResourceConfig {
// audit-on-failure coverage. VersioningFilter is intentionally not applied
to these: it
// negotiates Gravitino's "application/vnd.gravitino.vN+json" media type for
the Jersey-managed
// REST surface, and none of these servlets are part of it or read that
header. /metrics and
- // /prometheus/metrics are registered by JettyServer#initialize() itself
(see server-common's
- // JettyServer), outside GravitinoServer's own control entirely. See
GH-12760.
+ // /prometheus/metrics are registered directly on JettyServer's servlet
context, outside the
+ // Jersey-managed REST surface. See GH-12760.
private static final ImmutableList<String> ROOT_MOUNTED_PATHS =
ImmutableList.<String>builder()
.add("/configs")
@@ -133,6 +133,7 @@ public class GravitinoServer extends ResourceConfig {
JettyServerConfig jettyServerConfig =
JettyServerConfig.fromConfig(serverConfig, WEBSERVER_CONF_PREFIX);
server.initialize(jettyServerConfig, SERVER_NAME);
+ server.addMetricsServlets(gravitinoEnv.metricsSystem());
ServerAuthenticator.getInstance().initialize(serverConfig);
diff --git
a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
index 1288446005..7aa46fa6e4 100644
--- a/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
+++ b/server/src/test/java/org/apache/gravitino/server/TestGravitinoServer.java
@@ -157,6 +157,24 @@ public class TestGravitinoServer {
gravitinoServer.stop();
}
+ @Test
+ public void testMetricsEndpointsAreAvailableOnMainServer() throws Exception {
+ gravitinoServer.initialize();
+ gravitinoServer.start();
+
+ int port =
+ JettyServerConfig.fromConfig(spyServerConfig,
GravitinoServer.WEBSERVER_CONF_PREFIX)
+ .getHttpPort();
+ HttpClient client = HttpClient.newHttpClient();
+ for (String path : JettyServer.METRICS_PATH_SPECS) {
+ HttpResponse<String> response =
+ client.send(
+ HttpRequest.newBuilder(URI.create("http://127.0.0.1:"; + port +
path)).GET().build(),
+ HttpResponse.BodyHandlers.ofString());
+ assertEquals(200, response.statusCode(), "Metrics endpoint " + path);
+ }
+ }
+
@Test
public void testStartWithoutInitialise() throws Exception {
assertThrows(RuntimeException.class, () -> gravitinoServer.start());
