yuqi1129 commented on code in PR #13938:
URL: https://github.com/apache/gravitino/pull/13938#discussion_r4311444564
##########
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:
Same issue as in `RESTService`: with standalone `metrics.enabled=true`, the
metrics paths lose `RequestContextFilter`/`HttpAuditFilter` (GH-12760) and the
custom filters. It would be good to restore both when metrics are enabled, and
to bring back the audit-coverage test.
##########
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:
nit: no unit test guards this line. Locally, deleting it still passes all 13
tests in `TestGravitinoServer`. Only the ITs would catch it, indirectly,
because `BaseIT`/`MiniGravitino` use the main `/metrics` as their startup
probe. Maybe add a direct assertion that the main server serves `/metrics` and
`/prometheus/metrics`?
##########
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:
When standalone `metrics.enabled=true`, the metrics paths now lose the
filter coverage they had before:
- `registerMetricsPathFilters` is removed, so `/metrics` and
`/prometheus/metrics` no longer get `RequestContextFilter` or
`HttpAuditFilter`. That brings back the gap GH-12760 closed, and
`testMetricsPathsHaveAuditFilterCoverage` was deleted along with it.
- `addCustomFilters` now covers only ``ICEBERG_SPEC``. Some deployments use
`customFilters` for their own authentication, and before this PR those filters
also covered the metrics paths. With the opt-in, the endpoints would be served
without them, which works against the goal of this PR.
Could you keep both when metrics are enabled? Something like:
```java
List<String> customFilterPaths = new ArrayList<>();
if (shouldEnableMetrics(auxMode, config)) {
registerMetricsPathFilters(server, eventBus);
customFilterPaths.addAll(JettyServer.METRICS_PATH_SPECS);
}
customFilterPaths.add(ICEBERG_SPEC);
server.addCustomFilters(customFilterPaths.toArray(new String[0]));
```
Then the deleted test could come back as "metrics enabled -> paths covered;
disabled -> no metrics servlet".
##########
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:
nit: this changes the default for existing standalone deployments, but there
is no way to turn it back on except editing the conf file by hand:
- Neither the `gravitino-iceberg-rest-server` nor the
`gravitino-lance-rest-server` chart has a value for `metrics.enabled`, even
though both charts' tests use `prometheus.io/scrape: "true"`.
- `dev/docker/iceberg-rest-server/rewrite_config.py` has no env var mapping
for it.
Scrapes would silently start returning 404 after upgrade. Could you add a
chart value and an env mapping, note the opt-in in `docs/metrics.md`, and call
it out in the release note? A follow-up issue for the chart and image would
also be fine.
--
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]
