roryqi commented on code in PR #13938:
URL: https://github.com/apache/gravitino/pull/13938#discussion_r4312115385
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java:
##########
@@ -224,42 +226,18 @@ protected void configure() {
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]));
+ server.addCustomFilters(ICEBERG_SPEC);
Review Comment:
Addressed in : when standalone metrics are enabled, both metrics paths
receive , , and configured custom filters again. I also added a direct
main-server endpoint test. I did not restore the separate standalone audit-path
coverage test in this commit; I’ll keep this thread open so that gap is visible.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java:
##########
@@ -152,21 +153,14 @@ eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatch
}
// 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]));
+ server.addCustomFilters(LANCE_SPEC);
Review Comment:
Addressed in : standalone metrics-enabled mode now applies request-context,
audit, and configured custom filters to both metrics paths. I did not add the
dedicated standalone audit-coverage test yet, so I’m leaving this thread open.
##########
server/src/main/java/org/apache/gravitino/server/GravitinoServer.java:
##########
@@ -133,6 +133,7 @@ public void initialize() {
JettyServerConfig jettyServerConfig =
JettyServerConfig.fromConfig(serverConfig, WEBSERVER_CONF_PREFIX);
server.initialize(jettyServerConfig, SERVER_NAME);
+ server.addMetricsServlets(gravitinoEnv.metricsSystem());
Review Comment:
Added the direct assertion for both and on the main server in ; the
targeted test passes.
##########
docs/iceberg-rest-service.md:
##########
@@ -140,6 +140,7 @@ distribution published under the `com.datastrato` Maven
group. Its version is pi
| `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.metrics.enabled` | Whether standalone mode
serves `/metrics` and `/prometheus/metrics`. Ignored in auxiliary mode, where
metrics remain available on the Gravitino server endpoint. Enable only when the
standalone service port is trusted. | `false` | No |
Review Comment:
Addressed the deployment opt-in pieces in : added Helm values for both
standalone charts, Docker environment-variable mappings, and the
opt-in/security note in . I couldn’t find a versioned release-note file in this
repository, so the behavior change is documented in the metrics guide for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
