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

A0-4591: Added major sync metric back (although by different name) #1919

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

Conversation

Marcin-Radecki
Copy link
Contributor

Description

This PR brings back major syncing metric that was unwillingly removed in 14 release. It copies some code from polkadot-sdk (metric definition), copy is required since we want different name and MajorSyncingGauge constructor is private in polkadot-sdk. Other than that implementation approach is taken from the polkadot-sdk - the AtomicBool is passed from its creation down to when it's used in the metric gauge.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Testing

I run locally ./scripts/run_nodes.sh -v 4, then immediately run `watch -n -1 ' curl localhost:9616/metrics | grep major_sync' is one console and in the other

[12:22] marol-Latitude-5521:aleph-node (A0-4591 *%) | tail -f run-nodes-local/node-1.log | grep "major sync"
2025-01-24 12:22:36.317  INFO tokio-runtime-worker aleph-block-sync: Switched to major sync state.
2025-01-24 12:22:46.320  INFO tokio-runtime-worker aleph-block-sync: No longer in major sync state.

and I saw correlation when the log appeared, the metric switched its state accordingly.

@Marcin-Radecki Marcin-Radecki marked this pull request as ready for review January 24, 2025 12:16
Copy link
Contributor

@timorleph timorleph left a comment

Choose a reason for hiding this comment

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

This can probably be made quite a bit simpler, see the single comment. But other than that it's alright, so if you think my suggestion is bad feel free to merge.

bin/node/src/service.rs Outdated Show resolved Hide resolved
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.

2 participants