-
Notifications
You must be signed in to change notification settings - Fork 517
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.
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(api): refactor/downgrade record logic for slow log #2347
Conversation
add some TODOs & assign @SunnyBoy-WYH to address it
@Override | ||
public String toString() { | ||
return "SlowQueryLog{executeTime=" + executeTime + | ||
", startTime=" + startTime + | ||
", rawQuery='" + rawQuery + '\'' + | ||
", method='" + method + '\'' + | ||
", threshold=" + threshold + | ||
", path='" + path + '\'' + | ||
'}'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to use this class now, just reserve it
Codecov Report
@@ Coverage Diff @@
## master #2347 +/- ##
============================================
+ Coverage 60.44% 65.90% +5.46%
- Complexity 678 981 +303
============================================
Files 507 507
Lines 42083 42073 -10
Branches 5832 5831 -1
============================================
+ Hits 25438 27730 +2292
+ Misses 14069 11642 -2427
- Partials 2576 2701 +125
... and 90 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
...raph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java
Outdated
Show resolved
Hide resolved
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/PathFilter.java
Outdated
Show resolved
Hide resolved
method, timeThreshold, path); | ||
LOG.info("Slow query: {}", JsonUtil.toJson(log)); | ||
// Record slow query if meet needs, watch out the perf | ||
if (timeThreshold > 0 && timeThreshold < executeTime && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer executeTime > timeThreshold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to just update the small change in this PR so that we don’t have to keep track of it.
background
: in order to make it easier to read/understand, prefer to determine whether the executeTime exceeds a threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to just update the small change in this PR so that we don’t have to keep track of it.
background
: in order to make it easier to read/understand, prefer to determine whether the executeTime exceeds a threshold.
Done, already divide it, @SunnyBoy-WYH could address the background
in next PR
@@ -19,25 +19,36 @@ | |||
|
|||
public class SlowQueryLog { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be removed?
depends on whether to use it for subsequent optimization(could keep it now)
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/metrics/SlowQueryLog.java
Outdated
Show resolved
Hide resolved
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/metrics/SlowQueryLog.java
Outdated
Show resolved
Hide resolved
method, timeThreshold, path); | ||
LOG.info("Slow query: {}", JsonUtil.toJson(log)); | ||
// Record slow query if meet needs, watch out the perf | ||
if (timeThreshold > 0 && timeThreshold < executeTime && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to just update the small change in this PR so that we don’t have to keep track of it.
background
: in order to make it easier to read/understand, prefer to determine whether the executeTime exceeds a threshold.
@javeme hi,like this pr's todo , we need record the request's query language, do you know how can we get the requestbody? from my option, i try some methods but only this work: [get the post query and set it again] ,but it seems caused the bug. ref:https://stackoverflow.com/questions/14560276/how-to-use-jersey-interceptors-to-get-request-body when use loader on C/S model ,it cause GZIP format error. |
* fix(api): refactor/downgrade record logic for slow log add some TODOs & assign @SunnyBoy-WYH to address it * fix typo * enhance the perf
Purpose of the PR
add some TODOs & assign @SunnyBoy-WYH to address it
Main Changes
Verifying these changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODO
Doc - Done
Doc - No Need