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

865-update-logs-and-metrics. Added the requestId to the server context. #898

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

ebadiere
Copy link
Contributor

This PR modifies rate limited logs in order to include the requestId in a standard log message.

Adds a Koa requestId package that supports storing the requestId in the application context.
Fixes #885

Notes for reviewer:
Provides the request ID in the logs right as the rateLimit for a given IP is triggered, in the standard log format.

Existing tests cover rateLimited requests.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2023

Codecov Report

Base: 74.75% // Head: 74.41% // Decreases project coverage by -0.34% ⚠️

Coverage data is based on head (2a5771c) compared to base (41debf8).
Patch coverage: 51.85% of modified lines in pull request are covered.

❗ Current head 2a5771c differs from pull request most recent head 1f8e64b. Consider uploading reports for the commit 1f8e64b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #898      +/-   ##
==========================================
- Coverage   74.75%   74.41%   -0.34%     
==========================================
  Files          29       28       -1     
  Lines        1838     1841       +3     
  Branches      344      348       +4     
==========================================
- Hits         1374     1370       -4     
- Misses        367      371       +4     
- Partials       97      100       +3     
Impacted Files Coverage Δ
packages/server/src/rateLimit/index.ts 57.50% <33.33%> (-0.40%) ⬇️
packages/server/src/server.ts 74.29% <50.00%> (-2.59%) ⬇️
packages/server/src/koaJsonRpc/index.ts 63.23% <75.00%> (+1.69%) ⬆️
packages/relay/src/formatters.ts

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.

Nana-EC
Nana-EC previously approved these changes Feb 15, 2023
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG
DCO also

packages/server/src/formatters.ts Outdated Show resolved Hide resolved
@@ -134,11 +135,48 @@ app.getKoaApp().use(async (ctx, next) => {
}
});

app.getKoaApp().use(async (ctx, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's look to rework this method next sprint.
Could see readability and testability improvements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Nana-EC Nana-EC added enhancement New feature or request P1 process Build, test and deployment-process related tasks labels Feb 15, 2023
@Nana-EC Nana-EC added this to the 0.18.0 milestone Feb 15, 2023
@ebadiere ebadiere merged commit c321f65 into main Feb 15, 2023
@ebadiere ebadiere deleted the 865-update-logs-and-metrics branch February 15, 2023 15:57
Nana-EC pushed a commit that referenced this pull request Feb 15, 2023
…t. (#898)

* 865-update-logs-and-metrics.  Added the requestId to the server context.

Signed-off-by: ebadiere <[email protected]>

* Added missing formatters.ts file.

Signed-off-by: ebadiere <[email protected]>

* Updated year in copyright.

Signed-off-by: ebadiere <[email protected]>

---------

Signed-off-by: ebadiere <[email protected]>
Nana-EC pushed a commit that referenced this pull request Feb 15, 2023
…t. (#898)

* 865-update-logs-and-metrics.  Added the requestId to the server context.

Signed-off-by: ebadiere <[email protected]>

* Added missing formatters.ts file.

Signed-off-by: ebadiere <[email protected]>

* Updated year in copyright.

Signed-off-by: ebadiere <[email protected]>

---------

Signed-off-by: ebadiere <[email protected]>
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Nana-EC added a commit that referenced this pull request Feb 15, 2023
Cherry pick PRs that went into main branch since cutting a 0.18.0-alpha1. PRs map to individual commits
- [PR 882 - eth_feeHistory invalid cached response](#882)
- [PR 887 - Needed changes for metrics improvements](#887)
- [PR 883 - Handle GRPC timeout error](#883)
- PR 898 - update-logs-and-metrics Added the requestId to the server context](#898)

---------

Signed-off-by: ebadiere <[email protected]>
Signed-off-by: Nana Essilfie-Conduah <[email protected]>

---------

Signed-off-by: nikolay <[email protected]>
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Ivo Yankov <[email protected]>
Signed-off-by: ebadiere <[email protected]>
Co-authored-by: Nikolay Atanasow <[email protected]>
Co-authored-by: Alfredo Gutierrez <[email protected]>
Co-authored-by: Ivo Yankov <[email protected]>
Co-authored-by: Eric Badiere <[email protected]>
Nana-EC pushed a commit that referenced this pull request Feb 15, 2023
…t. (#898)

* 865-update-logs-and-metrics.  Added the requestId to the server context.

Signed-off-by: ebadiere <[email protected]>

* Added missing formatters.ts file.

Signed-off-by: ebadiere <[email protected]>

* Updated year in copyright.

Signed-off-by: ebadiere <[email protected]>

---------

Signed-off-by: ebadiere <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 process Build, test and deployment-process related tasks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants