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

Relayer metrics #515

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

Relayer metrics #515

wants to merge 5 commits into from

Conversation

vovac12
Copy link
Contributor

@vovac12 vovac12 commented Jun 2, 2023

No description provided.

Copy link
Contributor

@Alexey-N-Chernyshov Alexey-N-Chernyshov left a comment

Choose a reason for hiding this comment

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

  1. Use gauge for non-monotonic metrics (ids, lengths, etc.). Otherwise Prometheus will consider it as metric reset.
  2. Just an idea, you can use histogram for response time and response code for RPC calls, as it is in actix-web-prometheus crate.

Comment on lines 49 to 51
let beefy = self.beefy.map(|x| BeefyLightClient::new(x, client.inner()));
let inbound = self.beefy.map(|x| InboundChannel::new(x, client.inner()));
let outbound = self.beefy.map(|x| OutboundChannel::new(x, client.inner()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let beefy = self.beefy.map(|x| BeefyLightClient::new(x, client.inner()));
let inbound = self.beefy.map(|x| InboundChannel::new(x, client.inner()));
let outbound = self.beefy.map(|x| OutboundChannel::new(x, client.inner()));
let beefy = self.beefy.map(|x| BeefyLightClient::new(x, client.inner()));
let inbound = self.inbound.map(|x| InboundChannel::new(x, client.inner()));
let outbound = self.outbound.map(|x| OutboundChannel::new(x, client.inner()));

Typo?

if self.enable_metrics {
let mut builder = metrics_exporter_prometheus::PrometheusBuilder::new();
if let Some(address) = &self.prometheus_address {
builder = builder.with_http_listener(address.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
builder = builder.with_http_listener(address.clone());
builder = builder.with_http_listener(*address);

metrics::absolute_counter!(ETH_OUTBOUND_NONCE, nonce, labels);
}
let block_number = self.client.get_block_number().await?.as_u64();
metrics::counter!(ETH_BLOCK_NUMBER, block_number, labels);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
metrics::counter!(ETH_BLOCK_NUMBER, block_number, labels);
metrics::absolute_counter!(ETH_BLOCK_NUMBER, block_number, labels);

Set an absolute value, not increment.

vset.id,
labels
);
metrics::absolute_counter!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Counter is a monotonic value, but as I understand, the next length might be less than current. Consider gauge type for the metric.

.beefy_light_client()
.current_validator_set(network_id);
self.update_metric(&address, block_hash, |vset| {
metrics::absolute_counter!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Id the ID is not a monotonic, consider gauge type.

Comment on lines 88 to 89
metrics::absolute_counter!(SUB_BEEFY_CURRENT_ID, vset.id);
metrics::absolute_counter!(SUB_BEEFY_CURRENT_LEN, vset.len as u64);
Copy link
Contributor

Choose a reason for hiding this comment

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

If ID and length is not a monotonic, consider gauge.

metrics::absolute_counter!(ETH_BEEFY_LATEST_BLOCK, latest_block, labels);
}
if let Some(contract) = &self.inbound {
let nonce: u64 = contract.nonce().call().await?;
Copy link
Contributor

@Alexey-N-Chernyshov Alexey-N-Chernyshov Jun 2, 2023

Choose a reason for hiding this comment

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

Suggested change
let nonce: u64 = contract.nonce().call().await?;
let nonce: u64 = contract.batch_nonce().call().await?;

It is batch_nonce in develop in InboundChannel.sol

# Conflicts:
#	Cargo.lock
#	relayer/src/cli/bridge/relay/sora/evm.rs
#	relayer/src/substrate/mod.rs
@Tieumsan
Copy link
Contributor

@vovac12 can we merge and close this ?

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.

3 participants