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

Upgrade urfave dependency which now supports DisableSliceFlagSeparato… #10950

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

fridrik01
Copy link
Contributor

Context
In #10761 we added a new RPC benchmarking tool which parses options from terminal to build up RPC requests. However, our current urfave version does not support parsing cli options if they have a comma (,) in them. This was however added by urfave a few months ago by introducing a new flag DisableSliceFlagSeparator.

Unfortunately, upgrading the urfave dependency introduced regressions in how it generated --help output, especially for commands with subcategories, where the subcategory was not included in the help output.

NOTE that are have been some changes still in how urfave generates --help output from our previous version (2.16.3) but these look ok to me

Test plan
Observed that lotus-bench rpc now works when passing it options with comma:

lotus-bench rpc --duration=3s --method='eth_getTransactionCount:10:2000:["0xd4c70007F3F502f212c7e6794b94C06F36173B36", "latest"]
[eth_getTransactionCount]:
- Options:
  - concurrency: 10
  - params: ["0xd4c70007F3F502f212c7e6794b94C06F36173B36", "latest"]
  - qps: 2000
- Total Requests: 5987
- Total Duration: 3004ms
- Requests/sec: 1992.487708
- Avg latency: 0ms
- Median latency: 0ms
- Latency distribution:
    10.00% in 0ms
    50.00% in 0ms
    90.00% in 0ms
    95.00% in 0ms
    99.00% in 0ms
    99.90% in 11ms
- Histogram:
     0-1ms|  5958|################################################################################################### (99.52%)
     1-2ms|     3| (0.05%)
     2-3ms|     3| (0.05%)
     3-4ms|     5| (0.08%)
     4-5ms|     5| (0.08%)
     5-6ms|     3| (0.05%)
     6-7ms|     0| (0.00%)
     7-8ms|     0| (0.00%)
     8-9ms|     1| (0.02%)
    9-13ms|     9| (0.15%)
- Status codes:
    [200]: 5987
- Errors (top 10):
    [nil]: 5987

@fridrik01 fridrik01 marked this pull request as ready for review June 3, 2023 17:47
@fridrik01 fridrik01 requested a review from a team as a code owner June 3, 2023 17:47
@ZenGround0
Copy link
Contributor

Looks like this needs go mod tidy

Taking a look now.

Nice work pushing the fix through the dependency.

Name: "lotus-bench",
Usage: "Benchmark performance of lotus on your hardware",
Version: build.UserVersion(),
DisableSliceFlagSeparator: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but is it possible to restrict this only to the rpcCmd? As I understand it we only want this to apply to that one subcommand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This option is only available on the cli app and not for individual subcommands.

@fridrik01 fridrik01 merged commit 594c52b into master Jun 7, 2023
@fridrik01 fridrik01 deleted the upgrade-urface-dependency branch June 7, 2023 21:31
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.

3 participants