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 to poem 3.x.x and poem-openapi 5.x.x #13929

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Upgrade to poem 3.x.x and poem-openapi 5.x.x #13929

merged 1 commit into from
Jul 12, 2024

Conversation

banool
Copy link
Contributor

@banool banool commented Jul 8, 2024

Description

In this PR I upgrade us from poem 1.x.x and poem-openapi 2.x.x to poem 3.x.x and poem-openapi 5.x.x. Besides it being good in general to stay up to date, we want to upgrade specifically to get the latest opentelemetry features so we can benefit from distributed tracing throughout the API stack. There have been other instances where being pinned to a specific version caused us issues.

The new Poem traits had issues with Box<UserTransaction> in transaction, so I just unboxed it. I can't see why it needs to be in a Box and the original commit (3ecd8e9) doesn't explain it. Perhaps there used to be some recursive type that we had to break.

Besides that, fortunately there wasn't much to change with our types or our custom derive macros.

By default this changes the spec slightly, primarily the inclusion of charset=utf-8 in the content type. I recall this caused issues for code generators, and it'd require changes to our Accept logic, so I remove that again. It also seems to change some of the enum variants. I don't believe we make use of the OpenAPI spec anywhere anymore so it shouldn't affect us, but we should reach out to anyone who might be affected (perhaps an old Go SDK, if it's still in development).

I will wait for poem-web/poem#829 to be released and then upgrade the poem deps again before releasing this PR.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

For now just CI, let's see how that goes first.

I run a localnet and connected the explorer to it and clicked around and everything seemed to work fine, no errors from the network tab. The spec explorer (at /v1/spec) works as before.

Key Areas to Review

  • Ensure Box<UserTransaction> -> UserTransaction is valid.
  • Make sure the modified OpenAPI spec is okay.

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Jul 8, 2024

⏱️ 10h 40m total CI duration on this PR
Job Cumulative Duration Recent Runs
test-fuzzers 5h 39m 🟩🟩🟩🟩🟩 (+5 more)
execution-performance / single-node-performance 1h 16m 🟩🟩🟩
rust-images / rust-all 19m 🟩
rust-targeted-unit-tests 17m 🟩
execution-performance / test-target-determinator 15m 🟩🟩🟩
rust-move-tests 15m 🟩
rust-move-tests 15m 🟩
forge-framework-upgrade-test / forge 15m 🟩
rust-move-tests 14m 🟩
forge-e2e-test / forge 14m 🟩
general-lints 14m 🟩🟩🟩🟩 (+4 more)
forge-compat-test / forge 13m 🟩
test-target-determinator 11m 🟩
rust-move-tests 10m 🟩
rust-cargo-deny 10m 🟩🟩🟩🟩 (+2 more)
rust-move-tests 9m 🟩
check-dynamic-deps 9m 🟩🟩🟩🟩🟩 (+5 more)
rust-move-tests 8m 🟩
check 4m 🟥🟥🟩
semgrep/ci 4m 🟩🟩🟩🟩🟩 (+5 more)
rust-move-tests 3m
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+5 more)
file_change_determinator 2m 🟩🟩🟩🟩🟩 (+5 more)
rust-move-tests 50s
permission-check 35s 🟩🟩🟩🟩🟩 (+5 more)
permission-check 34s 🟩🟩🟩🟩🟩 (+5 more)
file_change_determinator 30s 🟩🟩🟩
permission-check 29s 🟩🟩🟩🟩🟩 (+5 more)
permission-check 27s 🟩🟩🟩🟩🟩 (+5 more)
rust-move-tests 21s
Backport PR 9s 🟥🟥
permission-check 8s 🟩🟩🟩
determine-docker-build-metadata 6s 🟩🟩🟩
rust-move-tests 1s

🚨 2 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
test-target-determinator 8m 5m +45%
execution-performance / test-target-determinator 3m 5m -39%

settingsfeedbackdocs ⋅ learn more about trunk.io

@banool banool requested a review from geekflyer July 8, 2024 11:57
@banool banool marked this pull request as ready for review July 8, 2024 13:56
@banool banool requested review from bchocho and JoshLind July 8, 2024 15:34
@banool
Copy link
Contributor Author

banool commented Jul 11, 2024

Applying the do not merge label until poem-web/poem#829 is released.

@banool
Copy link
Contributor Author

banool commented Jul 12, 2024

Okay the maintainer seems to be away, we'll just use a git dep for now.

@banool banool force-pushed the banool/poem-3 branch 2 times, most recently from a450b92 to 94fb74f Compare July 12, 2024 11:41
@banool
Copy link
Contributor Author

banool commented Jul 12, 2024

I confirmed that a localnet and the spec explorer work still, let's send it.

@banool banool enabled auto-merge (squash) July 12, 2024 12:29

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1 (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 9617.873948282773 txn/s, latency: 3474.6155406576895 ms, (p50: 2600 ms, p90: 7400 ms, p99: 9900 ms), latency samples: 346060
2. Upgrading first Validator to new version: 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6467.567192238285 txn/s, latency: 4243.1129698338445 ms, (p50: 4200 ms, p90: 4800 ms, p99: 5000 ms), latency samples: 123980
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6366.96636366016 txn/s, latency: 4667.828166982609 ms, (p50: 4600 ms, p90: 5500 ms, p99: 6100 ms), latency samples: 242660
3. Upgrading rest of first batch to new version: 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7226.565176366638 txn/s, latency: 3705.9516080617495 ms, (p50: 4200 ms, p90: 4400 ms, p99: 4500 ms), latency samples: 139920
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6526.122018304122 txn/s, latency: 4691.973381896346 ms, (p50: 4500 ms, p90: 6500 ms, p99: 7800 ms), latency samples: 251220
4. upgrading second batch to new version: 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10579.722587239987 txn/s, latency: 2580.7072967374415 ms, (p50: 2500 ms, p90: 3600 ms, p99: 4300 ms), latency samples: 193100
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10069.08791904476 txn/s, latency: 3144.7216489079883 ms, (p50: 3000 ms, p90: 4400 ms, p99: 6900 ms), latency samples: 339740
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1

two traffics test: inner traffic : committed: 9360.223321148793 txn/s, latency: 4254.763674474016 ms, (p50: 4200 ms, p90: 4800 ms, p99: 10800 ms), latency samples: 3559040
two traffics test : committed: 99.96569644881546 txn/s, latency: 2292.3322580645163 ms, (p50: 2100 ms, p90: 2600 ms, p99: 8400 ms), latency samples: 1860
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.239, avg: 0.219", "QsPosToProposal: max: 1.977, avg: 1.817", "ConsensusProposalToOrdered: max: 0.317, avg: 0.292", "ConsensusOrderedToCommit: max: 0.411, avg: 0.394", "ConsensusProposalToCommit: max: 0.702, avg: 0.687"]
Max round gap was 1 [limit 4] at version 1705061. Max no progress secs was 5.864486 [limit 15] at version 1705061.
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1 (PR)
Upgrade the nodes to version: 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1183.0222938196084 txn/s, submitted: 1185.1239915385931 txn/s, failed submission: 2.1016977189846973 txn/s, expired: 2.1016977189846973 txn/s, latency: 2661.717696407422 ms, (p50: 2100 ms, p90: 4500 ms, p99: 10500 ms), latency samples: 101320
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1045.0925773161396 txn/s, submitted: 1047.9242162292514 txn/s, failed submission: 2.831638913111675 txn/s, expired: 2.831638913111675 txn/s, latency: 2765.953522300959 ms, (p50: 2100 ms, p90: 4800 ms, p99: 9000 ms), latency samples: 95960
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1 passed
Upgrade the remaining nodes to version: 0b4bb6cc3eb4b9a2986d240f249b963199a2f7f1
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1219.8503134781117 txn/s, submitted: 1221.9705631539139 txn/s, failed submission: 2.1202496758020484 txn/s, expired: 2.1202496758020484 txn/s, latency: 2772.0565372730784 ms, (p50: 2000 ms, p90: 5200 ms, p99: 11100 ms), latency samples: 103560
Test Ok

@banool banool merged commit 26c0a51 into main Jul 12, 2024
42 of 45 checks passed
@banool banool deleted the banool/poem-3 branch July 12, 2024 13:37
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