[Fixes] [Native Yield Automation Service] Observability improvements for Hoodi testing #1789
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add OperationModeFailureTotal metric to track operation mode failures - Add UNKNOWN mode to OperationMode enum for failures before mode selection - Implement incrementOperationModeFailure method in metrics updater - Track current mode in OperationModeSelector and increment failure metric in catch block - Add comprehensive unit tests for failure metric functionality - Update all test mocks to include new incrementOperationModeFailure method
…status enum - Merge OperationModeFailureTotal into OperationModeExecutionTotal with status label - Add OperationModeExecutionStatus enum (Success, Failure) for type safety - Update incrementOperationModeExecution to accept optional status parameter - Remove incrementOperationModeFailure method - Update OperationModeSelector to use enum values for status tracking - Update all test files to use enum instead of string literals - Reduces metric overhead and simplifies success rate calculations
- Remove OperationModeTriggerTotal metric enum value - Remove incrementOperationModeTrigger method from interface and implementation - Remove incrementOperationModeTrigger calls from all operation mode processors - Remove incrementOperationModeTrigger from all test mocks and expectations - Remove related test case for incrementOperationModeTrigger - Clean up unused OperationTrigger imports where no longer needed
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| - name: Build and push native-yield-automation-service image | ||
| uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 #v6.18.0 | ||
| if: ${{ env.PUSH_IMAGE == 'true' || github.event_name == 'workflow_dispatch' }} | ||
| # if: ${{ env.PUSH_IMAGE == 'true' || github.event_name == 'workflow_dispatch' }} |
There was a problem hiding this comment.
Bug: Commented-out condition causes unintended Docker image pushes
The if condition controlling when to push Docker images has been commented out. This causes the "Build and push native-yield-automation-service image" step to always run with push: true, even when PUSH_IMAGE is set to false. The workflow was designed to only push images when PUSH_IMAGE == 'true' or during manual workflow_dispatch events. This change is not mentioned in the PR description and appears to be accidentally committed debugging code that would result in unwanted image pushes to the registry.
…ub settleableLidoFeesValue - Add settleableLidoFeesValue method to IVaultHub interface and VaultHubContractClient - Update YieldReportingProcessor to use VaultHub.settleableLidoFeesValue instead of Dashboard.peekUnpaidLidoProtocolFees - Remove peekUnpaidLidoProtocolFees from IDashboard interface and DashboardContractClient - Add logger parameter to VaultHubContractClient constructor for error handling - Update all tests to use vaultHubClient instead of dashboardClient for fee queries - Remove unused logger instance variable from DashboardContractClient
…idoFees - Rename metric enum LastPeekUnpaidLidoProtocolFees to LastSettleableLidoFees - Update metric string to reflect settleable fees terminology - Rename setLastPeekUnpaidLidoProtocolFees to setLastSettleableLidoFees - Update method documentation and gauge descriptions - Update all test mocks and expectations to use new naming - Aligns naming with VaultHub.settleableLidoFeesValue method
- Change logger.debug to logger.error in peekYieldReport catch block - Update test to expect logger.error instead of logger.debug - Makes simulation failures visible in production logs (INFO level and above) - Helps diagnose why yieldReport can be undefined without visible errors
…tClient - Create test-yield-manager-contract-client.ts script following same pattern as test-lazy-oracle-contract-client.ts - Supports required env vars: RPC_URL, PRIVATE_KEY, YIELD_MANAGER_ADDRESS - Supports optional env vars: REBALANCE_TOLERANCE_BPS, MIN_WITHDRAWAL_THRESHOLD_ETH - Includes placeholder for method calls to test YieldManagerContractClient methods
- Add getSignerAddress() method to IBlockchainClient interface - Implement getSignerAddress() in ViemBlockchainClientAdapter that delegates to contractSignerClient.getAddress() - Add test case to verify getSignerAddress() delegates correctly - Enables access to signer address through IBlockchainClient interface
Update peekYieldReport to include account parameter in simulate.reportYield call using contractClientLibrary.getSignerAddress(). This ensures the simulation uses the correct signer address for accurate yield report peeking. - Add account parameter to contract.simulate.reportYield call - Update unit tests to mock getSignerAddress and verify account parameter
Update recordRebalance method signature to accept all RebalanceDirection enum values (NONE, STAKE, UNSTAKE) instead of restricting to STAKE | UNSTAKE. Add handling for NONE direction to skip recording when no rebalance is required. - Update interface to accept full RebalanceDirection enum - Add NONE check to skip recording when direction is NONE - Update JSDoc to reflect all accepted directions
Add recordRebalance call when rebalance direction is NONE to ensure metrics are tracked even when no rebalance is required. Records with STAKE direction and 0 amount (which will be skipped by the metrics updater but maintains consistent call pattern). - Add recordRebalance call in _handleRebalance for NONE case - Update unit test to verify recordRebalance is called
…ipts - Remove zero-value checks before recording metrics to ensure all transaction data is captured, even when values are zero - Add warning logs when transaction receipts are undefined to improve observability - Update tests to match new behavior: metrics are always recorded - Remove unused logger void statement from constructor
- Add warning log when yield report is not found in receipt for recordReportYieldMetrics method - Add warning log when withdrawal event is not found in receipt for recordSafeWithdrawalMetrics method - Update tests to verify new warning logs are called correctly - Improves observability by logging when expected transaction data is missing
- Add getTxReceipt method to IBlockchainClient interface - Implement getTxReceipt in ViemBlockchainClientAdapter that retrieves transaction receipts by hash using Viem's getTransactionReceipt - Handle errors gracefully by returning undefined and logging warnings - Add comprehensive unit tests covering success, not found, network error, and generic error cases
- Add **/*.test.ts pattern to .eslintignore to exclude test files from linting - Remove trailing newline from IDashboard.ts
Remove early return check that skipped recording metrics for NONE direction or non-positive amounts. The method now always records rebalance metrics regardless of direction or amount value. Update unit tests to verify metrics are recorded for zero amounts, negative amounts, and NONE direction.
Fix bug where metrics were incorrectly recorded with STAKE direction when rebalance direction was NONE. Now correctly records NONE direction with amount 0 when no rebalance is needed. Update unit test to expect NONE direction instead of STAKE.
Add info log message when safeAddToWithdrawalReserveIfAboveThreshold skips execution due to available withdrawal balance being below the minimum threshold. This improves observability for debugging withdrawal reserve operations. Update unit test to verify logger.info is called with expected message containing balance and threshold values.
Add info log message when safeMaxAddToWithdrawalReserve skips execution due to available withdrawal balance being below the minimum threshold. This improves observability for debugging withdrawal reserve operations. Update unit test to verify logger.info is called with expected message containing balance and threshold values.
Refactor _shouldReportYield to log the decision result (shouldReportYield) along with the input values. This improves observability by making it easier to see the decision outcome in logs without needing to parse threshold calculations. Move logging to after threshold calculations so the decision result can be included in the log message. Update unit tests to expect the new log format with shouldReportYield value included.
This reverts commit 1f9ccf8.
This reverts commit eb8590c.
Add logging statements before all early returns to follow the principle 'Do not silently do early return, please log'. This improves observability by ensuring all early exit conditions are logged with appropriate log levels: - debug: for expected/benign conditions (already running, empty lists) - info: for informational conditions (no work needed) - warn: for unexpected but handled conditions (missing data, errors) - error: for error conditions that cause early return Updated all corresponding unit tests to verify logging calls are made with appropriate messages and log levels. Affected classes: - OperationModeSelector - OssificationPendingProcessor - YieldManagerContractClient - OperationModeMetricsRecorder - ConsensysStakingApiClient - BeaconChainStakingClient - LazyOracleContractClient
Separate validator array logging into two log calls for better log level separation: - logger.info: Main message with array length (summary information) - logger.debug: Full validator array content (detailed information) This allows info-level logs to show summary data without verbose array content, while debug-level logs provide full details when needed. Updated methods: - _submitPartialWithdrawalRequests - _submitValidatorExits
Upgrade informational logger.debug calls to logger.info for better log level separation. Keep logger.debug only for large data structures (validator arrays) that should only appear in debug-level logs. Changed to logger.info: - Method start messages (submitWithdrawalRequestsToFulfilAmount, submitMaxAvailableWithdrawalRequests) - Empty list and early return conditions - Remaining withdrawal counts Kept as logger.debug: - Full validator array content (large data structures) Updated corresponding unit tests to expect logger.info instead of logger.debug for the changed log calls.
…ConsensysStakingApiClient - Split logger.debug calls that log large data structures into two logs: - logger.info for short summary messages with counts - logger.debug for the actual large data structures - Change logger.debug to logger.info for simple numeric value logs - Fix typo: 'succeded' -> 'succeeded' - Update tests to expect both logger.info and logger.debug calls This matches the logging pattern used in BeaconChainStakingClient.ts for better observability: info logs provide quick summaries while debug logs contain verbose data structures.
- Change logger.debug to logger.info for simple 'started' messages - Split unstake method logging into logger.info (summary with validatorCount) and logger.debug (full withdrawalParams object) - Update tests to expect logger.info for started messages and both logger.info and logger.debug for unstake method logs This matches the logging pattern used in ConsensysStakingApiClient and BeaconChainStakingClient: info logs for summaries, debug logs for large data structures.
- Add test case to cover branch where nodes is undefined - Tests the ?? 0 fallback in validatorCount logging - Achieves 100% branch coverage (was 93.33%)
- Add LastTotalPendingPartialWithdrawalsGwei gauge metric enum - Create gauge in NativeYieldAutomationMetricsUpdater constructor - Implement setLastTotalPendingPartialWithdrawalsGwei setter method - Add method signature to INativeYieldAutomationMetricsUpdater interface - Call metric setter in BeaconChainStakingClient when getTotalPendingPartialWithdrawalsWei is called - Update all relevant test files with new metric and assertions
- Change gauge setter methods from async Promise<void> to synchronous void - Remove async/await from setLastPeekedNegativeYieldReport - Remove async/await from setLastPeekedPositiveYieldReport - Remove async/await from setLastSettleableLidoFees - Remove async/await from setLastTotalPendingPartialWithdrawalsGwei - Update all call sites to remove await and Promise.all - Update all test files to remove async/await from gauge setter calls The underlying setGauge method is synchronous, so these wrapper methods don't need to be async. This simplifies the code and removes unnecessary promise overhead.
- Add IValidatorDataClient dependency to OperationModeSelector constructor - Create private refreshGaugeMetrics method that refreshes LastTotalPendingPartialWithdrawalsGwei gauge - Call refreshGaugeMetrics at start of each loop iteration wrapped in attempt utility - Update NativeYieldAutomationServiceBootstrap to pass validatorDataClient instance - Add comprehensive tests for refreshGaugeMetrics behavior The method follows the same pattern as BeaconChainStakingClient and ensures the gauge metric is refreshed every loop iteration. Failures are handled gracefully using the attempt utility so they don't interrupt the main loop.
Add logging to track the count of pending partial withdrawals when successfully fetched. Update unit tests to verify the new log line.
Update log message from 'Initialising HTTPS agent' to 'Initialising Web3SignerClientAdapter' for better observability and clarity. Update corresponding unit test assertions.
Add LOG_LEVEL environment variable to control Winston logger verbosity in Native Yield Automation Service. Supports all Winston log levels: error, warn, info, verbose, debug, silly. Defaults to 'info' if not set. - Add LOG_LEVEL to config schema with enum validation - Update toClientConfig to use LOG_LEVEL env var with 'info' default - Add comprehensive tests for schema validation and config mapping
Capture and log detailed error information when refreshGaugeMetrics fails, including error message and stack trace. This helps diagnose failures that occur even when API calls succeed. - Capture attempt() result and check for errors - Log detailed error information at ERROR level when refreshGaugeMetrics fails - Update unit tests to verify error logging behavior
…ApiClient Fix 'Cannot mix BigInt and other types' error by converting GraphQL string responses (balance, effectiveBalance, validatorIndex) to bigint before using them in calculations. GraphQL returns numeric values as strings, but the code expects bigint values. - Convert GraphQL response values to bigint using BigInt() constructor - Update unit test to mock GraphQL responses with strings and verify conversion - Handle undefined nodes case properly with logging
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Better track when operation mode execution succeeds and fails:
More relevant trigger for reportYield:
Fix failing YieldManagerContractClient::peekReportYield method
test-yield-manager-contract-client.tsto diagnose the issueMitigate sparsely updated Counter metric issue
Add logs for early returns
Change logger.debug to logger.info + logger.debug for large data structure
Update YieldManager ABI
Add gauge metric
linea_native_yield_automation_service_last_total_pending_partial_withdrawals_gweiNote
Overhauls metrics/observability, switches fee source to
VaultHub, adds new gauges and execution status tracking, updates ABIs/clients, supportsLOG_LEVEL, and adds a manual test script.OperationModeTriggerTotalwithOperationModeExecutionTotalincludingstatus(success|failure); add histogram updates and failure tracking across modes.LastSettleableLidoFeesandLastTotalPendingPartialWithdrawalsGwei; update setters and usage.OperationModeSelectornow records success/failure per run, refreshes pending withdrawals gauge each loop, and addsUNKNOWNmode.VaultHub.settleableLidoFeesValue(new ABI + client method) instead ofDashboard.obligations; remove dashboard obligations from ABI/client.YieldManagerABI; fixpeekYieldReportby simulating with signeraccount; broaden info-level logs.ViemBlockchainClientAdapter: addgetSignerAddressandgetTxReceiptAPIs.YieldReportingProcessorandOssification*processors: remove trigger counter usage, improve logging and error handling; integrate new fee gauge and metrics recorder behaviors.BeaconChainStakingClient: promote key logs to info; emit pending partial withdrawals gauge.ConsensysStakingApiClient: convert GraphQL numeric strings tobigint; improved logging and warnings.LOG_LEVEL; plumb intologgerOptions(defaultinfo).scripts/test-yield-manager-contract-client.ts..eslintignoreignores tests.Written by Cursor Bugbot for commit 8636544. This will update automatically on new commits. Configure here.