Fix: default value of rate-limit and unnecessary content of access logs. #70

kokokuo · 2022-09-12T10:25:43Z

Description

Fix the default value of sending 5 times requests in 1 minute to 60 times in 1 minute for the rate limit and refactor the access log message.
We rename the AuditLoggingMiddleware to AccessLogMiddleware, and change the module name from audit-log to access-log.

The access log message become to below format:

2022-09-12 10:15:26.997  INFO  [ACCESS_LOG:b995f7d9-9147-4456-b59c-e07b4d16d7a3] --> 227.173.195.209 -- "PUT http://ripe-celebration.org" -- size: 15.93kb 
2022-09-12 10:15:26.998  INFO  [ACCESS_LOG:b995f7d9-9147-4456-b59c-e07b4d16d7a3]  -> header: {"X-Agent":"test-school-client"} 
2022-09-12 10:15:27.000  INFO  [ACCESS_LOG:b995f7d9-9147-4456-b59c-e07b4d16d7a3]  -> query: {"sortby":"score"} 
2022-09-12 10:15:27.001  INFO  [ACCESS_LOG:b995f7d9-9147-4456-b59c-e07b4d16d7a3]  -> params: {"uuid":"9d086934-3156-4e22-8faa-10f52ce1eefa"} 
2022-09-12 10:15:27.002  INFO  [ACCESS_LOG:b995f7d9-9147-4456-b59c-e07b4d16d7a3] <-- status: 200 -- size: 85.2kb 
2022-09-12 10:15:27.002  INFO  [ACCESS_LOG:b995f7d9-9147-4456-b59c-e07b4d16d7a3]  <- header: undefined

Commit Message

977e858 fix(serve): default value of rate limit and unnecessary content of audit logs
- set default max to 60 in rate-limit middleware.
- change audit-log middleware message format.
- update test cases.
916da3e - chore(serve): rename audit-log to access-log
- rename AuditLoggingMiddleware to AccessLogMiddleware.
- rename option name from audit-log to access-log.
- rename AUDIT to ACCESS_LOG in getLogger function.
5099cb4 0 fix(serve): make other scope logger also hidden the file path and function name.
18a90d7 - fix(serve): move the AccessLogMiddleware log after other middleware ran next()
9efb93e - chore(core): refactor logger to make all defined and non-defined logging scope both hide the function name and file path

…dit logs - set default max to 60 in rate-limit middleware. - change audit-log middleware message format. - update test cases.

- rename "AuditLoggingMiddleware" to "AccessLogMiddleware". - rename option name from "audit-log" to "access-log". - rename "AUDIT" to "ACCESS_LOG" in "getLogger" function.

codecov-commenter · 2022-09-13T03:51:53Z

Codecov Report

Base: 92.08% // Head: 91.94% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (18a90d7) compared to base (8bb4910).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #70      +/-   ##
===========================================
- Coverage    92.08%   91.94%   -0.14%     
===========================================
  Files          215      215              
  Lines         2980     2991      +11     
  Branches       346      354       +8     
===========================================
+ Hits          2744     2750       +6     
- Misses         172      175       +3     
- Partials        64       66       +2

Flag	Coverage Δ
build	`94.87% <ø> (ø)`
cli	`91.90% <ø> (ø)`
core	`92.20% <20.00%> (-0.26%)`	⬇️
extension-dbt	`97.43% <ø> (ø)`
extension-debug-tools	`98.11% <ø> (ø)`
integration-testing	`96.42% <ø> (ø)`
serve	`88.80% <92.00%> (-0.03%)`	⬇️
test-utility	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/core/src/lib/utils/logger.ts	`52.27% <20.00%> (-5.23%)`	⬇️
...es/serve/src/lib/middleware/rateLimitMiddleware.ts	`81.81% <85.71%> (-3.90%)`	⬇️
...es/serve/src/lib/middleware/accessLogMiddleware.ts	`93.75% <93.75%> (ø)`
packages/serve/src/lib/middleware/index.ts	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

oscar60310

Other part LGTM.

oscar60310 · 2022-09-14T08:25:38Z

packages/serve/src/lib/middleware/accessLogMiddleware.ts

+    const reqSize = req.length ? bytes(req.length).toLowerCase() : 'none';
+    const respSize = resp.length ? bytes(resp.length).toLowerCase() : 'none';


Nice to print request/response size, but I can't get both of them in lab env, how do we get them to work?

Should we calculate the response size after other middlewares (after next() function)? Response data might be set after them.

Hi @oscar60310, thanks for reviewing and suggesting, I have checked the lab env both, and like you said the request/response size (Content-Length) shows an undefined value ( Also includes integrating-testing and my app.spec test cases in serve package.

So I searching for the reason, and I the request header only have Content-Length when HTTP method is POST or PUT, because Content-Length only calculates payload data size, please check the references below and my test by sample with typescript-koa-starter to add test api:

https://www.ibm.com/docs/en/ibm-mq/9.0?topic=headers-content-length-http-entity-header

https://stackoverflow.com/questions/8540931/what-is-the-correct-content-length-to-set-for-a-get-request

other repository issues also discussion the issue > RequestBodyBufferMiddleware: 411 No Content Length for GET requests? reactphp/http#232 (comment)

https://stackoverflow.com/questions/3854842/content-length-header-with-head-requests

POST request and return JSON format

GET request and return JSON format

For the response header, as we know, because we use the stream and the header will shows Transfer-Encoding: chunck, so Content-Length won't work.

https://stackoverflow.com/questions/2773396/whats-the-content-length-field-in-http-header

GET request and return Stream format

POST request and return Stream format

Currently I still keep the request and response size, because maybe we will have POST / PUT API in the future.

Thanks for the survey!

oscar60310 · 2022-09-14T08:28:29Z

packages/core/src/lib/utils/logger.ts

+    displayFilePath: 'hideNodeModulesOnly',
+    displayFunctionName: true,


May I ask why we set different default config from Access log?

2022-09-14 08:21:41.405 DEBUG [SERVE packages/serve/src/lib/response-formatter/jsonFormatter.ts:71 JsonStringTransformer.<anonymous>] convert to json format stream > done.

The path can be so long that I think it should be displayed with --verbose flag.

Thanks for reviewing, discussing with me, and giving suggestions, I have made CORE, BUILDand , SERVE both hide the file path and function name, and according to the discussion, we will make it show 'hideNodeModulesOnly' and display the function name in the future by implementing the `--verbose flag.

I also refactor the getLogger, so even non above the default logger scope also hides the file path and function name, it refactors by commit 9efb93e

Below is the screenshot:

oscar60310 · 2022-09-14T08:29:30Z

packages/serve/src/lib/middleware/accessLogMiddleware.ts

+    /**
+     * TODO: The response body of our API server might be huge.
+     * We can let users to set what data they want to record in config in the future.
+     */


Should we remove this note?

Thanks for finding the non removed comment, it has been removed, please check it

…ction name.

…ware ran next()

…ing scope both hide the function name and file path

kokokuo · 2022-09-15T07:46:35Z

Hi @oscar60310, The PR has been fixed, please check it, thanks!

kokokuo added 2 commits September 12, 2022 17:32

fix(serve): default value of rate-limit and unnecessary content of au…

977e858

…dit logs - set default max to 60 in rate-limit middleware. - change audit-log middleware message format. - update test cases.

chore(serve): rename "audit-log" to "access-log".

916da3e

- rename "AuditLoggingMiddleware" to "AccessLogMiddleware". - rename option name from "audit-log" to "access-log". - rename "AUDIT" to "ACCESS_LOG" in "getLogger" function.

kokokuo marked this pull request as ready for review September 13, 2022 02:17

kokokuo requested a review from oscar60310 September 13, 2022 02:17

oscar60310 approved these changes Sep 14, 2022

View reviewed changes

kokokuo added 3 commits September 15, 2022 14:41

fix(serve): make other scope logger also hidden the file path and fun…

5099cb4

…ction name.

fix(serve): move the access log middleware logging after other middle…

18a90d7

…ware ran next()

chore(core): refactor logger to make all defined and non-defined logg…

9efb93e

…ing scope both hide the function name and file path

oscar60310 merged commit 06aca46 into develop Sep 15, 2022

oscar60310 deleted the fix/ratelimit-default-value-with-access-logs-contents branch September 15, 2022 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: default value of rate-limit and unnecessary content of access logs. #70

Fix: default value of rate-limit and unnecessary content of access logs. #70

kokokuo commented Sep 12, 2022 •

edited

Loading

codecov-commenter commented Sep 13, 2022 •

edited

Loading

oscar60310 left a comment

oscar60310 Sep 14, 2022 •

edited

Loading

kokokuo Sep 15, 2022 •

edited

Loading

kokokuo Sep 15, 2022

oscar60310 Sep 15, 2022

oscar60310 Sep 14, 2022

kokokuo Sep 15, 2022 •

edited

Loading

oscar60310 Sep 14, 2022

kokokuo Sep 15, 2022

kokokuo commented Sep 15, 2022

		const reqSize = req.length ? bytes(req.length).toLowerCase() : 'none';
		const respSize = resp.length ? bytes(resp.length).toLowerCase() : 'none';

		displayFilePath: 'hideNodeModulesOnly',
		displayFunctionName: true,

Fix: default value of rate-limit and unnecessary content of access logs. #70

Fix: default value of rate-limit and unnecessary content of access logs. #70

Conversation

kokokuo commented Sep 12, 2022 • edited Loading

Description

Commit Message

codecov-commenter commented Sep 13, 2022 • edited Loading

Codecov Report

oscar60310 left a comment

Choose a reason for hiding this comment

oscar60310 Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

kokokuo Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

POST request and return JSON format

GET request and return JSON format

GET request and return Stream format

POST request and return Stream format

kokokuo Sep 15, 2022

Choose a reason for hiding this comment

oscar60310 Sep 15, 2022

Choose a reason for hiding this comment

oscar60310 Sep 14, 2022

Choose a reason for hiding this comment

kokokuo Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

oscar60310 Sep 14, 2022

Choose a reason for hiding this comment

kokokuo Sep 15, 2022

Choose a reason for hiding this comment

kokokuo commented Sep 15, 2022

kokokuo commented Sep 12, 2022 •

edited

Loading

codecov-commenter commented Sep 13, 2022 •

edited

Loading

oscar60310 Sep 14, 2022 •

edited

Loading

kokokuo Sep 15, 2022 •

edited

Loading

kokokuo Sep 15, 2022 •

edited

Loading