Gm/ssv 137 add pre and post consensus logs #1387

guym-blox · 2024-05-06T14:05:22Z

This pull request proposes the addition of detailed logging to the ProcessPreConsensus,ProcessConsensus and ProcessPostConsensus functions. we aim to enhance visibility into the time consumption of various components during consensus and post-consensus processes.

…nse about time consuming by diff components

…ive more sense about time consuming by diff components

y0sher · 2024-05-20T18:48:40Z

protocol/v2/ssv/runner/attester.go

 		logger.Debug("🧩 reconstructed partial signatures",
-			zap.Uint64s("signers", getPostConsensusSigners(r.GetState(), root)))
+			zap.Uint64s("signers", getPostConsensusSigners(r.GetState(), root)),
+			fields.QuorumTime(r.metrics.GetPostConsensusTime()))


don't you think its confusing to use QuorumTime with same key for pre, consensus and post? just for sake of easy search or comparing results, or making averages, it makes sense to add some prefix like pre_/cons_/post_ to it.

y0sher · 2024-05-20T18:51:27Z

protocol/v2/ssv/runner/attester.go


+		attestationSubmissionEnd := r.metrics.StartBeaconSubmission()


this confused me at first, I think the var name should be or include "EndFunc"

y0sher

@guym-blox and @moshe-blox
I think we should go ahead and soon merge this (with appropriate changes) into alan/no-fork as well.

Approved but please consider my comments.

y0sher · 2024-05-26T09:11:36Z

protocol/v2/ssv/runner/metrics/metrics.go

+	if cm != nil {
+		cm.beaconDataStart = time.Now()
+	}
+}
+
+// EndBeaconData sends metrics for data duration.
+func (cm *ConsensusMetrics) EndBeaconData() {
+	if cm != nil && cm.beaconData != nil && !cm.beaconDataStart.IsZero() {
+		duration := time.Since(cm.beaconDataStart)
+		cm.beaconDataDuration = duration
+		log.Printf("setting beaconDataDuration to %v (started: %s)", duration, cm.beaconDataStart)
+		cm.beaconData.Observe(duration.Seconds())
+		cm.beaconDataStart = time.Time{}
+	}


imo its better to do if cm == nil { return }

imo its better to do if cm == nil { return }

i think this condition should not even exist 😆

instead of checking for nil, we can just use a noopConsensusMetrics implementation

however, this is how the other methods are implemented so we can leave this for a future metrics refactor PR

* Draft of partial signatures as decided * Deploy to stage exporter * Increase buffer size * Add debug log * Fix build and lint issues * Add debug log for any message * More logs * Add message type log * Fix message type * Fix panic * Delete redundant logs * Storage logs * Delete redundant logs * More storage logs * Fix log * Sort signers * Fix some issues * Cleanup * Restore old decided logic for compatibility with explorer * Restore old decided logic for compatibility with explorer [2] * Revert "Restore old decided logic for compatibility with explorer [2]" This reverts commit 80a2e37. * Revert "Restore old decided logic for compatibility with explorer" This reverts commit b61a1b9. * Adjust new API to the old one * Remove redundant cases of using NewDecidedHandler * Identifier as base64 in API * Remove participants DB logs * Remove dummy data * Set commit msg type in deprecated API * Godoc * Don't send data in new format to stream * Rename Operators to Signers * Add FullData to response * Add dummy round * Configurable usage of new API * Rename Operators to Signers in API response * Godoc * Cleanup * Cleanup [2] * Fix TestHandleDecidedQuery * Disable deployment * Fix TestHandleNonCommitteeMessages * Fix option name * Revert "Disable deployment" This reverts commit 369c26b. * Disable deployment * deploy to stage * deploy to stage * remove deployment change * deploy to stage * revert deployment * change deployment * change deployment * change deployment * change deployment * Gm/ssv 137 add pre and post consensus logs (#1387) * add more logs to ProcessPreConsensus * add logs to ProcessConsensus and ProcessPostConsensus to give more sense about time consuming by diff components * change gitlab ci to test on stage * add logs [attester] to ProcessConsensus and ProcessPostConsensus to give more sense about time consuming by diff components * change github ci in order to test on stage * add symbols to validator registration logs * fix typo mistake * fix text to align spec * align all runners logs * add got decided to attester runner * add logs to attester runner * add logs to sync_committee runner * pushing to nodes 61-64 * refactors (still WIP) * fixes * rename * log * fix * refactor * refactors * add slots to logs * bugfix * deploy stage + logs PR to 1-60 * approve spec diffs * revert gitlab * deploy stage + logs PR to nodes 1-60 * revert gitlab * review * fixed log * improve aggregators logs * deploy to 13-60 * fixed agg consensus time * approve spec diffs * fix logs * log block hash in submission errors * fix block hash log * approve spec diff * revert gitlab --------- Co-authored-by: guy muroch <[email protected]> Co-authored-by: moshe-blox <[email protected]> * test deployment * test deployment * fix query_handlers.go merge * fix deployment file * fix deployment file * add logs * add logs * add logs * add logs * add logs * add logs * add logs * add logs * add logs * pull from alan/no-fork * align e2e types * add attester runner to exporter roles * created new storage for old role types * remove extra qbft store * add logs * adding logs * change relevant places spectypes.Role to exporter Role in order to support Attester * fixing typo * adjust participantsRange to work with old spec roles * add validation for validatorByIndex * add validation for validatorByIndex * change Role type to attester instead of committee * change Role type to attester instead of committee * change DomainType back to spectypes * remove space * deploy to stage * push to stage * add logic to save participants with right role (sync_committee, attester) * add syncCommittee role to exporter roles * add sync committee role to roles in exporter * add sync committee role to roles in exporter * add sync committee role to roles in exporter * remove all deprecations from query handlers using decided, only participants work now * add sync_committee store * add sync_committee store * add sync_committee store * fix string * fix broadcast decided message string * add log after save participatns * add log for debug * fix saved participants log * loop over post consensus partial sig messages to build decided * change nonCommittee stracture to support many validators * change parsing non committee messages to work with committeeObserver and not per validator * remove support for query multi public keys * clean code and add logs * fix exporter spec tests * fix tests for exporter + extend TestHandleDecidedQuery to all roles * fix spec test reletead to exporter + remove redundant init qbft stores for operator controller * adding qbft store to operator controller * remove space * remove redundant code * fix issues after code review * fix issues after code review * fix issues after code review * fix issues after code review * finalize PR review * renamed function, added err handling for nvc message processing. * align to alan/no-fork (Domain type) and fix package (convert) reference rename --------- Co-authored-by: Nikita Kryuchkov <[email protected]> Co-authored-by: guy muroch <[email protected]> Co-authored-by: moshe-blox <[email protected]> Co-authored-by: y0sher <[email protected]>

* message validation: use maxPartialSignatureMsgsSize from block with all constants (#1458) * feat: implement genesis validator * fixed shares * fixed queues * adjust exporter to committee consensus (#1409) * Draft of partial signatures as decided * Deploy to stage exporter * Increase buffer size * Add debug log * Fix build and lint issues * Add debug log for any message * More logs * Add message type log * Fix message type * Fix panic * Delete redundant logs * Storage logs * Delete redundant logs * More storage logs * Fix log * Sort signers * Fix some issues * Cleanup * Restore old decided logic for compatibility with explorer * Restore old decided logic for compatibility with explorer [2] * Revert "Restore old decided logic for compatibility with explorer [2]" This reverts commit 80a2e37. * Revert "Restore old decided logic for compatibility with explorer" This reverts commit b61a1b9. * Adjust new API to the old one * Remove redundant cases of using NewDecidedHandler * Identifier as base64 in API * Remove participants DB logs * Remove dummy data * Set commit msg type in deprecated API * Godoc * Don't send data in new format to stream * Rename Operators to Signers * Add FullData to response * Add dummy round * Configurable usage of new API * Rename Operators to Signers in API response * Godoc * Cleanup * Cleanup [2] * Fix TestHandleDecidedQuery * Disable deployment * Fix TestHandleNonCommitteeMessages * Fix option name * Revert "Disable deployment" This reverts commit 369c26b. * Disable deployment * deploy to stage * deploy to stage * remove deployment change * deploy to stage * revert deployment * change deployment * change deployment * change deployment * change deployment * Gm/ssv 137 add pre and post consensus logs (#1387) * add more logs to ProcessPreConsensus * add logs to ProcessConsensus and ProcessPostConsensus to give more sense about time consuming by diff components * change gitlab ci to test on stage * add logs [attester] to ProcessConsensus and ProcessPostConsensus to give more sense about time consuming by diff components * change github ci in order to test on stage * add symbols to validator registration logs * fix typo mistake * fix text to align spec * align all runners logs * add got decided to attester runner * add logs to attester runner * add logs to sync_committee runner * pushing to nodes 61-64 * refactors (still WIP) * fixes * rename * log * fix * refactor * refactors * add slots to logs * bugfix * deploy stage + logs PR to 1-60 * approve spec diffs * revert gitlab * deploy stage + logs PR to nodes 1-60 * revert gitlab * review * fixed log * improve aggregators logs * deploy to 13-60 * fixed agg consensus time * approve spec diffs * fix logs * log block hash in submission errors * fix block hash log * approve spec diff * revert gitlab --------- Co-authored-by: guy muroch <[email protected]> Co-authored-by: moshe-blox <[email protected]> * test deployment * test deployment * fix query_handlers.go merge * fix deployment file * fix deployment file * add logs * add logs * add logs * add logs * add logs * add logs * add logs * add logs * add logs * pull from alan/no-fork * align e2e types * add attester runner to exporter roles * created new storage for old role types * remove extra qbft store * add logs * adding logs * change relevant places spectypes.Role to exporter Role in order to support Attester * fixing typo * adjust participantsRange to work with old spec roles * add validation for validatorByIndex * add validation for validatorByIndex * change Role type to attester instead of committee * change Role type to attester instead of committee * change DomainType back to spectypes * remove space * deploy to stage * push to stage * add logic to save participants with right role (sync_committee, attester) * add syncCommittee role to exporter roles * add sync committee role to roles in exporter * add sync committee role to roles in exporter * add sync committee role to roles in exporter * remove all deprecations from query handlers using decided, only participants work now * add sync_committee store * add sync_committee store * add sync_committee store * fix string * fix broadcast decided message string * add log after save participatns * add log for debug * fix saved participants log * loop over post consensus partial sig messages to build decided * change nonCommittee stracture to support many validators * change parsing non committee messages to work with committeeObserver and not per validator * remove support for query multi public keys * clean code and add logs * fix exporter spec tests * fix tests for exporter + extend TestHandleDecidedQuery to all roles * fix spec test reletead to exporter + remove redundant init qbft stores for operator controller * adding qbft store to operator controller * remove space * remove redundant code * fix issues after code review * fix issues after code review * fix issues after code review * fix issues after code review * finalize PR review * renamed function, added err handling for nvc message processing. * align to alan/no-fork (Domain type) and fix package (convert) reference rename --------- Co-authored-by: Nikita Kryuchkov <[email protected]> Co-authored-by: guy muroch <[email protected]> Co-authored-by: moshe-blox <[email protected]> Co-authored-by: y0sher <[email protected]> * message validation: replace treemap with an array (#1457) Change treemap to array * genesis message validation: remove unused qbft config (#1463) * message validation: optimize constructing validator indices (#1462) message validation: optimize constructing validator indices * Fix commented-out code in network/p2p/test_utils.go (#1456) * Remove remaining GetDefaultDomain/SetDefaultDomain (#1455) Remove remaining GetDefaultDomain/SetDefaultDomain * fix panic and networking * fix rest of the metrics * rename metrics to genesis * fix genesis runners * fix: pass only relevant duty validator shares into the slot committee runner (#1460) * pass only relevant duty validator shares into the slot committee runner * fix: Remove beacon duties without shares in StartDuty function The StartDuty function in the Committee struct now removes beacon duties that do not have a corresponding share. This ensures that only relevant duty validator shares are passed into the slot committee runner. * Stop committee duty if no beacon duties exist. * fix bug in controller * go.mod: remove gods dependency (#1474) * message validation: check voluntary exit existence (#1468) message validation: check voluntary exit existence * refetch duties after metadata update (#1470) * Adjust P2P Scoring for Committee Consensus (#1418) * Message rate calculation * Refactor message rate computation to make formulas more readable * Add validation nil check. Return rate in seconds * Add operators and validators map input. Use the correct message rate calculation * Pass operators and validators maps to NewSubnetTopicOpts and define new GetCommitteeMapsForTopic for PubSubConfig * Add GetCommitteeMapsForTopic method to Config and to the Controller interface * Add GetCommitteeMapsForTopic method to Controller mock * Change log to add committees and validators in topic * Re-deploy * Check if validator is participating before including it * Use list of storage.Committee instead of two maps * Deprecate created GetCommitteeMapsForTopic function. Use list of storage.Committee * Align tests to use committee list * Add debugging logs * Logs for debugging * Re-deploy * Debugging logs * Use topic full name for filtering comparison * Remove debugging logs * Remove unused errors * Remove unnecessary error from returned values. * Remove mistaken newline --------- Co-authored-by: MatheusFranco99 <[email protected]> * Refresh P2P scoring parameters (#1469) * Message rate calculation * Refactor message rate computation to make formulas more readable * Add validation nil check. Return rate in seconds * Add operators and validators map input. Use the correct message rate calculation * Pass operators and validators maps to NewSubnetTopicOpts and define new GetCommitteeMapsForTopic for PubSubConfig * Add GetCommitteeMapsForTopic method to Config and to the Controller interface * Add GetCommitteeMapsForTopic method to Controller mock * Change log to add committees and validators in topic * Re-deploy * Check if validator is participating before including it * Use list of storage.Committee instead of two maps * Deprecate created GetCommitteeMapsForTopic function. Use list of storage.Committee * Align tests to use committee list * Add debugging logs * Logs for debugging * Re-deploy * Debugging logs * Use topic full name for filtering comparison * Remove debugging logs * Remove unused errors * Remove unnecessary error from returned values. * Remove mistaken newline * Add function in topics controller to update the topic params * Trigger UpdateScoreParams in p2pNetwork.UpdateSubnets function * Stop using p2pNetwork.UpdateSubnets and use a new function solely for updating the score params * Remove error.Join due to compilation error * Apply suggestion to use a CommitteeProvider interface --------- Co-authored-by: Nikita Kryuchkov <[email protected]> * empty commit * Update holesky-stage.go * continue to read messages instead of returning on exporter (#1476) * align committee runner consensus logs (#1475) * align committee runner consensus logs * change convert.Role to spectype.RunnerRole in runners * added back logs for debug * change got partial signatures log to contain more details --------- Co-authored-by: guy muroch <[email protected]> * switch to old msg validations * add comment to be more clear * remove old share storage * fix: (alan) bring back slashing check from spec (#1479) * bring back valcheck from spec * chore: Pass attestingValidators to CommitteeRunnerFunc in SetupCommitteeRunners for value checking. * refactor: attestingValidators -> slashableValidators * Alan/switch domain at runtime (#1454) * add DomainType() calculating fork * update network consumers to use DomainType() * add ENR update function at fork * add fork listener to scheduler * add fork handler test * some refactors and fixes * TODO * use dynamic domain type in runners * add next domain type support in discovery * fix merge leftovers * (handshakes) update self NodeInfo domain type before handing over to peers * change Alan domain type for holesky-stage --------- Co-authored-by: moshe-blox <[email protected]> Co-authored-by: y0sher <[email protected]> * empty commit * add subscriber * empty commit * tests: fix store tests * fix domain test * pass domain type provider to handshake config (#1484) * fix controller tests * (Alan/no-fork) Make spec tests work again (#1480) * fixed compilation errors and runtime panics * removed incorrect filtering * removed panic * added helpers to correclty get signers * removed convert usage outside of db work * fixed qbft * added missing domain provider in fixRunner * added TODO for filtering * returned slashable validators. fixed ibft store tests * fixed comments * fix subscriber * fix bug in network wrapper * enable logs * align attester to support prefork * add genesis duty executor * add more logs * add more logs * even more logs * genesis msg handling * fix fork resolving * add logs --------- Co-authored-by: Nikita Kryuchkov <[email protected]> Co-authored-by: guym-blox <[email protected]> Co-authored-by: guy muroch <[email protected]> Co-authored-by: moshe-blox <[email protected]> Co-authored-by: y0sher <[email protected]> Co-authored-by: MatheusFranco99 <[email protected]> Co-authored-by: Lior Rutenberg <[email protected]> Co-authored-by: Pavel Krolevets <[email protected]> Co-authored-by: Anton Korpusenko <[email protected]>

guy muroch added 2 commits May 6, 2024 15:14

add more logs to ProcessPreConsensus

e5796b0

add logs to ProcessConsensus and ProcessPostConsensus to give more se…

ec45aa6

…nse about time consuming by diff components

guym-blox requested review from y0sher, liorrutenberg, moshe-blox and Roy-blox May 6, 2024 14:05

guy muroch and others added 24 commits May 6, 2024 17:21

change gitlab ci to test on stage

511234a

add logs [attester] to ProcessConsensus and ProcessPostConsensus to g…

68437de

…ive more sense about time consuming by diff components

change github ci in order to test on stage

86faac4

add symbols to validator registration logs

083108e

fix typo mistake

9a9e425

fix text to align spec

cbc6381

align all runners logs

e929a15

add got decided to attester runner

c6fd801

add logs to attester runner

9648058

add logs to sync_committee runner

f36ed8e

pushing to nodes 61-64

17e5623

refactors (still WIP)

23f2a53

Merge branch 'stage' into gm/SSV-137_add_pre_and_post_consensus_logs

0a20141

fixes

ac79e35

rename

40296bd

log

8d006b9

fix

4f9ceec

refactor

cd80766

refactors

354d7bc

add slots to logs

92bf68f

bugfix

391e78e

deploy stage + logs PR to 1-60

aac191a

approve spec diffs

449efd7

revert gitlab

a232281

moshe-blox added 2 commits May 20, 2024 17:49

deploy stage + logs PR to nodes 1-60

fe7413a

revert gitlab

d1b63f6

y0sher reviewed May 20, 2024

View reviewed changes

y0sher previously approved these changes May 20, 2024

View reviewed changes

review

87b6063

moshe-blox dismissed y0sher’s stale review via 87b6063 May 21, 2024 09:31

moshe-blox added 10 commits May 21, 2024 12:34

fixed log

9d0c45e

improve aggregators logs

04863ee

deploy to 13-60

44192e7

fixed agg consensus time

32c652c

approve spec diffs

d7d823b

fix logs

b1ceb69

log block hash in submission errors

d790317

fix block hash log

ba7ae84

approve spec diff

7ae3a12

revert gitlab

9a3c8ab

moshe-blox requested review from y0sher and nkryuchkov May 22, 2024 13:44

y0sher reviewed May 26, 2024

View reviewed changes

y0sher approved these changes May 26, 2024

View reviewed changes

moshe-blox approved these changes May 26, 2024

View reviewed changes

moshe-blox merged commit cbe6254 into stage May 26, 2024
5 checks passed

moshe-blox deleted the gm/SSV-137_add_pre_and_post_consensus_logs branch May 26, 2024 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gm/ssv 137 add pre and post consensus logs #1387

Gm/ssv 137 add pre and post consensus logs #1387

guym-blox commented May 6, 2024

y0sher May 20, 2024

moshe-blox May 21, 2024

y0sher May 20, 2024

moshe-blox May 21, 2024

y0sher left a comment

y0sher May 26, 2024

moshe-blox May 26, 2024 •

edited

Loading


		attestationSubmissionEnd := r.metrics.StartBeaconSubmission()

Gm/ssv 137 add pre and post consensus logs #1387

Gm/ssv 137 add pre and post consensus logs #1387

Conversation

guym-blox commented May 6, 2024

y0sher May 20, 2024

Choose a reason for hiding this comment

moshe-blox May 21, 2024

Choose a reason for hiding this comment

y0sher May 20, 2024

Choose a reason for hiding this comment

moshe-blox May 21, 2024

Choose a reason for hiding this comment

y0sher left a comment

Choose a reason for hiding this comment

y0sher May 26, 2024

Choose a reason for hiding this comment

moshe-blox May 26, 2024 • edited Loading

Choose a reason for hiding this comment

moshe-blox May 26, 2024 •

edited

Loading