feat: time and gas consumption metrics for each contract function

[carneiro-cw]

No description provided.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, due to the extensive changes across multiple files, the introduction of new functionality related to metrics, and the need to ensure that the changes do not introduce bugs or performance issues.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The use of unwrap() in multiple places across the PR could lead to panics if the expected conditions are not met. This is risky, especially in production code.
	Performance Concern: The addition of metrics logging in various parts of the transaction execution path could potentially impact performance, especially under high load. This needs careful consideration and possibly stress testing.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/eth/primitives/bytes.rs
suggestion	Consider handling the case where SIGNATURES_4_BYTES.get(&id) returns None more gracefully. Currently, it defaults to encoding the ID, but this might not always be the desired behavior. Perhaps logging or a different fallback could be implemented. [important]
relevant line	None => Cow::from(const_hex::encode_prefixed(id)),

---

relevant file	build.rs
suggestion	Replace unwrap() calls with more robust error handling to prevent potential panics. For example, you could use ? to propagate errors or provide more context-specific error messages. [important]
relevant line	let prefix = signature_file.file_name().unwrap().to_str().unwrap().split('.').next().unwrap();

---

relevant file	src/eth/executor.rs
suggestion	Consider caching the result of evm_input.extract_function() to avoid recalculating it multiple times within the same scope, which currently happens in several places. This could improve performance, especially for complex transactions. [medium]
relevant line	let function = evm_input.extract_function();

---

relevant file	src/eth/executor.rs
suggestion	Ensure that metrics related to function execution (metrics::inc_executor_transact_gas) are accurately capturing all scenarios, including error paths and early returns. This might require additional instrumentation in the error handling sections. [medium]
relevant line	metrics::inc_executor_transact_gas(tx_execution.execution().gas.as_u64() as usize, true, function);

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Improve error handling by replacing unwrap() with safer alternatives Replace the multiple unwrap() calls with a safer error handling approach to prevent potential panics if any of the operations fail. build.rs [41] -let prefix = signature_file.file_name().unwrap().to_str().unwrap().split('.').next().unwrap(); +let prefix = signature_file.file_name().ok_or("Missing file name")?.to_str().ok_or("Invalid UTF-8 in file name")?.split('.').next().ok_or("Missing prefix in file name")?; Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a potential source of runtime panics due to multiple unwrap() calls, which is a significant issue in Rust programming. Implementing safer error handling is crucial for robustness.	8
	Add error handling for file names without a '.' character Consider handling the case where the file name does not contain a '.' character, which would cause next() to return None and subsequently unwrap() to panic. build.rs [41] -let prefix = signature_file.file_name().unwrap().to_str().unwrap().split('.').next().unwrap(); +let file_name = signature_file.file_name().ok_or("Missing file name")?.to_str().ok_or("Invalid UTF-8 in file name")?; +let prefix = file_name.split('.').next().ok_or("File name does not contain a '.' character")?; Suggestion importance[1-10]: 8 Why: This suggestion addresses another potential panic scenario related to file name parsing, which is a valid concern in the context of the provided code. Proper error handling for this edge case enhances the code's safety and reliability.	8
	Ensure initialization of function variable before use Ensure that the function variable is available before using it in the metrics function to avoid potential use of uninitialized value. src/eth/executor.rs [193] -let function = evm_input.extract_function(); +let function = evm_input.extract_function().unwrap_or_else(|| "unknown_function".to_string()); Suggestion importance[1-10]: 7 Why: The suggestion improves the safety of the code by ensuring that the function variable is initialized before use. This prevents potential runtime errors in the metrics logging functionality.	7
Security	Add overflow checks to the From implementation to ensure safety Consider checking for potential overflow issues when converting from Cow<'static, str> to owned string in the From implementation, especially if the input might be unexpectedly large. src/infra/metrics.rs [271] -Some(str) => Self::Some(str.into_owned()), +Some(str) => { + if str.len() > MAX_LABEL_SIZE { + panic!("Label size exceeds maximum allowed"); + } + Self::Some(str.into_owned()) +} Suggestion importance[1-10]: 8 Why: Adding overflow checks is crucial for security and stability, especially in a metrics context where unexpected large inputs could cause issues.	8
Possible issue	Add default values for labels in metric definitions to prevent runtime issues Consider adding a default case for the histogram_duration and histogram_counter macros to ensure that they handle cases where function or success labels might not be provided, potentially leading to runtime errors or misclassifications in metrics. src/infra/metrics.rs [178] -histogram_duration executor_external_transaction{function} [], +histogram_duration executor_external_transaction{function="unknown"} [], Suggestion importance[1-10]: 7 Why: Adding default values for labels in metric definitions is a good practice to prevent runtime issues, but the suggestion does not reflect a critical bug fix.	7
Maintainability	Add documentation to the new From implementation for clarity Add documentation comments to the newly added From implementation for LabelValue to explain its purpose and usage, enhancing code maintainability and readability. src/infra/metrics.rs [268] +/// Converts an `Option<Cow<'static, str>>` into a `LabelValue`. +/// This is used to handle optional labels in metric counters. impl From<Option<Cow<'static, str>>> for LabelValue { Suggestion importance[1-10]: 7 Why: Adding documentation enhances maintainability and readability, which is beneficial, though not critical.	7
Enhancement	Improve error messages for better debugging Use a more descriptive error message in the expect call to aid in debugging if the conversion fails. src/eth/executor.rs [79] -signatures_4_bytes.entry(id.try_into().unwrap(), &signature); +signatures_4_bytes.entry(id.try_into().expect("Failed to convert id to required 4 byte format"), &signature); Suggestion importance[1-10]: 6 Why: Enhancing error messages is beneficial for debugging, although it's a relatively minor improvement compared to the other suggestions. The suggestion is correctly targeted at a specific line where error handling could be more informative.	6
Performance	Optimize the From implementation by using direct string references if applicable Ensure that the Cow type used in the From implementation for LabelValue is necessary for the intended use case. If string literals are the primary use, using &'static str directly might be more efficient. src/infra/metrics.rs [268] -impl From<Option<Cow<'static, str>>> for LabelValue { +impl From<Option<&'static str>> for LabelValue { Suggestion importance[1-10]: 6 Why: The suggestion to optimize the From implementation by using direct string references is valid for performance, but it's not clear if Cow was used for specific reasons that might justify its use.	6