jerryshao commented on issue #11230: URL: https://github.com/apache/gravitino/issues/11230#issuecomment-4552482678
## Comparison: NCSARequestLog vs. Servlet Filter → EventBus Two approaches have been proposed to address the HTTP-level visibility gap. Here's a structured comparison. ### Approach A (original proposal): Jetty `NCSARequestLog` Wire Jetty's built-in `NCSARequestLog` in `JettyServer.initialize()` via `server.setRequestLog()`, controlled by new config keys (`accessLog.enabled`, `accessLog.path`, `accessLog.retainDays`). ### Approach B (alternative): Servlet Filter → EventBus → AuditLogManager A custom `javax.servlet.Filter` scoped to 4xx/5xx responses, emitting synthetic events onto the `EventBus`, flowing through `AuditLogManager` and the existing `AuditLogWriter`. --- ### Coverage comparison | Failure type | Approach A | Approach B | |---|:---:|:---:| | TLS/SSL handshake failure | ✅ | ❌ connector-level, never reaches servlet | | HTTP-level malformed framing | ✅ | ❌ rejected by Jetty parser before dispatch | | Unknown context path | ✅ | ❌ outside servlet context | | Auth filter rejection (401) | ✅ | ⚠️ only if filter is positioned before `AuthenticationFilter` | | JAX-RS unknown route (404) | ✅ | ✅ | | Normal business operations | ✅ | ✅ | Approach B cannot capture requests that fail *before* the servlet filter chain — which is exactly the gap the issue identifies. --- ### Pros and Cons **Approach A — NCSARequestLog** | Pros | Cons | |---|---| | Captures 100% of HTTP traffic including pre-servlet failures | Produces a separate log file (not unified with audit log) | | Standard NCSA/Combined format; compatible with all major log aggregators | No business context (user identity, operation type) beyond what HTTP headers carry | | Zero coupling to Gravitino business logic | New config surface area (`accessLog.*` keys) | | Works identically across all three servers (Gravitino, Iceberg REST, Lance REST) | | | No new event model changes required | | **Approach B — Servlet Filter → EventBus** | Pros | Cons | |---|---| | Reuses existing `AuditLogWriter` — no new log destination | Cannot capture TLS failures, malformed HTTP, or pre-servlet rejections | | Async via `EventBus` means no request latency impact | `AuditLog` schema is business-oriented; HTTP-level fields (`method`, `URI`, `status`, `latency`) don't map cleanly to existing `Operation`/`identifier()` fields | | Single log stream for compliance querying | `EventBus`/`AuditLogManager` are wired into `GravitinoEnv` — not uniformly available across standalone Iceberg/Lance deployments | | Can be added via existing `customFilters` extension without core changes | 4xx/5xx scoping is arbitrary: a 200 from an unusual source IP is security-relevant; a 404 for a static asset is not | | | Pollutes the audit log with infrastructure noise (health checks, Prometheus scrapes, etc.) | --- ### Recommendation Approach A (NCSARequestLog) is recommended as the primary implementation because it is the only option that fully addresses the stated requirement: *"a complete, unfiltered record of every request, including those that never reach the application layer."* For teams requiring a unified search stream, the standard practice is to ship both the access log and the audit log to the same SIEM index (Elasticsearch, Splunk, etc.). This achieves unified queryability without semantic contamination of the audit log. Approach B could serve as a future **enhancement** — enriching audit entries with HTTP-layer metadata for requests that do reach the servlet layer — but is not a viable replacement for an HTTP access log. -- 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]
