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.

Sign up for GitHub

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

Jump to bottom

Sishan/basic metrics #1852

Merged
merged 24 commits into from
Oct 11, 2023
Merged

Sishan/basic metrics #1852

merged 24 commits into from
Oct 11, 2023

Conversation

dailinsubjam
Copy link
Contributor

@dailinsubjam dailinsubjam commented Oct 2, 2023 •
edited
Loading

This PR revokes metrics for

  • NetworkingMetrics
    • Number of how many peers are connected
    • Number of messages have been received directly
    • Number of messages have been received by broadcast
    • Number of messages have been send directly
    • Number of messages have been send by broadcast
    • Number of messages failed to send
  • ConsensusMetrics
    • The number of last synced synced block height
    • The number of last decided view
    • The current view
    • Number of views that are in-flight since the last decided view
    • Number of views that are in-flight since the last anchor view
    • Number of invalid QCs we've seen since the last commit
    • Number of rejected transactions
    • Number of outstanding transactions
    • Memory size in bytes of the serialized transactions still outstanding
    • Number of views that timed out

The main parts for review are:
crates/types/src/consensus.rs, crates/task-impls/src/consensus.rs, and crates/hotshot/src/traits/networking.rs.

dailinsubjam and others added 16 commits September 20, 2023 11:48
@dailinsubjam
add debug for metrics
fbca867
@dailinsubjam
add debug for metrics
1fd54e5
@dailinsubjam
add derive(Debug) for TestMetrics
2f64ee9
@dailinsubjam
Merge branch 'sishan/basic_metrics' of github.com:EspressoSystems/Hot…
35e4754 
…Shot into sishan/basic_metrics
@dailinsubjam
Add clone() to NetworkingMetrics
417ab81
@dailinsubjam
fix lint
d5c0786
@dailinsubjam
add Debug to NetworkingMetrics
c1fd4dd
@dailinsubjam
Add Debug for MemoryNetowkrInner
5fb9218
@dailinsubjam
fmt check
b447503
@dailinsubjam
Merge pull request #1802 from EspressoSystems/sishan/basic_metrics_co…
995f06d 
…nnected_peers

Sishan/Revoke metric for tracking the number of peers.
@dailinsubjam
basic metrics completed for networking metrics
480ac7a
@dailinsubjam
Merge pull request #1828 from EspressoSystems/sishan/basic_metrics_ne…
02c0e3e 
…tworking_metrics

basic metrics completed for networking metrics
@dailinsubjam
revoke initialization of consensus metrics, no more NoMetrics Initial…
35c930d 
…ization
@dailinsubjam
fix lint
089edb2
@dailinsubjam
synced most recent block height
ebfc29b
@dailinsubjam
Merge pull request #1842 from EspressoSystems/sishan/basic_metrics_sy…
89d3c6e 
…nc_last_block

sishan/synced most recent block height
@dailinsubjam dailinsubjam linked an issue Oct 2, 2023 that may be closed by this pull request
Basic metrics to support outside validators #1748
Closed
7 tasks
elliedavidson
elliedavidson requested changes Oct 9, 2023
View reviewed changes
crates/task-impls/src/consensus.rs Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/consensus.rs
@@ -1075,6 +1096,8 @@ where
"We received a timeout event in the consensus task for view {}!",
*view
);
let consensus = self.consensus.read().await;
Copy link
Member

Choose a reason for hiding this comment

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

Where do we reset this metric? I think we should reset it once we have a view that succeeds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for resetting the consensus metric number_of_timeouts, and will not be used for any other place. The whole consensus thing is wrapped in an Arc, therefore as long as we know which one to change it should be okay (like here we simply want to get self.consensus).

shenkeyao
shenkeyao reviewed Oct 9, 2023
View reviewed changes
crates/task-impls/src/consensus.rs Show resolved Hide resolved
crates/types/src/consensus.rs Outdated Show resolved Hide resolved
@dailinsubjam dailinsubjam merged commit cb42183 into develop Oct 11, 2023
4 checks passed
@dailinsubjam dailinsubjam deleted the sishan/basic_metrics branch October 11, 2023 19:24
@dailinsubjam dailinsubjam mentioned this pull request Dec 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elliedavidson elliedavidson elliedavidson approved these changes

@bfish713 bfish713 Awaiting requested review from bfish713 bfish713 is a code owner

@DieracDelta DieracDelta Awaiting requested review from DieracDelta

@rob-maron rob-maron Awaiting requested review from rob-maron rob-maron is a code owner

@jbearer jbearer Awaiting requested review from jbearer

@shenkeyao shenkeyao Awaiting requested review from shenkeyao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Basic metrics to support outside validators
3 participants
@dailinsubjam @shenkeyao @elliedavidson