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

ProposerFactory impl Clone #3389

Merged
merged 4 commits into from
Feb 21, 2024
Merged

Conversation

tmpolaczyk
Copy link
Contributor

@tmpolaczyk tmpolaczyk commented Feb 19, 2024

In Tanssi, we need a way to stop the collator code and then start it again. This is to support rotating the same collator between different runtimes. Currently, this works very well, except for the proposer metrics, because they only get registered the first time they are started. Afterwards, we see this warning log:

Failed to register proposer prometheus metrics: Duplicate metrics collector registration attempted

So this PR adds a method to set metrics, to allow us to register metrics manually before creating the ProposerFactory, and then clone the same metrics every time we need to start the collator. Implemented Clone instead

@bkchr
Copy link
Member

bkchr commented Feb 19, 2024

An alternative would be to #[derive(Clone)] for ProposerFactory, but that seemed more intrusive.

I would propose this way :D

@tmpolaczyk
Copy link
Contributor Author

I would propose this way :D

Ok, I manually implemented Clone, because deriving it would add unnecessary bounds such as C: Clone because of generics. And I wasn't able to use CloneNoBound because frame_support is not a dependency here, not sure if I should just add it.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5261781

include_proof_in_block_size_estimation: self
.include_proof_in_block_size_estimation
.clone(),
_phantom: self._phantom.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_phantom: self._phantom.clone(),
_phantom: self._phantom,

Comment on lines 194 to 198

/// Set metrics.
pub fn set_metrics(&mut self, metrics: PrometheusMetrics) {
self.metrics = metrics;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Set metrics.
pub fn set_metrics(&mut self, metrics: PrometheusMetrics) {
self.metrics = metrics;
}

@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label Feb 21, 2024
@tmpolaczyk tmpolaczyk changed the title ProposerFactory add method to set metrics ProposerFactory impl Clone Feb 21, 2024
@bkchr bkchr requested a review from a team February 21, 2024 16:22
@bkchr bkchr enabled auto-merge February 21, 2024 16:22
@bkchr bkchr added this pull request to the merge queue Feb 21, 2024
Merged via the queue into paritytech:master with commit 318fed3 Feb 21, 2024
131 of 132 checks passed
girazoki pushed a commit to moondance-labs/polkadot-sdk that referenced this pull request Feb 22, 2024
In Tanssi, we need a way to stop the collator code and then start it
again. This is to support rotating the same collator between different
runtimes. Currently, this works very well, except for the proposer
metrics, because they only get registered the first time they are
started. Afterwards, we see this warning log:

> Failed to register proposer prometheus metrics: Duplicate metrics
collector registration attempted


~~So this PR adds a method to set metrics, to allow us to register
metrics manually before creating the `ProposerFactory`, and then clone
the same metrics every time we need to start the collator.~~ Implemented
Clone instead
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
In Tanssi, we need a way to stop the collator code and then start it
again. This is to support rotating the same collator between different
runtimes. Currently, this works very well, except for the proposer
metrics, because they only get registered the first time they are
started. Afterwards, we see this warning log:

> Failed to register proposer prometheus metrics: Duplicate metrics
collector registration attempted


~~So this PR adds a method to set metrics, to allow us to register
metrics manually before creating the `ProposerFactory`, and then clone
the same metrics every time we need to start the collator.~~ Implemented
Clone instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants