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(rpc): Log unrecognized RPC requests #3860

Merged
merged 6 commits into from
Apr 22, 2022
Merged

Conversation

jvff
Copy link
Contributor

@jvff jvff commented Mar 13, 2022

Motivation

While preparing Zebra to support being a backend for lightwalletd, we should ensure we're not missing any RPCs sent by lightwalletd. One thing that can help is to trace the received RPCs and log errors if unrecognized RPC requests are received.

Closes #3855.

Solution

  • Enable tracing of received RPC requests by default, by making the default tracing filter allow trace level output from jsonrpc-core
  • Create a custom middleware for the RPC server that prints error messages if an unrecognized RPC request is received

Review

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

jvff added 3 commits March 13, 2022 03:28
Enable RPC call tracing by default. This is useful for development
purposes, and should probably be removed in the future.
A middleware that will print error messages if there are unrecognized
RPC requests.
Enable logging of unrecognized RPC requests.
@jvff jvff added C-enhancement Category: This is an improvement P-Medium ⚡ A-diagnostics Area: Diagnosing issues or monitoring performance labels Mar 13, 2022
@jvff jvff self-assigned this Mar 13, 2022
@jvff jvff requested a review from a team as a code owner March 13, 2022 03:32
@jvff jvff requested review from dconnolly and removed request for a team March 13, 2022 03:32
@jvff jvff changed the title Log unrecognized RPC requests feat(rpc): Log unrecognized RPC requests Mar 13, 2022
@codecov
Copy link

codecov bot commented Mar 13, 2022

Codecov Report

Merging #3860 (5e1fd4b) into main (53e2d2e) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head 5e1fd4b differs from pull request most recent head 105cc94. Consider uploading reports for the commit 105cc94 to get more accurate results

@@            Coverage Diff             @@
##             main    #3860      +/-   ##
==========================================
+ Coverage   78.74%   78.80%   +0.05%     
==========================================
  Files         295      295              
  Lines       33788    33788              
==========================================
+ Hits        26606    26626      +20     
+ Misses       7182     7162      -20     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think this change could be really bad for performance, let's try to limit the performance impact?

Also, logging lots of data can cause the acceptance tests to hang.

zebra-rpc/src/server/tracing_middleware.rs Outdated Show resolved Hide resolved
zebrad/src/application.rs Outdated Show resolved Hide resolved
zebra-rpc/src/server/tracing_middleware.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 added the lightwalletd any work associated with lightwalletd label Mar 18, 2022
@teor2345 teor2345 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Apr 4, 2022
jvff and others added 3 commits April 21, 2022 18:17
Instead of reporting it as an error.

Co-authored-by: teor <teor@riseup.net>
This might improve performance.
@jvff jvff requested a review from teor2345 April 21, 2022 21:54
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

We can also add "Received unrecognized RPC request" to the integration test failure strings, after we get all the RPCs implemented.

mergify bot added a commit that referenced this pull request Apr 21, 2022
@mergify mergify bot merged commit 903eabd into main Apr 22, 2022
@mergify mergify bot deleted the log-unrecognized-rpc-requests branch April 22, 2022 00:09
@mpguerra mpguerra mentioned this pull request Apr 11, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Diagnosing issues or monitoring performance C-enhancement Category: This is an improvement lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log unknown RPCs received by Zebra
2 participants