Repository navigation
[#13936] fix(server): expose metrics only on main server - #13938
Conversation
| | `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 | |
There was a problem hiding this comment.
The table and the one below are not aligned.
There was a problem hiding this comment.
Aligned the metrics row with the surrounding configuration table in commit 44dd3299b8. Thanks for catching that.
yuqi1129
left a comment
yuqi1129
left a comment
There was a problem hiding this comment.
Thanks for closing this gap. One blocking issue and a few smaller ones below.
Blocking: the standalone Iceberg REST ITs will time out at startup
IcebergRESTServerManager (iceberg/iceberg-rest-server/src/test/.../integration/test/util/IcebergRESTServerManager.java:149) waits for http://host:port/metrics to return 200 before it treats the server as started. Both the embedded and the deploy managers use that checkUri. With this PR, a standalone server returns 404 for /metrics by default, so the manager waits the full 60 seconds and then fails with "GravitinoIcebergRESTServer start failed". This file is not in the diff, so I'm raising it here.
That affects 15 IT classes, including IcebergRESTServiceIT, the Memory/Jdbc/Hive backends, the S3/OSS/GCS/ADLS credential ITs, the Kerberos ITs, IcebergCacheIT, and TestIcebergExtendAPI.
I checked it locally with TestIcebergExtendAPI:
- On this PR, the server starts ("iceberg-rest web server started ... port 2100") but the probe never gets a 200, and the test fails.
- On the base commit with the same setup, the test passes.
- I pointed the probe at
127.0.0.1for both runs because0.0.0.0does not connect on my machine. That edit was local only and has been reverted.
Suggestion: probe /iceberg/health/live instead. IcebergHealthOperations serves it, and LanceNamespaceAuthorizationIT already probes /lance/health/live. I'd avoid enabling metrics.enabled in the IT config, because then every IT would run with a non-default setting. I did not run the deploy-mode ITs locally, but they use the same checkUri.
| List<String> customFilterPaths = new ArrayList<>(JettyServer.METRICS_PATH_SPECS); | ||
| customFilterPaths.add(ICEBERG_SPEC); | ||
| server.addCustomFilters(customFilterPaths.toArray(new String[0])); | ||
| server.addCustomFilters(ICEBERG_SPEC); |
There was a problem hiding this comment.
When standalone metrics.enabled=true, the metrics paths now lose the filter coverage they had before:
registerMetricsPathFiltersis removed, so/metricsand/prometheus/metricsno longer getRequestContextFilterorHttpAuditFilter. That brings back the gap Servlets mounted outside the /api/* pathspec receive no authentication and no audit #12760 closed, andtestMetricsPathsHaveAuditFilterCoveragewas deleted along with it.addCustomFiltersnow covers onlyICEBERG_SPEC. Some deployments usecustomFiltersfor 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:
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".
There was a problem hiding this comment.
Addressed in commit 44dd329: 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.
| List<String> customFilterPaths = new ArrayList<>(JettyServer.METRICS_PATH_SPECS); | ||
| customFilterPaths.add(LANCE_SPEC); | ||
| server.addCustomFilters(customFilterPaths.toArray(new String[0])); | ||
| server.addCustomFilters(LANCE_SPEC); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Addressed in commit 44dd329: 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.
| JettyServerConfig jettyServerConfig = | ||
| JettyServerConfig.fromConfig(serverConfig, WEBSERVER_CONF_PREFIX); | ||
| server.initialize(jettyServerConfig, SERVER_NAME); | ||
| server.addMetricsServlets(gravitinoEnv.metricsSystem()); |
There was a problem hiding this 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?
There was a problem hiding this comment.
Added the direct assertion for both metrics endpoints on the main server in commit 44dd329; the targeted TestGravitinoServer test passes.
| | `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 | |
There was a problem hiding this 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-servernor thegravitino-lance-rest-serverchart has a value formetrics.enabled, even though both charts' tests useprometheus.io/scrape: "true". dev/docker/iceberg-rest-server/rewrite_config.pyhas 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.
There was a problem hiding this comment.
Addressed the deployment opt-in pieces in commit 44dd329: 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.
|
The standalone Iceberg IT startup probe now checks |
Code Coverage Report
Files
|
What changes were proposed in this pull request?
Make metrics servlet registration opt-in. Iceberg and Lance auxiliary listeners never expose
/metricsor/prometheus/metrics; their metrics remain available through the Gravitino server endpoint. Standalone services can expose the endpoints only when their service-specificmetrics.enabledsetting 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=trueorgravitino.lance-rest.metrics.enabled=trueto 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