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

backend/ui: update server to accept string for duration as shown in proto file and render server output in ui #8

Closed
willpoint opened this issue Dec 13, 2022 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@willpoint
Copy link
Contributor

Currently, there's a mismatch, the proto definitions mandate the UI to send a string for durations in seconds while the server validations expect a different type. Perhaps server needs to be updated to allow seconds sent as string.

Also the UI needs to display the returned data from the server in place of the current dummy output.

Screenshot 2022-12-13 at 23 12 03

@willpoint willpoint added the bug Something isn't working label Dec 13, 2022
@willpoint
Copy link
Contributor Author

willpoint commented Dec 13, 2022

@kirbyquerby what other details are required for the server to produce the expected result, especially in conjunction with the Client Factory field.

I have used kvstore to run the test before seeing the mismatch between server's expectation and proto file. Is there anything else. For example, what values should be inputed in the endpoints field when running the local server.

@kirbyquerby
Copy link
Contributor

Ugh, the proto-to-swagger-to-typescript process seems ridiculously lossy. First we had the issue with turning enums into raw strings and now it's doing the same thing for Durations:

// The duration (in seconds) for which to handle the load test.
// Maps to --time in tm-load-test.
google.protobuf.Duration duration = 3;

"duration": {
"type": "string",
"description": "The duration (in seconds) for which to handle the load test.\nMaps to --time in tm-load-test."
},

I'm going to try setting up grpc-web, which will hopefully generate a more accurate client for the UI.


As for client factories, currently there's just kvstore and test-cosmos-client-factory. Users will implement and register their own custom client factories here:

// Add logic to register your custom client factories to this function.
func registerClientFactories() error {
cdc := codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
txConfig := authtx.NewTxConfig(cdc, authtx.DefaultSignModes)
cosmosClientFactory := myabciapp.NewCosmosClientFactory(txConfig)
if err := loadtest.RegisterClientFactory("test-cosmos-client-factory", cosmosClientFactory); err != nil {
return fmt.Errorf("failed to register client factory %s: %w", "test-cosmos-client-factory", err)
}
return nil
}


endpoints should be urls pointing at a tendermint server. For instance, if you run simapp locally, you could use ws://localhost:26657/websocket.


Here's an example request that I'm using locally with postman:

{
    "broadcast_tx_method": "BROADCAST_TX_METHOD_COMMIT",
    "endpoint_select_method": "ENDPOINT_SELECT_METHOD_SUPPLIED",
    "client_factory": "test-cosmos-client-factory",
    "connection_count": 10,
    "duration": {
        "seconds": 10
    },
    "endpoints": [
        "ws://localhost:26657/websocket"
    ],
    "max_endpoint_count": 0,
    "min_peer_connectivity_count": 1,
    "peer_connect_timeout": {
        "seconds": 2
    },
    "send_period": {
        "seconds": 2
    },
    "stats_output_file_path": "foo.txt",
    "transaction_count": 100,
    "transaction_size_bytes": 100,
    "transactions_per_second": 100
}

@kirbyquerby
Copy link
Contributor

Update on duration: in the proto3 JSON mapping documentation, it looks like durations are the number of seconds followed by the letter "s":
image

This still seems silly, though, so I'm still going to look into grpc-web.

kirbyquerby added a commit that referenced this issue Dec 14, 2022
This change adds a generated typescript grpc-web client ( https://github.com/grpc/grpc-web ) and adds support for grpc-web requests to the Go server.

Updates #8
@willpoint
Copy link
Contributor Author

willpoint commented Dec 14, 2022

@kirbyquerby in the interest of time to avoid rewriting parts using the existing generated code, I decided to update the patch fields that should have been of type Duration but were of string and turned them into objects - it didn't work but led to a syntax error.

I also tried passing the s suffix but returned syntax errors. The errors weren't helpful. So we should actually consider moving to the usage of grpc-web. I'm now going through samples of the usage.

I think we can merge this in and I'd send a follow up PR with grpc-web - my initial hesitation for the grpc-web on #10 was that the generated code was too verbose. But if it works - verbosity got nothing on us.

// currently we have:
import { Api } from 'src/gen/LoadTest.ts';

// but now it would be:
import { }  from 'src/gen/orijtech/cosmosloadtester/v1/Loadtest_serviceServiceClientPb.ts`

@willpoint
Copy link
Contributor Author

willpoint commented Dec 14, 2022

@kirbyquerby when using grpc-web - I'm encountering an blocking issue with the generated code at

var google_api_annotations_pb = require('../../../google/api/annotations_pb.js');

The error is that it fails to resolve the import because - ../../../google/api/annotations_pb.js wasn't in generated code - I'm not sure how to resolve this but it may involve making buf also generate this part of the google code into the gen folder.

ERROR in ./src/gen/orijtech/cosmosloadtester/v1/loadtest_service_pb.js 33:32-80
Module not found: Error: Can't resolve '../../../google/api/annotations_pb.js' in 'src/gen/orijtech/cosmosloadtester/v1'

kirbyquerby added a commit that referenced this issue Dec 14, 2022
With this change, I'm able to import ./gen/orijtech/cosmosloadtester/v1/loadtest_service_pb and build the UI successfully.

Updates #8
@kirbyquerby
Copy link
Contributor

@willpoint #11 should fix this by generating those files.

@willpoint
Copy link
Contributor Author

willpoint commented Dec 15, 2022

Mistaken git commit - 3bc4b27

Screenshot 2022-12-15 at 03 40 29

@kirbyquerby can you check what could be missing? The return values from the server currently show zero values. I used my local instance of a tendermint server - I also tried using simd but same result.

@kirbyquerby kirbyquerby reopened this Dec 15, 2022
kirbyquerby added a commit that referenced this issue Dec 15, 2022
changes include:
* exit on interrupt signal rather than blocking on gwServer.listenAndServe()
* use correct stats output file path
* validate config before running loadtest
* properly map endpoint select methods

Updates #8
kirbyquerby added a commit that referenced this issue Dec 15, 2022
This change adds the missing field for transaction count. It also populates the missing connection count value in the gRPC-web request (even though transaction count was the value missing a field?? Dynamically-typed languages truly horrify me). 

I also tweaked some (but not all) of the default values to be in line with the tm-load-test CLI's defaults.

Updates #8
kirbyquerby added a commit that referenced this issue Dec 15, 2022
This change adds the missing field for transaction count. It also populates the missing connection count value in the gRPC-web request (even though transaction count was the value missing a field?? Dynamically-typed languages truly horrify me). 

I also tweaked some (but not all) of the default values to be in line with the tm-load-test CLI's defaults.

Updates #8
kirbyquerby added a commit that referenced this issue Dec 15, 2022
changes include:
* exit on interrupt signal rather than blocking on gwServer.listenAndServe()
* use correct stats output file path
* validate config before running loadtest
* properly map endpoint select methods

Updates #8
@kirbyquerby
Copy link
Contributor

@willpoint looks like the UI was sending 0 for transaction count. The fix is in #12 . The server also wasn't doing very good input validation (and had some bugs!), so I've fixed that in #13 .

@kirbyquerby
Copy link
Contributor

Here's a screenshot of the UI working:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants