Copilot commented on code in PR #2466:
URL: https://github.com/apache/hugegraph/pull/2466#discussion_r3323571526
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java:
##########
@@ -128,6 +134,16 @@ public void filter(ContainerRequestContext requestContext,
}
}
+ private String getClientIP(ContainerRequestContext requestContext) {
+ try {
+ UriInfo uriInfo = requestContext.getUriInfo();
+ String host = uriInfo.getRequestUri().getHost();
+ return InetAddress.getByName(host).getHostAddress();
+ } catch (UnknownHostException e) {
+ return "unknown";
+ }
+ }
Review Comment:
`getClientIP` reads `uriInfo.getRequestUri().getHost()`, which is the
**server** host (the host the client connected to, e.g. the value of the `Host`
header or the bind address), not the **client** IP. This will log the server's
own IP/hostname on every slow query, defeating the purpose of the change
("support ... client IP").
`AuthenticationFilter` already shows the correct pattern in this codebase:
inject Grizzly's `Request` and use `request.getRemoteAddr()` (see
`AuthenticationFilter.java:86-90` and `:114-120`). Consider doing the same here.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/PathFilter.java:
##########
@@ -32,27 +39,37 @@ public class PathFilter implements ContainerRequestFilter {
public static final String REQUEST_TIME = "request_time";
public static final String REQUEST_PARAMS_JSON = "request_params_json";
+ public static final int MAX_SLOW_LOG_BODY_LENGTH = 512;
@Override
public void filter(ContainerRequestContext context) throws IOException {
- context.setProperty(REQUEST_TIME, System.currentTimeMillis());
+ long startTime = System.currentTimeMillis();
+
+ context.setProperty(REQUEST_TIME, startTime);
+
+ collectRequestParams(context);
+ }
- // TODO: temporarily comment it to fix loader bug, handle it later
- /*// record the request json
+ private void collectRequestParams(ContainerRequestContext context) throws
IOException {
String method = context.getMethod();
- String requestParamsJson = "";
- if (method.equals(HttpMethod.POST)) {
- requestParamsJson = IOUtils.toString(context.getEntityStream(),
- Charsets.toCharset(CHARSET));
- // replace input stream because we have already read it
- InputStream in = IOUtils.toInputStream(requestParamsJson,
Charsets.toCharset(CHARSET));
- context.setEntityStream(in);
+ if (method.equals(HttpMethod.POST) || method.equals(HttpMethod.PUT) ||
+ method.equals(HttpMethod.DELETE)) {
+ BufferedInputStream bufferedStream = new
BufferedInputStream(context.getEntityStream());
+
+ bufferedStream.mark(Integer.MAX_VALUE);
+ String body = IOUtils.toString(bufferedStream,
+ Charsets.toCharset(CHARSET));
+ body = body.length() > MAX_SLOW_LOG_BODY_LENGTH ?
+ body.substring(0, MAX_SLOW_LOG_BODY_LENGTH) : body;
+
+ context.setProperty(REQUEST_PARAMS_JSON, body);
+
+ bufferedStream.reset();
+
+ context.setEntityStream(bufferedStream);
Review Comment:
Two concerns with the body-capture logic here:
1. Memory/perf: `bufferedStream.mark(Integer.MAX_VALUE)` followed by
`IOUtils.toString(bufferedStream, ...)` causes `BufferedInputStream` to grow
its internal buffer to hold the entire request body in memory so that `reset()`
can replay it. This runs unconditionally for every POST/PUT/DELETE — including
loader batch imports of up to `batch.max_vertices_per_batch` /
`batch.max_edges_per_batch` entries (default 500) which can be many MB — even
when `log.slow_query_threshold <= 0` or when the path is not one that
`AccessLogFilter.needRecordLog` records (i.e. most APIs). The body is also
fully read and converted to a String before being truncated to 512 chars.
Consider gating the capture by the slow-query-log enabled flag and/or
`needRecordLog(...)`, or by limiting the bytes read from the stream (e.g.
`BoundedInputStream` / read only up to `MAX_SLOW_LOG_BODY_LENGTH` before
resetting), to avoid doubling memory and copy cost on the hot batch-import path
that this PR is explicitly tr
ying to unbreak.
2. `HttpMethod.DELETE` requests typically have no body; including DELETE
means wrapping the entity stream and doing an empty read+reset for every delete
call. Unless there is a concrete reason to log DELETE bodies, restricting this
to POST/PUT would be safer and cheaper.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
