Skip to content

feat: update tracing crate to thin aptos-telemetry wrapper #1246

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

Merged
merged 10 commits into from
Jun 25, 2025

Conversation

0xIcarus
Copy link
Contributor

@0xIcarus 0xIcarus commented Jun 5, 2025

Summary

  • RFCs: $\emptyset$
  • Categories: util

Updated metrics for all components using aptos-core. The tracing crate now acts as a thin wrapper around aptos-telemetry.

Changelog

  • Modified lib.rs in the tracing crate by calling aptos-telemetry with the custom metrics changes that have been incorporated in our fork

Testing

Run using this command: nix develop --command bash -c "just movement-full-node native build.setup.eth-local.celestia-local.telemetry --keep-project"

Metrics should be exposed at localhost:9464

Outstanding issues

@0xmovses
Copy link
Collaborator

Is this ready to go?

@0xmovses
Copy link
Collaborator

Is this required for metrics to go out or an improvement.

@0xIcarus
Copy link
Contributor Author

@0xmovses its a requirement

@0xIcarus
Copy link
Contributor Author

yes it is ready to go but need to change the refs after we merge the aptos-core PR

@0xIcarus 0xIcarus marked this pull request as ready for review June 18, 2025 07:41
@Copilot Copilot AI review requested due to automatic review settings June 18, 2025 07:41
Copilot

This comment was marked as outdated.

Copy link
Contributor

@andygolay andygolay left a comment

Choose a reason for hiding this comment

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

Looking good @0xIcarus just need to resolve conflicts.

@0xmovses
Copy link
Collaborator

@0xIcarus could you resolve these and we get it merged? Let's ship this.

@0xmovses 0xmovses requested a review from andygolay June 23, 2025 23:03
@0xmovses
Copy link
Collaborator

New conflicts have emerged due to #1138 being merged just now (this was expected and by design). After you resolve this new changes in main, we should be good to merge this.

Then we can start testing out the next release, which will have metrics + da-sequencer (monza).

@0xmovses
Copy link
Collaborator

@andygolay & @musitdev pending your approval for merge.

@0xmovses
Copy link
Collaborator

@0xIcarus Cargo fmt Check is failing. Run cargo format at the root of the workspace (or your changed files)

@0xmovses 0xmovses requested a review from Copilot June 24, 2025 17:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the tracing crate into a thin wrapper around aptos-telemetry, updates all services to use the new telemetry setup, and exposes/promotes metrics endpoints across local and Docker Compose workflows.

  • Replace in-crate tracing setup with aptos-telemetry initialization in the standalone movement-tracing binary
  • Remove legacy movement_tracing::init_* calls from light-node and full-node binaries
  • Update Compose and Prometheus configurations to expose and scrape metrics on port 9464

Reviewed Changes

Copilot reviewed 8 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
util/tracing/src/main.rs Added a standalone telemetry server binary with logger setup
util/tracing/Cargo.toml Swapped core tracing deps for forked aptos-telemetry crates; set default-run
protocol-units/da/movement/protocol/light-node/src/main.rs Removed old tracing subscriber initialization
process-compose/movement-full-node/process-compose.yml Exposed host port 9464 for metrics
process-compose/movement-full-node/process-compose.telemetry.yml Refactored telemetry service entry and env vars
networks/movement/movement-full-node/src/main.rs Removed legacy telemetry init from the movement-full-node binary
docker/compose/movement-full-node/prometheus.yml Added new scrape jobs for movement metrics endpoints
docker/compose/movement-full-node/docker-compose.telemetry.yml Updated telemetry service env vars and port mappings
Comments suppressed due to low confidence (3)

protocol-units/da/movement/protocol/light-node/src/main.rs:9

  • The tracing subscriber initialization was removed, so metrics and tracing spans will no longer be captured. Please reintroduce a call to initialize telemetry (e.g., movement_tracing::ensure_telemetry_initialized()).
async fn main() -> Result<(), Box<dyn std::error::Error>> {

networks/movement/movement-full-node/src/main.rs:6

  • Telemetry initialization was removed here, so the full-node binary will start without metrics. You should call the new wrapper's initialization function (e.g., ensure_telemetry_initialized()) at startup.
#[tokio::main]

util/tracing/Cargo.toml:27

  • The warp dependency (and other crates like hex, rand, ring, clap) are not referenced in main.rs. Consider removing unused dependencies to reduce build times and maintenance overhead.
warp = "0.3"

@0xmovses 0xmovses changed the title Update tracing crate to be a thin aptos-telemetry wrapper feat: update tracing crate to be a thin aptos-telemetry wrapper Jun 24, 2025
@0xmovses 0xmovses changed the title feat: update tracing crate to be a thin aptos-telemetry wrapper feat: update tracing crate to thin aptos-telemetry wrapper Jun 24, 2025
Copy link
Contributor

@musitdev musitdev left a comment

Choose a reason for hiding this comment

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

I don't know how prometheus is working so I don't have much remarks about its integration. My only one, is I don't see where it is integrated in the node.

@0xIcarus
Copy link
Contributor Author

I don't know how prometheus is working so I don't have much remarks about its integration. My only one, is I don't see where it is integrated in the node.

It is pulling metrics that are emmited from components using aptos-core directly

@0xmovses 0xmovses merged commit 06b1562 into main Jun 25, 2025
39 of 40 checks passed
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