Skip to content

Comments

fix(server): Filter dynamice path(PUT/GET/DELETE) with params cause OOM#2569

Merged
VGalaxies merged 8 commits intoapache:masterfrom
JackyYangPassion:fix-2566
Nov 6, 2024
Merged

fix(server): Filter dynamice path(PUT/GET/DELETE) with params cause OOM#2569
VGalaxies merged 8 commits intoapache:masterfrom
JackyYangPassion:fix-2566

Conversation

@JackyYangPassion
Copy link
Contributor

@JackyYangPassion JackyYangPassion commented Jul 2, 2024

Purpose of the PR

Main Changes

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 2, 2024
@dosubot dosubot bot added the bug Something isn't working label Jul 2, 2024
@imbajin
Copy link
Member

imbajin commented Jul 2, 2024

@SunnyBoy-WYH could u take a look for it~

@SunnyBoy-WYH
Copy link
Contributor

can we merge the requestContext.getUriInfo()?

URI uri = requestContext.getUriInfo().getRequestUri();
String path = uri.getRawPath();
String method = requestContext.getMethod();
UriInfo uriInfo = requestContext.getUriInfo();
String path = normalizePath(uriInfo.getPath(),method);
String metricsName = join(path, method);

@SunnyBoy-WYH
Copy link
Contributor

Let’s not talk about the oom for now. for HISTOGRAM metric,seems path without param will be better.? @JackyYangPassion @imbajin

@SunnyBoy-WYH
Copy link
Contributor

As a consult, Do you have oom system log and ensure the oom due to this map?

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 11, 2024
@JackyYangPassion
Copy link
Contributor Author

As a consult, Do you have oom system log and ensure the oom due to this map?

Yes
Correction approach:

1. Determine if there are variable parameters in the request path.
2. If there are, replace the values with keys to normalize the metrics.
3. Special handling for /{graphs}/ to accommodate multi-graph mode.

@imbajin imbajin requested a review from javeme July 11, 2024 09:48
Copy link
Contributor

@SunnyBoy-WYH SunnyBoy-WYH left a comment

Choose a reason for hiding this comment

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

can we del LOG.debug ?

@codecov
Copy link

codecov bot commented Jul 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 1.64%. Comparing base (0cb6115) to head (5b087d6).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...g/apache/hugegraph/api/filter/AccessLogFilter.java 0.00% 23 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0cb6115) and HEAD (5b087d6). Click for more details.

HEAD has 5 uploads less than BASE
Flag BASE (0cb6115) HEAD (5b087d6)
7 2
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #2569       +/-   ##
============================================
- Coverage     47.76%   1.64%   -46.13%     
+ Complexity      821      73      -748     
============================================
  Files           720     707       -13     
  Lines         58980   56759     -2221     
  Branches       7604    7186      -418     
============================================
- Hits          28174     934    -27240     
- Misses        27986   55764    +27778     
+ Partials       2820      61     -2759     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@imbajin
Copy link
Member

imbajin commented Jul 20, 2024

can we del LOG.debug ?

@SunnyBoy-WYH Thanks, adjust it to trace & use {} format for it, anymore suggestion?

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 5, 2024
@VGalaxies VGalaxies merged commit 392dffc into apache:master Nov 6, 2024
VGalaxies pushed a commit that referenced this pull request Nov 6, 2024
…OM (#2569)

Co-authored-by: imbajin <jin@apache.org>
Co-authored-by: yangjiaqi <jiaqi.yang@veriti@xyz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] AccessLogFilter for path matrics make Server OOM

5 participants