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 1 commit
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.config.HugeConfig;
import org.apache.hugegraph.config.ServerOptions;
Expand All @@ -38,6 +41,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 @@ -109,13 +113,25 @@
// 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

}
}
}

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 131 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#L130-L131 

Added lines #L130 - L131 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
40 changes: 26 additions & 14 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 @@ -35,24 +42,29 @@ public class PathFilter implements ContainerRequestFilter {

@Override
public void filter(ContainerRequestContext context) throws IOException {
context.setProperty(REQUEST_TIME, System.currentTimeMillis());
long startTime = System.currentTimeMillis();

context.setProperty(REQUEST_TIME, startTime);

recordRequestJson(context);
}

// TODO: temporarily comment it to fix loader bug, handle it later
/*// record the request json
private void recordRequestJson(ContainerRequestContext context) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer collectRequestParams(), seems 'record' means output to log

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);
BufferedInputStream bufferedStream = new BufferedInputStream(context.getEntityStream());

bufferedStream.mark(Integer.MAX_VALUE);

context.setProperty(REQUEST_PARAMS_JSON,
IOUtils.toString(bufferedStream, Charsets.toCharset(CHARSET)));
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's very large for batch-write, can we cut part of the content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can cut the special length? like 512?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or we will make it can be config? like 512/1024/2048 and default will be 512? @imbajin @liuxiaocs7 @VGalaxies


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