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.

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

feat(api): support recording slow query log #2327

Merged
merged 12 commits into from
Nov 6, 2023

Conversation

SunnyBoy-WYH
Copy link
Contributor

@SunnyBoy-WYH SunnyBoy-WYH commented Oct 16, 2023

Purpose of the PR

subtask of #2325

Main Changes

  1. change the log4j.xml to record slow query log to single log file.
  2. record log in AccessLogFilter.java
  3. add some config items about threshold and enabled.

image

Verifying these changes

we can run test , and we can see:

image

Does this PR potentially affect the following parts?

  • Nope
  • Dependencies (add/update license info)
  • Modify configurations
  • The public API
  • Other affects (typed here)

Documentation Status

  • Doc - TODO

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #2327 (6d3be26) into master (869fc81) will increase coverage by 2.43%.
Report is 5 commits behind head on master.
The diff coverage is 97.29%.

@@             Coverage Diff              @@
##             master    #2327      +/-   ##
============================================
+ Coverage     63.85%   66.28%   +2.43%     
- Complexity      684      981     +297     
============================================
  Files           505      507       +2     
  Lines         41902    42084     +182     
  Branches       5817     5832      +15     
============================================
+ Hits          26756    27897    +1141     
+ Misses        12455    11475     -980     
- Partials       2691     2712      +21     
Files Coverage Δ
...va/org/apache/hugegraph/api/filter/PathFilter.java 100.00% <100.00%> (ø)
...ava/org/apache/hugegraph/config/ServerOptions.java 100.00% <100.00%> (ø)
...ava/org/apache/hugegraph/metrics/SlowQueryLog.java 100.00% <100.00%> (ø)
...g/apache/hugegraph/api/filter/AccessLogFilter.java 96.87% <93.75%> (-3.13%) ⬇️

... and 111 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@imbajin imbajin changed the title Feat: support recording slow query log feat(api): support recording slow query log Oct 17, 2023
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

please rebase latest master code

javeme
javeme previously approved these changes Oct 22, 2023
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

LGTM


MetricsUtil.registerHistogram(
join(metricsName, METRICS_PATH_RESPONSE_TIME_HISTOGRAM))
.update(responseTime);

HugeConfig config = configProvider.get();
Long timeThreshold = config.get(ServerOptions.SLOW_QUERY_LOG_TIME_THRESHOLD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Holding the timeThreshold in a private variable and changing its type to long would be better, reducing the cost of unboxing and hash searching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks , but i dont know “Holding the timeThreshold in a private variable ” means what? thanks again

Copy link
Contributor

Choose a reason for hiding this comment

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

Just like this:

public class AccessLogFilter implements ContainerResponseFilter {
     private long timeThreshold =configProvider.get().get(ServerOptions.SLOW_QUERY_LOG_TIME_THRESHOLD);

.....
}

Of course, you need to check for null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like this:

public class AccessLogFilter implements ContainerResponseFilter {
     private long timeThreshold =configProvider.get().get(ServerOptions.SLOW_QUERY_LOG_TIME_THRESHOLD);

.....
}

Of course, you need to check for null.

we cant get timeThreshold as private var from configProvider, because configProvider managed by @context; it will case NPE.

@imbajin imbajin added this to the 1.2 milestone Oct 27, 2023
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

LGTM

@imbajin
Copy link
Member

imbajin commented Nov 6, 2023

Could also add some basic introduction for the record in doc

@imbajin imbajin merged commit 69e6b46 into apache:master Nov 6, 2023
19 of 21 checks passed
VGalaxies pushed a commit to VGalaxies/incubator-hugegraph that referenced this pull request Nov 10, 2023
* chore(api): code style for cr

---------

Co-authored-by: imbajin <[email protected]>
imbajin added a commit that referenced this pull request Nov 10, 2023
* chore(api): code style for cr

---------

Co-authored-by: imbajin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants