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

Serve some basic config info at /info. #14842

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Serve some basic config info at /info. #14842

merged 1 commit into from
Oct 14, 2024

Conversation

grao1991
Copy link
Contributor

@grao1991 grao1991 commented Oct 2, 2024

Description

As discussed, serve some basic config under /v1/info. We can add more later.

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

Copy link

trunk-io bot commented Oct 2, 2024

⏱️ 4h 45m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 2h 30m 🟩🟩🟩🟥🟩 (+2 more)
execution-performance / test-target-determinator 25m 🟩🟩🟩🟩🟩 (+1 more)
test-target-determinator 19m 🟩🟩🟩 (+1 more)
check 18m 🟩🟩🟩 (+1 more)
rust-cargo-deny 11m 🟩🟩🟩 (+2 more)
check-dynamic-deps 9m 🟩🟩🟩🟩🟩 (+2 more)
fetch-last-released-docker-image-tag 9m 🟩🟩🟩🟩 (+1 more)
rust-doc-tests 5m 🟩
rust-doc-tests 5m 🟩
rust-doc-tests 5m 🟩
rust-doc-tests 5m 🟩
general-lints 3m 🟩🟩🟩🟩🟩 (+2 more)
semgrep/ci 3m 🟩🟩🟩🟩🟩 (+2 more)
rust-move-tests 2m 🟩
rust-move-tests 2m

settingsfeedbackdocs ⋅ learn more about trunk.io

@grao1991 grao1991 enabled auto-merge (squash) October 2, 2024 05:48
@grao1991 grao1991 requested a review from sionescu October 2, 2024 05:48
@grao1991 grao1991 force-pushed the grao_info branch 3 times, most recently from 253bae3 to f62bf1d Compare October 2, 2024 05:59
api/src/basic.rs Outdated
self.context.node_config.indexer_db_config
));

Html(rows.join("\n"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to send outputs as a map? It is JSON

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 did whatever chatgpt told me to do.

Changed to use map now.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Please generate the new OpenAPI spec. Cc @banool

api/src/basic.rs Outdated
operation_id = "info",
tag = "ApiTags::General"
)]
async fn info(&self) -> Json<HashMap<String, String>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be a breaking change if later we change this to a nested structure?
(How does it look like in the API spec?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generated the spec, PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it just says "json in text" (so we are good)?
But can an expert confirm? 😂 @banool ?

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

format!("{:?}", self.context.node_config.indexer_db_config),
);

Json(info)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is intended only for human consumption, hence the keys like "Internal Indexer Config" rather than internal_indexer_config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's what I'm expecting. Changed to snake_case anyway.

api/src/basic.rs Outdated
operation_id = "info",
tag = "ApiTags::General"
)]
async fn info(&self) -> Json<HashMap<String, String>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This output will be more standard and parseable if you just use JSON values. How about you instead use:

Json<HashMap<String, serde_json::Value>>

as the output?

As it is now you're using the debug representation inside a string, so that will be difficult to parse. Like it'll look sort of like JSON but not quite.

So instead of:

            format!(
                "{:?}",
                self.context
                    .node_config
                    .state_sync
                    .state_sync_driver
                    .bootstrapping_mode
            ),

Do this:


                serde_json::Value::from(&self.context
                    .node_config
                    .state_sync
                    .state_sync_driver
                    .bootstrapping_mode)

Copy link
Contributor

@banool banool Oct 9, 2024

Choose a reason for hiding this comment

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

Even better, you could also just return Json<HashMap<String, T>> where T: Serialize and then poem will do the serialization for you I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@banool banool left a comment

Choose a reason for hiding this comment

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 7eeba4cd15892717741a614add1afde004c7855f ==> 163fa1e602797f48b22ddac9e4980784a6cc8726

Compatibility test results for 7eeba4cd15892717741a614add1afde004c7855f ==> 163fa1e602797f48b22ddac9e4980784a6cc8726 (PR)
1. Check liveness of validators at old version: 7eeba4cd15892717741a614add1afde004c7855f
compatibility::simple-validator-upgrade::liveness-check : committed: 13910.86 txn/s, latency: 2471.04 ms, (p50: 2100 ms, p70: 2200, p90: 2600 ms, p99: 7600 ms), latency samples: 480220
2. Upgrading first Validator to new version: 163fa1e602797f48b22ddac9e4980784a6cc8726
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7693.93 txn/s, latency: 3733.58 ms, (p50: 4100 ms, p70: 4300, p90: 4500 ms, p99: 4600 ms), latency samples: 143140
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6849.69 txn/s, latency: 4681.56 ms, (p50: 4700 ms, p70: 4800, p90: 6900 ms, p99: 7000 ms), latency samples: 230420
3. Upgrading rest of first batch to new version: 163fa1e602797f48b22ddac9e4980784a6cc8726
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6392.09 txn/s, latency: 4409.31 ms, (p50: 5100 ms, p70: 5400, p90: 5500 ms, p99: 5600 ms), latency samples: 117220
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6006.38 txn/s, latency: 5315.11 ms, (p50: 5400 ms, p70: 5600, p90: 7200 ms, p99: 7400 ms), latency samples: 198600
4. upgrading second batch to new version: 163fa1e602797f48b22ddac9e4980784a6cc8726
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10470.50 txn/s, latency: 2622.98 ms, (p50: 2700 ms, p70: 2900, p90: 3100 ms, p99: 4300 ms), latency samples: 184640
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11552.70 txn/s, latency: 2739.02 ms, (p50: 2700 ms, p70: 2800, p90: 3000 ms, p99: 3600 ms), latency samples: 375840
5. check swarm health
Compatibility test for 7eeba4cd15892717741a614add1afde004c7855f ==> 163fa1e602797f48b22ddac9e4980784a6cc8726 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 163fa1e602797f48b22ddac9e4980784a6cc8726

two traffics test: inner traffic : committed: 13696.45 txn/s, latency: 2902.82 ms, (p50: 2700 ms, p70: 3000, p90: 3000 ms, p99: 3600 ms), latency samples: 5207700
two traffics test : committed: 100.01 txn/s, latency: 2592.80 ms, (p50: 2500 ms, p70: 2600, p90: 2800 ms, p99: 8700 ms), latency samples: 1880
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.237, avg: 0.220", "QsPosToProposal: max: 0.354, avg: 0.265", "ConsensusProposalToOrdered: max: 0.315, avg: 0.300", "ConsensusOrderedToCommit: max: 0.467, avg: 0.444", "ConsensusProposalToCommit: max: 0.765, avg: 0.744"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.96s no progress at version 2123702 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.32s no progress at version 2123700 (avg 8.32s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 7eeba4cd15892717741a614add1afde004c7855f ==> 163fa1e602797f48b22ddac9e4980784a6cc8726

Compatibility test results for 7eeba4cd15892717741a614add1afde004c7855f ==> 163fa1e602797f48b22ddac9e4980784a6cc8726 (PR)
Upgrade the nodes to version: 163fa1e602797f48b22ddac9e4980784a6cc8726
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1208.21 txn/s, submitted: 1211.22 txn/s, failed submission: 3.00 txn/s, expired: 3.00 txn/s, latency: 2624.99 ms, (p50: 2300 ms, p70: 2700, p90: 4700 ms, p99: 6700 ms), latency samples: 104560
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1010.28 txn/s, submitted: 1012.49 txn/s, failed submission: 2.21 txn/s, expired: 2.21 txn/s, latency: 2877.89 ms, (p50: 2400 ms, p70: 3000, p90: 5600 ms, p99: 8100 ms), latency samples: 91520
5. check swarm health
Compatibility test for 7eeba4cd15892717741a614add1afde004c7855f ==> 163fa1e602797f48b22ddac9e4980784a6cc8726 passed
Upgrade the remaining nodes to version: 163fa1e602797f48b22ddac9e4980784a6cc8726
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1193.48 txn/s, submitted: 1195.93 txn/s, failed submission: 2.45 txn/s, expired: 2.45 txn/s, latency: 2508.17 ms, (p50: 2400 ms, p70: 2700, p90: 3900 ms, p99: 5900 ms), latency samples: 107220
Test Ok

@grao1991 grao1991 merged commit 2a0e7d6 into main Oct 14, 2024
45 of 48 checks passed
@grao1991 grao1991 deleted the grao_info branch October 14, 2024 19:26
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.

5 participants