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

Analytics Indexer Operational Refactor #21486

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nickvikeras
Copy link

@nickvikeras nickvikeras commented Mar 13, 2025

Description

Changes

  • Refactor to support running multiple indexer tasks per process.
  • Use serde rather than clap to build configuration structs.
  • Move ShimIndexerProgressStore to sui-data-ingestion-core.

Why

  • Reduce operational complexity by reducing the number of processes we need to manage.
  • Reduce ingress by re-using checkpoints across tasks.

Next Steps

  • Define yaml files to configure pipelines.
    • I'll avoid merging until this is complete.
  • Migrate the running jobs to Kubernetes.
  • Test against testnet.

Open Questions

  • I introduced a task name. I noticed we have some metrics defined already. It seems like task name should be a metric dimension but I wasn't sure how to make that happen.
  • I made my best guess on which fields are task-specific and which are process specific. Let me know if anything seems wrong. It's also easy to change if we decide any of the fields are wrong later.
  • There is a limit to the number of checkpoints that can be worked on by a process concurrently. This means a slow pipeline will bottleneck the others. We don't exepct that to be an issue in the short term and will add metrics to monitor in the mid term. If we run into problems with this we can just split off a new process to run the slow pipeline.
  • I removed the support for clap args (to mimic the config structs in sui-data-ingestion). It would also be possible to support both clap args and yaml files, but I wasn't confident if the usefulness of that really warranted the extra complexity.

Test plan

  • I manually tested using the filesystem-based remote object store option. Everything seems to work fine.
  • Longer term a file-system-based test that we can easily run locally would be nice.

Ran

./target/debug/sui-analytics-indexer /Users/nick/test.yaml

test.yaml file contents:

rest_url: "https://fullnode-unpruned.mainnet.sui.io"
remote_store_config:
  object-store: File
  directory: "/tmp/object-store"
  bucket: "test"
remote_store_url: "https://storage.googleapis.com/mysten-mainnet-checkpoints"
tasks:
  - task_name: "transaction-csv"
    file_type: "Transaction"
    file_format: "CSV"
    checkpoint_interval: 10
    starting_checkpoint_seq_num: 1

  - task_name: "transaction-parquet"
    file_type: "Transaction"
    file_format: "PARQUET"
    checkpoint_interval: 10
    starting_checkpoint_seq_num: 1

Generated a bunch of files like:

/tmp/object-store
❯ tree | head -n 10
.
└── transactions
    ├── epoch_0
    │   ├── 1001_1011.csv
    │   ├── 1001_1011.parquet
    │   ├── 1011_1021.csv
    │   ├── 1011_1021.parquet
    │   ├── 101_111.csv
    │   ├── 101_111.parquet
    │   ├── 1021_1031.csv

The file contents look fine:

{"transaction_digest": "Asv7bLGEdpgXyA5BQJomHBuhAHnhtzAyyduS2QGTSkqN", "checkpoint": 2181, "epoch": 0, "timestamp_ms": 1681395238352, "sender": "0x0000000000000000000000000000000000000000000000000000000000000000", "transaction_kind": "ConsensusCommitPrologue", "is_system_txn": true, "is_sponsored_tx": false, "transaction_count": 0, "execution_success": true, "input": 1, "shared_input": 1, "gas_coins": 1, "created": 0, "mutated": 1, "deleted": 0, "transfers": 0, "split_coins": 0, "merge_coins": 0, "publish": 0, "upgrade": 0, "others": 0, "move_calls": 0, "packages": "", "gas_owner": "0x0000000000000000000000000000000000000000000000000000000000000000", "gas_object_id": "0x0000000000000000000000000000000000000000000000000000000000000000", "gas_object_sequence": 0, "gas_object_digest": "11111111111111111111111111111111", "gas_budget": 0, "total_gas_cost": 0, "computation_cost": 0, "storage_cost": 0, "storage_rebate": 0, "non_refundable_storage_fee": 0, "gas_price": 1, "raw_transaction": "AAMAAAAAAAAAALoTAAAAAAAA0EX3eocBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAA==", "has_zklogin_sig": false, "has_upgraded_multisig": false, "transaction_json": "{\"data\":[{\"intent_message\":{\"intent\":{\"scope\":0,\"version\":0,\"app_id\":0},\"value\":{\"V1\":{\"kind\":{\"ConsensusCommitPrologue\":{\"epoch\":0,\"round\":5050,\"commit_timestamp_ms\":1681395238352}},\"sender\":\"0x0000000000000000000000000000000000000000000000000000000000000000\",\"gas_data\":{\"payment\":[[\"0x0000000000000000000000000000000000000000000000000000000000000000\",0,\"11111111111111111111111111111111\"]],\"owner\":\"0x0000000000000000000000000000000000000000000000000000000000000000\",\"price\":1,\"budget\":0},\"expiration\":\"None\"}}},\"tx_signatures\":[\"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==\"]}],\"auth_signature\":{}}", "effects_json": "{\"V1\":{\"status\":\"Success\",\"executed_epoch\":0,\"gas_used\":{\"computationCost\":\"0\",\"storageCost\":\"0\",\"storageRebate\":\"0\",\"nonRefundableStorageFee\":\"0\"},\"modified_at_versions\":[[\"0x0000000000000000000000000000000000000000000000000000000000000006\",2181]],\"shared_objects\":[[\"0x0000000000000000000000000000000000000000000000000000000000000006\",2181,\"FoBqk5cvEJyenJWgRe4mwmEaR8ggxskiYK9JDNj26sqC\"]],\"transaction_digest\":\"Asv7bLGEdpgXyA5BQJomHBuhAHnhtzAyyduS2QGTSkqN\",\"created\":[],\"mutated\":[[[\"0x0000000000000000000000000000000000000000000000000000000000000006\",2182,\"9s9KgE89tqaQ6M8Li5MtDBe47magB23ofKdwHCQHEJmr\"],{\"Shared\":{\"initial_shared_version\":1}}]],\"unwrapped\":[],\"deleted\":[],\"unwrapped_then_deleted\":[],\"wrapped\":[],\"gas_object\":[[\"0x0000000000000000000000000000000000000000000000000000000000000000\",0,\"11111111111111111111111111111111\"],{\"AddressOwner\":\"0x0000000000000000000000000000000000000000000000000000000000000000\"}],\"events_digest\":null,\"dependencies\":[\"DLhTcjfkiuBqqxMoAXYt9Se86Smb3sgXbYzKuGiUeQ9o\"]}}"}

Copy link

vercel bot commented Mar 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 14, 2025 1:58am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Mar 14, 2025 1:58am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Mar 14, 2025 1:58am

Comment on lines +87 to +89
pub fn new(watermarks: HashMap<String, CheckpointSequenceNumber>) -> Self {
Self { watermarks }
}
Copy link
Author

Choose a reason for hiding this comment

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

Note that I moved this code and also made a small change to the constructor in the same PR. Before it took a Vec(K, V) and converted it to a HashMap<K, V> in the constructor. Now it takes the HashMap directly. This let me easily validate task name uniqueness in the sui-analytics-indexer main function.

@nickvikeras nickvikeras temporarily deployed to sui-typescript-aws-kms-test-env March 14, 2025 01:56 — with GitHub Actions Inactive
@nickvikeras nickvikeras marked this pull request as ready for review March 14, 2025 02:07
@nickvikeras nickvikeras temporarily deployed to sui-typescript-aws-kms-test-env March 14, 2025 02:07 — with GitHub Actions Inactive
@bmwill
Copy link
Contributor

bmwill commented Mar 14, 2025

Generally looks good to me! I think we probably should wait to land till @sadhansood or @phoenix-o have had a chance to also take a look

Copy link
Contributor

@phoenix-o phoenix-o left a comment

Choose a reason for hiding this comment

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

The data ingestion part looks good to me! I'm not very familiar with analytics parameters though, but we can start moving the pipelines to the new binary one by one

Copy link
Contributor

@sadhansood sadhansood left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @nickvikeras!

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.

4 participants