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

Remove useless whitespace in Websocket output #9720

Merged
merged 8 commits into from Nov 20, 2022
Merged

Remove useless whitespace in Websocket output #9720

merged 8 commits into from Nov 20, 2022

Conversation

adizere
Copy link
Contributor

@adizere adizere commented Nov 18, 2022

Closes #9696


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@adizere adizere requested a review from a team November 18, 2022 13:03
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Thanks for this @adizere! Could you please remove all prettified JSON RPC responses though, including the HTTP ones?

I think these are the only two remaining places:

jsonBytes, err := json.MarshalIndent(res, "", " ")

jsonBytes, err := json.MarshalIndent(v, "", " ")

@thanethomson thanethomson added S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.37.x Tell mergify to backport the PR to v0.37.x labels Nov 18, 2022
@adizere
Copy link
Contributor Author

adizere commented Nov 18, 2022

With the new branch, the output now looks as follows

For websocket output:

$ wscat -c ws://127.0.0.1:26657/websocket
Connected (press CTRL+C to quit)
> { "jsonrpc": "2.0", "method": "subscribe", "id": 0, "params": { "query": "tm.event='NewBlock'" } }
< {"jsonrpc":"2.0","id":0,"result":{}}
< {"jsonrpc":"2.0","id":0,"result":{"query":"tm.event='NewBlock'","data":{"type":"tendermint/event/NewBlock","value":{"block":{"header":{"version":{"block":"11","app":"1"},"chain_id":"test-chain-gjnVQx","height":"487","time":"2022-11-18T13:46:34.893604324Z","last_block_id":{"hash":"076C580AA40A471DE818940A0C78A0F4ADF69A92455AE60904366AD7F6CB0CDA","parts":{"total":1,"hash":"0FCF10C92D65D08471FED7EBC8697ACCAC2DE25A31FBD2CEB8565A2B514442A8"}},"last_commit_hash":"29F2B040E79C9CEBE7794FF800C9BEAC1F65D47963ED985C8361035EC76E820C","data_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","validators_hash":"C8B9A24D7717676A0BDDB1C17F9499D9A50E1D756420A791FD93948010629973","next_validators_hash":"C8B9A24D7717676A0BDDB1C17F9499D9A50E1D756420A791FD93948010629973","consensus_hash":"048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F","app_hash":"0000000000000000","last_results_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","evidence_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","proposer_address":"00F9A04401951975AB4445209672640EF0E187C8"},"data":{"txs":[]},"evidence":{"evidence":[]},"last_commit":{"height":"486","round":0,"block_id":{"hash":"076C580AA40A471DE818940A0C78A0F4ADF69A92455AE60904366AD7F6CB0CDA","parts":{"total":1,"hash":"0FCF10C92D65D08471FED7EBC8697ACCAC2DE25A31FBD2CEB8565A2B514442A8"}},"signatures":[{"block_id_flag":2,"validator_address":"00F9A04401951975AB4445209672640EF0E187C8","timestamp":"2022-11-18T13:46:34.893604324Z","signature":"nlPcmDO+rlXN8alPxXGLDxllRHBaH0US37NgCJCzR2Jj4zM25AHsMHN4bd/VSUzRBfiuRobC/rSe1A5zcEACBA=="}]}},"result_begin_block":{},"result_end_block":{"validator_updates":null}}},"events":{"tm.event":["NewBlock"]}}}
< {"jsonrpc":"2.0","id":0,"result":{"query":"tm.event='NewBlock'","data":{"type":"tendermint/event/NewBlock","value":{"block":{"header":{"version":{"block":"11","app":"1"},"chain_id":"test-chain-gjnVQx","height":"488","time":"2022-11-18T13:46:35.906382967Z","last_block_id":{"hash":"755DB59E9DFBF1F8A20948515DF4863D17FE46E51A1724A73994AB9A2B6F3CE1","parts":{"total":1,"hash":"CCFC30A2BEE2D0A2AF4CB397FB5BBDFC1FA69C3342509769CAB645627E6C4DA6"}},"last_commit_hash":"F705495755751E3E55BE40CA9E71BF26485AFB4004D07B21C20AE78D4ECA7973","data_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","validators_hash":"C8B9A24D7717676A0BDDB1C17F9499D9A50E1D756420A791FD93948010629973","next_validators_hash":"C8B9A24D7717676A0BDDB1C17F9499D9A50E1D756420A791FD93948010629973","consensus_hash":"048091BC7DDC283F77BFBF91D73C44DA58C3DF8A9CBC867405D8B7F3DAADA22F","app_hash":"0000000000000000","last_results_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","evidence_hash":"E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855","proposer_address":"00F9A04401951975AB4445209672640EF0E187C8"},"data":{"txs":[]},"evidence":{"evidence":[]},"last_commit":{"height":"487","round":0,"block_id":{"hash":"755DB59E9DFBF1F8A20948515DF4863D17FE46E51A1724A73994AB9A2B6F3CE1","parts":{"total":1,"hash":"CCFC30A2BEE2D0A2AF4CB397FB5BBDFC1FA69C3342509769CAB645627E6C4DA6"}},"signatures":[{"block_id_flag":2,"validator_address":"00F9A04401951975AB4445209672640EF0E187C8","timestamp":"2022-11-18T13:46:35.906382967Z","signature":"NNKamS/P1dnRpBeLAgq/A7zHaCnuZvLxk3VASWFnTxF8usWVMFEO3ik43YHnmk8EFTVAi9mLY+tanmsRL+E9AQ=="}]}},"result_begin_block":{},"result_end_block":{"validator_updates":null}}},"events":{"tm.event":["NewBlock"]}}}

It used to be:

> { "jsonrpc": "2.0", "method": "subscribe", "id": 0, "params": { "query": "tm.event='NewBlock'" } }
< {
  "jsonrpc": "2.0",
  "id": 0,
  "result": {}
}
< {
  "jsonrpc": "2.0",
  "id": 0,
  "result": {
    "query": "tm.event='NewBlock'",
    "data": {
      "type": "tendermint/event/NewBlock",
      "value": {
        "block": {
          "header": {
            "version": {
              "block": "11",
              "app": "1"
....

For RPC output:

It is now:

$ curl http://localhost:26657/status
{"jsonrpc":"2.0","id":-1,"result":{"node_info":{"protocol_version":{"p2p":"8","block":"11","app":"1"},"id":"0c47498ed3cfe332e60e6dd07597509064e92a82","listen_addr":"tcp://0.0.0.0:26656","network":"test-chain-gjnVQx","version":"0.38.0-dev","channels":"40202122233038606100","moniker":"hermes-bastion-tor1","other":{"tx_index":"on","rpc_address":"tcp://127.0.0.1:26657"}},"sync_info":{"latest_block_hash":"1A17BF7FABCDA1E0A1D3C7FC8F8854A831A53C718ABBDBCB8F2FAA478AB3F0A3","latest_app_hash":"0000000000000000","latest_block_height":"467","latest_block_time":"2022-11-18T13:46:14.581563537Z","earliest_block_hash":"32619EBAA20611BB1D0D5E731E582A62AA7DD5027F831D50E98C819189531DB8","earliest_app_hash":"","earliest_block_height":"1","earliest_block_time":"2022-11-18T13:32:21.415141857Z","catching_up":false},"validator_info":{"address":"00F9A04401951975AB4445209672640EF0E187C8","pub_key":{"type":"tendermint/PubKeyEd25519","value":"WMRogr4A8f7gkL+rGG9GVP55MycgsiKp25rsl7YlTd0="},"voting_power":"10"}}}

It used to be:

$ curl http://localhost:26657/status
{
  "jsonrpc": "2.0",
  "id": -1,
  "result": {
    "node_info": {
      "protocol_version": {
        "p2p": "8",
        "block": "11",
        "app": "1"
...

@adizere
Copy link
Contributor Author

adizere commented Nov 18, 2022

Something to consider: could this change be breaking for some users?

I imagine some users of the binary might have scripts doing $ curl http://localhost:26657/status | fgrep 'catching_up' (or similar). If such a script parses & expects a "catching_up": false or "catching_up": true on a single line, this change will be breaking.

Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I see there is one test that's failing that looks like it's due to these changes: https://github.com/tendermint/tendermint/actions/runs/3497400805/jobs/5856450702

It looks like the tests are matching on raw HTTP responses in https://github.com/tendermint/tendermint/blob/main/rpc/jsonrpc/server/http_server_test.go

Adapted the assertions in
- TestWriteRPCResponseHTTP
- TestWriteRPCResponseHTTPError
to work with non-pretty JSON-RPC output
@adizere
Copy link
Contributor Author

adizere commented Nov 19, 2022

Thanks Thane! Tests seem to pass now.

It was not clear if the changelog pending entry should go into 0.37 or 0.38 section of CHANGELOG_PENDING.md. I assumed it should be 0.38 -- 90bee8d glad to fix it if that's not right.

@adizere adizere self-assigned this Nov 19, 2022
Copy link

@liangping liangping left a comment

Choose a reason for hiding this comment

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

Yeah. it's a great improvement.

@thanethomson
Copy link
Contributor

It was not clear if the changelog pending entry should go into 0.37 or 0.38 section of CHANGELOG_PENDING.md. I assumed it should be 0.38 -- 90bee8d glad to fix it if that's not right.

In this case, since we're backporting it and it'll land up in v0.34 and v0.37.1, it probably makes sense to add it to those release branches' changelogs. Users can safely assume that in v0.38 it'll be present too, and so it wouldn't really be necessary to add it to the v0.38 changelog.

I'll make the necessary changes.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Thanks for this @adizere!

@thanethomson thanethomson merged commit 20ffa4f into main Nov 20, 2022
@thanethomson thanethomson deleted the adi/9696 branch November 20, 2022 14:39
mergify bot pushed a commit that referenced this pull request Nov 20, 2022
* First try at #9696

* Brief explanation

* Removed all prettified JSON RPC responses

* Fixes for failing tests.

Adapted the assertions in
- TestWriteRPCResponseHTTP
- TestWriteRPCResponseHTTPError
to work with non-pretty JSON-RPC output

* Added changelog pending entry

* Update CHANGELOG_PENDING.md

Co-authored-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit 20ffa4f)

# Conflicts:
#	CHANGELOG_PENDING.md
#	rpc/jsonrpc/server/http_server_test.go
thanethomson added a commit that referenced this pull request Nov 20, 2022
* First try at #9696

* Brief explanation

* Removed all prettified JSON RPC responses

* Fixes for failing tests.

Adapted the assertions in
- TestWriteRPCResponseHTTP
- TestWriteRPCResponseHTTPError
to work with non-pretty JSON-RPC output

* Added changelog pending entry

* Update CHANGELOG_PENDING.md

Co-authored-by: Thane Thomson <connect@thanethomson.com>
adizere added a commit that referenced this pull request Nov 21, 2022
* Remove useless whitespace in Websocket output (#9720)

* First try at #9696

* Brief explanation

* Removed all prettified JSON RPC responses

* Fixes for failing tests.

Adapted the assertions in
- TestWriteRPCResponseHTTP
- TestWriteRPCResponseHTTPError
to work with non-pretty JSON-RPC output

* Added changelog pending entry

* Update CHANGELOG_PENDING.md

Co-authored-by: Thane Thomson <connect@thanethomson.com>

* Add pending changelog and upgrading entries

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Adi Seredinschi <a@seredinschi.net>
adrianbrink pushed a commit to heliaxdev/tendermint that referenced this pull request May 23, 2023
…) (#221)

* Remove useless whitespace in Websocket output (tendermint#9720)

* First try at tendermint#9696

* Brief explanation

* Removed all prettified JSON RPC responses

* Fixes for failing tests.

Adapted the assertions in
- TestWriteRPCResponseHTTP
- TestWriteRPCResponseHTTPError
to work with non-pretty JSON-RPC output

* Added changelog pending entry

* Update CHANGELOG_PENDING.md

Co-authored-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit 20ffa4f)

# Conflicts:
#	CHANGELOG_PENDING.md
#	rpc/jsonrpc/server/http_server_test.go

* Fixed merging conflict introduced by mergify

---------

Co-authored-by: Adi Seredinschi <a@seredinschi.net>
Co-authored-by: Lasaro Camargos <lasaro@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.37.x Tell mergify to backport the PR to v0.37.x
Projects
Status: Done/Merged
Development

Successfully merging this pull request may close these issues.

Reducing size of data packet of web socket
3 participants