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

#7748 first changes #7797

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ssonthal
Copy link
Contributor

@ssonthal ssonthal commented Nov 23, 2024

Fixes #7748

Changes

  • logs table on startup for additional rpcs and their enabled protocols and modules.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

@ssonthal ssonthal marked this pull request as ready for review November 23, 2024 15:35
@obasekiosa
Copy link
Contributor

@ssonthal
logic looks good.

a few things to improve would be:

  1. removing the old logs that which additional rpc endpoints were activated(added)
  2. making the table cleaner -- spacing, header alingment, and column seperator alignments (+)

(1) is a bit straight forward, find the location of the logic that logs that info within the code and then figure out how much of it to remove.

to help with (2) -- if you look at the logs after a considerable amount of time syncing you'll notice there's a table that built for showing the sync stats. You can take a look at the code that built that and try adapting it.

@ssonthal
Copy link
Contributor Author

ssonthal commented Nov 27, 2024

@ssonthal logic looks good.

a few things to improve would be:

  1. removing the old logs that which additional rpc endpoints were activated(added)
  2. making the table cleaner -- spacing, header alingment, and column seperator alignments (+)

(1) is a bit straight forward, find the location of the logic that logs that info within the code and then figure out how much of it to remove.

to help with (2) -- if you look at the logs after a considerable amount of time syncing you'll notice there's a table that built for showing the sync stats. You can take a look at the code that built that and try adapting it.

Hey @obasekiosa, I couldn't find any table you mentioned in point (2). how long do I need to keep the server running to see the sync stats?

@ssonthal
Copy link
Contributor Author

@obasekiosa Regarding point 1, i can only see these logs related to additionalRpcUrls ->

Screenshot 2024-11-27 at 8 41 28 PM

They don't seem like a problem because they are logging for some unlikely cases, it seems. What do you think?

@obasekiosa
Copy link
Contributor

@obasekiosa Regarding point 1, i can only see these logs related to additionalRpcUrls ->

Screenshot 2024-11-27 at 8 41 28 PM

They don't seem like a problem because they are logging for some unlikely cases, it seems. What do you think?

oh no not these logs

so on start of the client you'd see a log that tells you all the additional rpc urls that are active.

@obasekiosa
Copy link
Contributor

@ssonthal logic looks good.
a few things to improve would be:

  1. removing the old logs that which additional rpc endpoints were activated(added)
  2. making the table cleaner -- spacing, header alingment, and column seperator alignments (+)

(1) is a bit straight forward, find the location of the logic that logs that info within the code and then figure out how much of it to remove.
to help with (2) -- if you look at the logs after a considerable amount of time syncing you'll notice there's a table that built for showing the sync stats. You can take a look at the code that built that and try adapting it.

Hey @obasekiosa, I couldn't find any table you mentioned in point (2). how long do I need to keep the server running to see the sync stats?

it takes a while after some blocks have been synced.
When you find the log that does that you'll notice.
there's a library within the code base that lets you draw tables easily. 😄

@obasekiosa
Copy link
Contributor

obasekiosa commented Nov 28, 2024

@ssonthal

I may be wrong but it seems you might be finding it hard to find those logs.

This an example of something similar to what they look like.
Should help with finding the code 😄

Nov 21 04:33:12 eth nethermind[515741]: 21 Nov 04:33:12 | ***** JSON RPC report *****
Nov 21 04:33:12 eth nethermind[515741]: -----------------------------------------------------------------------------------------------------------------------------------------------
Nov 21 04:33:12 eth nethermind[515741]: method                                  | successes |   avg (ms) |   max (ms) |    errors |   avg (ms) |   max (ms) | avg size B | total (kB) |
Nov 21 04:33:12 eth nethermind[515741]: -----------------------------------------------------------------------------------------------------------------------------------------------
Nov 21 04:33:12 eth nethermind[515741]: engine_exchangeCapabilities             |         1 |      5.754 |      5.754 |         0 |      0.000 |      0.000 |        414 |       0.40 |
Nov 21 04:33:12 eth nethermind[515741]: engine_forkchoiceUpdatedV3              |        27 |      3.282 |     20.278 |         0 |      0.000 |      0.000 |        197 |       5.19 |
Nov 21 04:33:12 eth nethermind[515741]: engine_getClientVersionV1               |         1 |     16.563 |     16.563 |         0 |      0.000 |      0.000 |        122 |       0.12 |
Nov 21 04:33:12 eth nethermind[515741]: engine_getPayloadBodiesByRangeV1        |        11 |     47.851 |     80.270 |         0 |      0.000 |      0.000 |    6580647 |   70690.54 |
Nov 21 04:33:12 eth nethermind[515741]: engine_newPayloadV3                     |        26 |     73.069 |    248.753 |         0 |      0.000 |      0.000 |        162 |       4.11 |
Nov 21 04:33:12 eth nethermind[515741]: eth_chainId                             |         3 |      1.718 |      4.174 |         0 |      0.000 |      0.000 |         39 |       0.11 |
Nov 21 04:33:12 eth nethermind[515741]: eth_getBlockByNumber                    |       898 |      1.856 |     10.137 |         0 |      0.000 |      0.000 |      15681 |   13751.88 |
Nov 21 04:33:12 eth nethermind[515741]: eth_getLogs                             |         2 |    162.104 |    309.276 |         0 |      0.000 |      0.000 |     422005 |     824.23 |
Nov 21 04:33:12 eth nethermind[515741]: eth_syncing                             |        17 |      1.387 |     20.526 |         0 |      0.000 |      0.000 |         39 |       0.65 |
Nov 21 04:33:12 eth nethermind[515741]: -----------------------------------------------------------------------------------------------------------------------------------------------
Nov 21 04:33:12 eth nethermind[515741]: TOTAL                                   |       986 |      4.621 |    309.276 |         0 |      0.000 |      0.000 |      88564 |   85277.24 |
Nov 21 04:33:12 eth nethermind[515741]: -----------------------------------------------------------------------------------------------------------------------------------------------
Nov 21 04:33:12 eth nethermind[515741]:  

@ssonthal
Copy link
Contributor Author

Screenshot 2024-11-28 at 8 36 38 PM

@ssonthal
Copy link
Contributor Author

hey @obasekiosa , i have removed the '+'s from the edges. and the made the pipe separators red to match the prior implementation.

@ssonthal
Copy link
Contributor Author

ssonthal commented Nov 28, 2024

Questions:

  1. currently the table is getting logged twice. Is that expected? or I should explore that?
  2. do you want a heading to the table as well? like the one you shared.

@obasekiosa
Copy link
Contributor

Questions:

  1. currently the table is getting logged twice. Is that expected? or I should explore that?
  2. do you want a heading to the table as well? like the one you shared.
  1. normally it shouldn't be logged twice.
    so yea explore that

  2. Yes a good heading would be nice.

Note: did you see the code that generates the reference table i sent?

@ssonthal
Copy link
Contributor Author

ssonthal commented Dec 2, 2024

Questions:

  1. currently the table is getting logged twice. Is that expected? or I should explore that?
  2. do you want a heading to the table as well? like the one you shared.
  1. normally it shouldn't be logged twice.
    so yea explore that
  2. Yes a good heading would be nice.

Note: did you see the code that generates the reference table i sent?

Yes, i checked the reference. It's also creating the table manually with same logic. So i thought I can go ahead with mine.

@ssonthal
Copy link
Contributor Author

ssonthal commented Dec 5, 2024

Hey @obasekiosa , what do you think?
Screenshot 2024-12-05 at 11 34 31 PM

@ssonthal
Copy link
Contributor Author

Hey @obasekiosa , need your input here.

@obasekiosa
Copy link
Contributor

Questions:

  1. currently the table is getting logged twice. Is that expected? or I should explore that?
  2. do you want a heading to the table as well? like the one you shared.
  1. normally it shouldn't be logged twice.
    so yea explore that
  2. Yes a good heading would be nice.

Note: did you see the code that generates the reference table i sent?

Yes, i checked the reference. It's also creating the table manually with same logic. So i thought I can go ahead with mine.

ahh seems i was mixing it up with the paprika repo. 😅

@obasekiosa
Copy link
Contributor

Hey @obasekiosa , need your input here.

i'd take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log additional info on the additional rpc urls.
2 participants