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 commit 44dd3299b8: when standalone metrics are enabled, both
metrics paths receive RequestContextFilter, HttpAuditFilter, 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 commit 44dd3299b8: 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 metrics endpoints on the main server in
commit 44dd3299b8; the targeted TestGravitinoServer 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 commit 44dd3299b8: added Helm
values for both standalone charts, Docker environment-variable mappings, and
the opt-in/security note in docs/metrics.md. 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]
