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.

Sign up for GitHub

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

Jump to bottom

[API] Log responses. #15424

Draft
wants to merge 1 commit into
base: aptos-release-v1.24
Choose a base branch
from
Draft

[API] Log responses. #15424

wants to merge 1 commit into from
+61 −15

Conversation

JoshLind
Copy link
Contributor

Description

How Has This Been Tested?

Key Areas to Review

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
  • Move Compiler
  • Other (specify)

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

@trunk-io Trunk.io
Copy link

trunk-io bot commented Nov 28, 2024 •
edited
Loading

⏱️ 4h 11m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 1h 9m 🟥 🟥 🟥 🟩 🟩 
rust-unit-tests 24m 🟩 
rust-unit-tests 23m 🟩 
execution-performance / test-target-determinator 21m 🟩 🟩 🟩 🟩 🟩 
test-target-determinator 15m 🟩 ⬜ ⬜ ⬜ 🟩 
check 14m 🟩 ⬜ ⬜ ⬜ 🟩 
check-dynamic-deps 12m 🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests 10m 🟩 
rust-move-tests 10m 🟩 
rust-cargo-deny 9m 🟩 🟩 ⬜ ⬜ 🟩 
fetch-last-released-docker-image-tag 8m 🟩 🟩 🟩 ⬜ 🟩 
rust-doc-tests 6m 🟩 
rust-doc-tests 5m 🟩 
semgrep/ci 3m 🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests 2m ⬜ 

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@JoshLind JoshLind added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Nov 28, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@trunk-io Trunk.io
Copy link

trunk-io bot commented Nov 28, 2024 •
edited
Loading

Static Badge   Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@JoshLind JoshLind force-pushed the api_debug_1_24 branch 3 times, most recently from b8f5112 to f5875f2 Compare November 28, 2024 03:25
graphite-app[bot]
graphite-app bot reviewed Nov 28, 2024
View reviewed changes
api/src/transactions.rs
@@ -29,6 +29,7 @@ use aptos_api_types::{
MAX_RECURSIVE_TYPES_ALLOWED, U64,
};
use aptos_crypto::{hash::CryptoHash, signing_message};
use aptos_logger::{info};
Copy link

Choose a reason for hiding this comment

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

Remove unnecessary curly braces around single import 'info'. Change 'use aptos_logger::{info};' to 'use aptos_logger::info;'

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 684b407c7679d1cfec2cce06be4f165ce4c33d64

two traffics test: inner traffic : committed: 13766.47 txn/s, latency: 2933.78 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 16900 ms), latency samples: 5234340
two traffics test : committed: 99.91 txn/s, latency: 2079.01 ms, (p50: 1400 ms, p70: 1500, p90: 1600 ms, p99: 17200 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.305, avg: 1.604", "ConsensusProposalToOrdered: max: 0.324, avg: 0.294", "ConsensusOrderedToCommit: max: 0.387, avg: 0.377", "ConsensusProposalToCommit: max: 0.680, avg: 0.671"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.01s no progress at version 1372247 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 15.82s no progress at version 5652733 (avg 14.22s) [limit 16].
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite compat success on c7fe4449949dccf8f7212cb03ce004f44e8f0002 ==> 684b407c7679d1cfec2cce06be4f165ce4c33d64

Compatibility test results for c7fe4449949dccf8f7212cb03ce004f44e8f0002 ==> 684b407c7679d1cfec2cce06be4f165ce4c33d64 (PR)
1. Check liveness of validators at old version: c7fe4449949dccf8f7212cb03ce004f44e8f0002
compatibility::simple-validator-upgrade::liveness-check : committed: 14223.70 txn/s, latency: 2047.12 ms, (p50: 1600 ms, p70: 1700, p90: 1900 ms, p99: 23800 ms), latency samples: 567140
2. Upgrading first Validator to new version: 684b407c7679d1cfec2cce06be4f165ce4c33d64
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6587.22 txn/s, latency: 4020.50 ms, (p50: 4500 ms, p70: 4800, p90: 4900 ms, p99: 5000 ms), latency samples: 131720
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6722.55 txn/s, latency: 4704.47 ms, (p50: 4900 ms, p70: 5100, p90: 6900 ms, p99: 7100 ms), latency samples: 221640
3. Upgrading rest of first batch to new version: 684b407c7679d1cfec2cce06be4f165ce4c33d64
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7930.72 txn/s, latency: 3664.62 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4400 ms), latency samples: 147800
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7922.89 txn/s, latency: 4039.87 ms, (p50: 4300 ms, p70: 4500, p90: 4800 ms, p99: 5300 ms), latency samples: 263220
4. upgrading second batch to new version: 684b407c7679d1cfec2cce06be4f165ce4c33d64
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11984.82 txn/s, latency: 2259.60 ms, (p50: 2200 ms, p70: 2500, p90: 3400 ms, p99: 3700 ms), latency samples: 206340
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11299.09 txn/s, latency: 2708.16 ms, (p50: 2400 ms, p70: 2700, p90: 5100 ms, p99: 6900 ms), latency samples: 370360
5. check swarm health
Compatibility test for c7fe4449949dccf8f7212cb03ce004f44e8f0002 ==> 684b407c7679d1cfec2cce06be4f165ce4c33d64 passed
Test Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@banool banool Awaiting requested review from banool banool will be requested when the pull request is marked ready for review banool is a code owner

@gregnazario gregnazario Awaiting requested review from gregnazario gregnazario will be requested when the pull request is marked ready for review gregnazario is a code owner

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan 0xmaayan will be requested when the pull request is marked ready for review 0xmaayan is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@JoshLind