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

fix(relayer): change reqwest for isahc in relayer blackbox tests (ENG-699) #1366

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

Fraser999
Copy link
Contributor

@Fraser999 Fraser999 commented Aug 14, 2024

Summary

This is a straight swap of isahc in place of reqwest in the relayer's blackbox tests. Other than a single timeout being increased, no logic was changed.

Background

Blackbox tests were failing on macOS due to the poor performance of the reqwest messages. The timeout for a GET was set to 100ms, and using reqwest I was seeing times around 120ms. By swapping to isahc, times dropped to 2ms.

Changes

  • Swapped isahc in place of reqwest in the realyer's blackbox tests.
  • Increased the timeout on a flaky test from 2s to 10s (the test occasionally takes just over 2s, less than 3s from several attempts locally).
  • Added [lints.rust] to the relayer's manifest to squash some warnings.

Testing

No new tests required, existing ones now pass on macOS.

Related Issues

Closes #1354.

@Fraser999 Fraser999 requested a review from a team as a code owner August 14, 2024 16:21
@Fraser999 Fraser999 requested a review from SuperFluffy August 14, 2024 16:21
@github-actions github-actions bot added the sequencer-relayer pertaining to the astria-sequencer-relayer crate label Aug 14, 2024
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

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

Interesting. Some years ago I observed that I'd get double digits millisecond latency on Linux compared to the macOS loopback device (which was in the low digit ms).

Do you know why isahc's latency is so much lower? Just experimenting?

tower = { version = "0.4.13" }
tokio-test.workspace = true
rand_chacha = "0.3.1"

[build-dependencies]
astria-build-info = { path = "../astria-build-info", features = ["build"] }

[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = [
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was getting a bunch of warnings when building on macOS, and this was the fix suggested as part of the warning text.

@@ -273,7 +273,7 @@ async fn should_filter_rollup() {
.await;
sequencer_relayer
.timeout_ms(
2_000,
10_000,
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it might just fix the tests itself (compared to swapping out reqwest for isahc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was more the calls to TestSequencerRelayer::get_from_relayer_api which were causing multiple tests to fail - specifically the 100ms timeout inside that method.

Even after fixing that (by using isahc) this test was occasionally taking just over 2 seconds on macOS. I didn't check if we also see that on Linux.

@Fraser999
Copy link
Contributor Author

@SuperFluffy

Do you know why isahc's latency is so much lower? Just experimenting?

I was recommended isahc a few months back by a friend who had found it to be much faster than reqwest. I believe the thinking is that reqwest burns a lot of cycles in setting up connection pools, but I haven't actually looked into that.

@Fraser999 Fraser999 added this pull request to the merge queue Aug 19, 2024
Merged via the queue into main with commit 49452f4 Aug 19, 2024
45 checks passed
@Fraser999 Fraser999 deleted the fraser/fix-relayer-tests branch August 19, 2024 11:51
steezeburger added a commit that referenced this pull request Aug 22, 2024
* main:
  refactor(core, proto)!: define app genesis state in proto (#1346)
  fix(sequencer): bump penumbra dep to fix ibc state access bug (#1389)
  feat(conductor)!: support disabled celestia auth (#1372)
  fix(sequencer)!: fix block fees (#1343)
  perf(sequencer): add benchmark for prepare_proposal (ENG-660) (#1337)
  fix(proto): fix import name mismatch (#1380)
  fix(ci): enable bridge withdrawer building with tag (#1374)
  feat(sequencer): rewrite memool to have per-account transaction storage and maintenance  (#1323)
  refactor(core, sequencer)!: require that bridge unlock address always be set (#1339)
  fix(sequencer)!: take funds from bridge in ics20 withdrawals (#1344)
  fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332)
  fix: abci error code (#1280)
  refactor(core): shorten `try_from_block_info_and_data()` (#1371)
  fix(relayer): change `reqwest` for `isahc` in relayer blackbox tests (ENG-699) (#1366)
  fix(conductor): update for celestia-node v0.15.0 (#1367)
  Chore: Upgrade celestia-node to v0.14.1 (#1360)
  chore(charts): fix charts production templates (#1359)
  chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencer-relayer pertaining to the astria-sequencer-relayer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix sequencer-relayer tests on macOS
2 participants