Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(server): fix server slow log, support loader import & client IP #2466

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(server): fix server slow log, support loader import & client IP #2466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee09bd1
fix(server): fix server slow log, support loader import & client IP
SunnyBoy-WYH Mar 1, 2024
868adcf
Merge remote-tracking branch 'origin/master' into slow-log-fix
SunnyBoy-WYH Jun 15, 2024
071943c
fix(server): support put/delete & when 512 length then log cut
SunnyBoy-WYH Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions ...h-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,17 @@

package org.apache.hugegraph.api.filter;

import static org.apache.hugegraph.api.filter.PathFilter.REQUEST_PARAMS_JSON;
import static org.apache.hugegraph.api.filter.PathFilter.REQUEST_TIME;
import static org.apache.hugegraph.metrics.MetricsUtil.METRICS_PATH_FAILED_COUNTER;
import static org.apache.hugegraph.metrics.MetricsUtil.METRICS_PATH_RESPONSE_TIME_HISTOGRAM;
import static org.apache.hugegraph.metrics.MetricsUtil.METRICS_PATH_SUCCESS_COUNTER;
import static org.apache.hugegraph.metrics.MetricsUtil.METRICS_PATH_TOTAL_COUNTER;

import java.io.IOException;
import java.net.InetAddress;
import java.net.URI;
import java.net.UnknownHostException;

import org.apache.hugegraph.auth.HugeAuthenticator;
import org.apache.hugegraph.config.HugeConfig;
Expand All @@ -40,6 +43,7 @@
import jakarta.ws.rs.container.ContainerResponseContext;
import jakarta.ws.rs.container.ContainerResponseFilter;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.UriInfo;
import jakarta.ws.rs.ext.Provider;

// TODO: should add test for this class
Expand Down Expand Up @@ -114,9 +118,11 @@
// Record slow query if meet needs, watch out the perf
if (timeThreshold > 0 && executeTime > timeThreshold &&
needRecordLog(requestContext)) {
// TODO: set RequestBody null, handle it later & should record "client IP"
LOG.info("[Slow Query] execTime={}ms, body={}, method={}, path={}, query={}",
executeTime, null, method, path, uri.getQuery());

LOG.info("[Slow Query] ip={} execTime={}ms, body={}, method={}, path={}, query={}",
getClientIP(requestContext), executeTime,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer this style: defining a local var for better readability:
for both: getClientIP and REQUEST_PARAMS_JSON.

requestContext.getProperty(REQUEST_PARAMS_JSON), method, path,
uri.getQuery());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are they repeated when GET: uri.getQuery() and REQUEST_PARAMS_JSON

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for GET ， REQUEST_PARAMS_JSON will extract the id info from /example/{id} ; uri.getQuery() will extract id=1 from http://example.com/resource?id=1

}
}

Expand All @@ -128,6 +134,16 @@
}
}

private String getClientIP(ContainerRequestContext requestContext) {
try {
UriInfo uriInfo = requestContext.getUriInfo();
String host = uriInfo.getRequestUri().getHost();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return here if host is an ip or empty?

return InetAddress.getByName(host).getHostAddress();
} catch (UnknownHostException e) {
return "unknown";

Check warning on line 143 in hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java

View check run for this annotation

Codecov / codecov/patch

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java#L142-L143 

Added lines #L142 - L143 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer "<unknown_ip>"

}
}

private boolean statusOk(int status) {
return status >= 200 && status < 300;
}
Expand Down
47 changes: 32 additions & 15 deletions hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/PathFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,16 @@

package org.apache.hugegraph.api.filter;

import static org.apache.hugegraph.api.API.CHARSET;

import java.io.BufferedInputStream;
import java.io.IOException;

import org.apache.commons.io.Charsets;
import org.apache.commons.io.IOUtils;

import jakarta.inject.Singleton;
import jakarta.ws.rs.HttpMethod;
import jakarta.ws.rs.container.ContainerRequestContext;
import jakarta.ws.rs.container.ContainerRequestFilter;
import jakarta.ws.rs.container.PreMatching;
Expand All @@ -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);
} else if (method.equals(HttpMethod.GET)) {
MultivaluedMap<String, String> pathParameters = context.getUriInfo()
.getPathParameters();
requestParamsJson = pathParameters.toString();
context.setProperty(REQUEST_PARAMS_JSON,
context.getUriInfo().getPathParameters().toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add all other branchs like PUT/DELETE

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add all other branchs like PUT/DELETE -- seems still missing

}

context.setProperty(REQUEST_PARAMS_JSON, requestParamsJson);*/
}
}
Loading