feat: Make use of new "Async" RPC #13
Conversation
WalkthroughThe pull request introduces several updates, including an upgrade of the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #13 +/- ##
============================================
- Coverage 80.15% 80.00% -0.16%
- Complexity 52 54 +2
============================================
Files 6 6
Lines 126 130 +4
============================================
+ Hits 101 104 +3
- Misses 25 26 +1 ☔ View full report in Codecov by Sentry. |
|
@L3tum Could you please fix the test here as well? |
|
Hey @rustatian , so sorry for the late update, I didn't expect there to be an issue here and even forgot it wasn't merged yet^^ |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/AbstractMetrics.php (1)
5-5: Consider removing the unused import statement.The
RPCInterfaceis imported but not used in this class. While it doesn't cause any issues, it's a good practice to remove unused imports for better code clarity.Apply this diff to remove the unused import:
-use Spiral\Goridge\RPC\RPCInterface; - abstract class AbstractMetrics implements MetricsInterface {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- composer.json (1 hunks)
- src/AbstractMetrics.php (1 hunks)
- src/Metrics.php (6 hunks)
- src/MetricsFactory.php (2 hunks)
- src/MetricsIgnoreResponse.php (1 hunks)
- src/MetricsOptions.php (1 hunks)
- tests/Unit/MetricsFactoryTest.php (2 hunks)
- tests/Unit/MetricsIgnoreResponseTest.php (1 hunks)
- tests/Unit/MetricsTest.php (6 hunks)
Additional comments not posted (40)
src/AbstractMetrics.php (1)
7-12: LGTM!The abstract class provides a solid foundation for implementing the metrics functionality. The
SERVICE_NAMEconstant is a good addition for centralizing the service identifier.src/MetricsOptions.php (2)
8-11: LGTM!The updated constructor documentation provides clear descriptions of all parameters, including the new ones. The type constraints for
$retryAttemptsand$retrySleepMicrosecondsimprove the clarity of the expected values.
17-17: LGTM!The new
$ignoreResponsesWherePossibleparameter in the constructor signature aligns with the updated documentation and provides more control over the behavior of theMetricsOptionsclass during metric collection. The default value offalseensures backward compatibility, as the new functionality is opt-in.src/MetricsFactory.php (5)
7-7: LGTM!The import statement for
AsyncRPCInterfaceis necessary to support the changes made in thecreatemethod.
17-37: LGTM!The changes made to the
createmethod introduce several improvements:
- More flexible handling of metrics creation based on the provided options.
- Improved robustness through conditional checks and warnings for the interaction between the
ignoreResponsesWherePossibleoption and the type of RPC interface used.- Granular control over the metrics behavior by instantiating different metrics objects based on the
ignoreResponsesWherePossibleoption.- Seamless incorporation of retry logic through the application of the
RetryMetricswrapper based on theretryAttemptsoption.The logic and syntax of the changes are correct and well-structured.
45-53: LGTM!The addition of the
createMetricsstatic method is a great enhancement to the usability of theMetricsFactoryclass. It provides a convenient way to instantiate the factory and create metrics with the specified options and logger in a single method call.The method is well-structured, and there are no apparent issues with its logic or syntax.
19-23: LGTM!The conditional checks and warnings added to the
createmethod improve its robustness by handling the interaction between theignoreResponsesWherePossibleoption and the type of RPC interface used.The warnings provide valuable information to the user about potential mismatches between the options and the RPC interface, helping to identify and address any inconsistencies.
The checks ensure that the appropriate metrics object is instantiated based on the
ignoreResponsesWherePossibleoption and the type of RPC interface, ensuring the correct behavior of the metrics.The logic and syntax of the checks and warnings are correct and well-structured.
31-37: LGTM!The application of the
RetryMetricswrapper based on theretryAttemptsoption is a great way to incorporate retry logic seamlessly into the metrics creation process.The wrapper is applied conditionally, ensuring that it is only used when retry attempts are specified. It takes the existing metrics object, the number of retry attempts, and the retry sleep duration as arguments, providing flexibility in configuring the retry behavior.
The logic and syntax of the wrapper application are correct and well-structured.
composer.json (1)
48-48: Upgrade tospiral/goridgev4.2 looks good!The version constraint update for the
spiral/goridgepackage from^4.0to^4.2is a sensible change. This upgrade could potentially bring in new features, improvements, or bug fixes from the newer version of the library.However, it's crucial to ensure that:
- The new version is compatible with the rest of the application and doesn't introduce any breaking changes.
- The application has been thoroughly tested with the new version to confirm that everything works as expected.
To verify the compatibility and identify any potential issues, consider running the following checks:
If all the tests pass and there are no compatibility issues or unexpected behavior, then the upgrade can be considered safe.
Verification successful
Upgrade to
spiral/goridgev4.2 is verified and looks good!The codebase analysis confirms that the
spiral/goridgepackage is extensively used throughout the project, particularly for RPC functionality. The update from ^4.0 to ^4.2 is a minor version bump, which should maintain backward compatibility.Key points:
- The package is well-integrated, using both synchronous (RPCInterface) and asynchronous (AsyncRPCInterface) RPC methods.
- Extensive use of interfaces suggests good abstraction, which helps with version compatibility.
- The project includes unit tests and static analysis tools, which can help catch potential issues.
Recommendation:
Before merging, ensure to run the existing test suite and static analysis:composer run tests composer run psalmIf these pass successfully, the upgrade can be considered safe to proceed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Install the updated dependencies composer update # Run the test suite composer run tests # Run static analysis composer run psalm # Search for usage of new features from spiral/goridge v4.2 rg --type php -A 5 $'spiral/goridge'Length of output: 324
Script:
#!/bin/bash # Search for potential imports or use statements of spiral/goridge rg --type php -i '(use|import).*spiral.*goridge' # Search for potential class names from spiral/goridge package rg --type php -i '(Relay|RPC|SocketRelay|StreamRelay|SocketRelayInterface|CodecInterface)' # Search for configuration files that might reference spiral/goridge rg -g '*.{php,yaml,yml,json,xml}' -i 'spiral.*goridge' # List all PHP files to get an overview of the project structure fd -e php # Check composer.json for any scripts or config related to spiral/goridge cat composer.json | jq '.scripts, .config'Length of output: 12135
src/Metrics.php (7)
7-12: LGTM!The imports are relevant and necessary for the changes made in the class.
33-33: Verify the impact of the RPC method name change.The
submethod now callsmetrics.SubRPC method instead ofMetrics.Sub. This change likely reflects a reorganization of the RPC service methods to better categorize or namespace them under "metrics". However, it might affect other parts of the codebase that depend on theMetrics.Submethod.Run the following script to verify the usage of
Metrics.Subin the codebase:Verification successful
Verification complete: RPC method name change consistently applied
The change from 'Metrics.Sub' to 'metrics.Sub' has been consistently implemented across the codebase. No remaining instances of 'Metrics.Sub' were found, and the new 'metrics.Sub' is correctly used in both implementation and test files. This change appears to be intentional and thoroughly applied, with no apparent issues or inconsistencies.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Metrics.Sub` in the codebase. # Test: Search for the usage of `Metrics.Sub`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'Metrics\.Sub'Length of output: 35
Script:
#!/bin/bash # Description: Comprehensive search for RPC method usage related to 'Metrics.Sub' and 'metrics.Sub' # Search for 'Metrics.Sub' without escaping the dot echo "Searching for 'Metrics.Sub':" rg --type php -i 'Metrics.Sub' # Search for 'metrics.Sub' echo -e "\nSearching for 'metrics.Sub':" rg --type php -i 'metrics.Sub' # Search for 'Sub' in the context of RPC calls echo -e "\nSearching for 'Sub' in RPC call context:" rg --type php -i "call\(.*Sub"Length of output: 1416
24-24: Verify the impact of the RPC method name change.The
addmethod now callsmetrics.AddRPC method instead ofMetrics.Add. This change likely reflects a reorganization of the RPC service methods to better categorize or namespace them under "metrics". However, it might affect other parts of the codebase that depend on theMetrics.Addmethod.Run the following script to verify the usage of
Metrics.Addin the codebase:Verification successful
Change to 'metrics.Add' has been consistently applied
The modification from 'Metrics.Add' to 'metrics.Add' has been consistently implemented across the codebase. This includes both the main source files (src/Metrics.php and src/MetricsIgnoreResponse.php) and the corresponding test files. No occurrences of the old 'Metrics.Add' method were found, indicating that the change has been thoroughly applied.
The consistency of this change suggests that it was an intentional refactoring, likely aimed at improving the organization or naming conventions of the RPC methods. No issues or inconsistencies were identified during this verification process.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Metrics.Add` in the codebase. # Test: Search for the usage of `Metrics.Add`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'Metrics\.Add'Length of output: 35
Script:
#!/bin/bash # Description: Perform a comprehensive search for RPC method usage related to metrics # Search for 'Metrics.Add' (case-sensitive) echo "Searching for 'Metrics.Add':" rg --type php -i 'Metrics\.Add' # Search for 'metrics.Add' (case-insensitive to catch potential variations) echo "Searching for 'metrics.Add' (case-insensitive):" rg --type php -i 'metrics\.add' # Search for any 'Add' method related to metrics echo "Searching for any 'Add' method related to metrics:" rg --type php -i '(metrics?|stat).*\.add' # Look for RPC configuration files echo "Searching for RPC configuration files:" fd -e php -e yaml -e json -e xml | grep -i 'rpc' # If any RPC config files are found, display their content echo "Content of RPC configuration files (if any):" fd -e php -e yaml -e json -e xml | grep -i 'rpc' | xargs -I {} sh -c 'echo "File: {}"; cat "{}"'Length of output: 2201
42-42: Verify the impact of the RPC method name change.The
observemethod now callsmetrics.ObserveRPC method instead ofMetrics.Observe. This change likely reflects a reorganization of the RPC service methods to better categorize or namespace them under "metrics". However, it might affect other parts of the codebase that depend on theMetrics.Observemethod.Run the following script to verify the usage of
Metrics.Observein the codebase:Verification successful
Verified: RPC method name change is consistent and properly implemented.
The change from 'Metrics.Observe' to 'metrics.Observe' has been consistently applied throughout the codebase. This modification appears to be part of a broader standardization of RPC method names, with all metrics-related RPC calls now using the 'metrics.' prefix (e.g., 'metrics.Add', 'metrics.Sub', 'metrics.Set').
The new method name is being used correctly in both Metrics.php and MetricsIgnoreResponse.php, and there are no remnants of the old 'Metrics.Observe' naming. The 'Metrics' class and namespace are still in use, indicating that only the RPC method names were changed, not the overall structure of the metrics system.
Based on this analysis, the change has been implemented consistently and doesn't appear to have any negative impacts on the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Metrics.Observe` in the codebase. # Test: Search for the usage of `Metrics.Observe`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'Metrics\.Observe'Length of output: 39
Script:
#!/bin/bash # Description: Verify the usage of both old and new RPC method names and check search functionality. # Test 1: Verify if the search command is working by searching for a known string 'rpc->call' echo "Test 1: Searching for 'rpc->call'" rg --type php -A 2 'rpc->call' # Test 2: Search for the new method name 'metrics.Observe' echo "Test 2: Searching for 'metrics.Observe'" rg --type php -A 2 'metrics\.Observe' # Test 3: Search for partial matches 'Observe' in the context of RPC calls echo "Test 3: Searching for 'Observe' in RPC calls" rg --type php -A 2 'rpc->call\([^)]*Observe' # Test 4: Search for 'Metrics' class or namespace usage echo "Test 4: Searching for 'Metrics' class or namespace" rg --type php -A 2 '\bMetrics\b'Length of output: 12981
51-51: Verify the impact of the RPC method name change.The
setmethod now callsmetrics.SetRPC method instead ofMetrics.Set. This change likely reflects a reorganization of the RPC service methods to better categorize or namespace them under "metrics". However, it might affect other parts of the codebase that depend on theMetrics.Setmethod.Run the following script to verify the usage of
Metrics.Setin the codebase:Verification successful
Change to 'metrics.Set' is consistent across the codebase
The modification from 'Metrics.Set' to 'metrics.Set' has been uniformly applied throughout the codebase. All relevant files, including tests and related classes, have been updated to use the new method name. No occurrences of the old 'Metrics.Set' were found, indicating a thorough implementation of the change.
- Updated files:
- src/Metrics.php
- src/MetricsIgnoreResponse.php
- tests/Unit/MetricsTest.php
- tests/Unit/MetricsIgnoreResponseTest.php
The impact of this change appears to be limited to Metrics-related classes and their corresponding tests, with no unexpected usage found elsewhere in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Metrics.Set` in the codebase. # Test: Search for the usage of `Metrics.Set`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'Metrics\.Set'Length of output: 35
Script:
#!/bin/bash # Description: Comprehensive search for 'Metrics.Set' and 'metrics.Set' usage in the codebase. # Search for 'Metrics.Set' without dot escape echo "Searching for 'Metrics.Set':" rg --type php -i 'Metrics.Set' echo -e "\nSearching for 'metrics.Set':" rg --type php -i 'metrics.Set' echo -e "\nSearching for string literals containing 'Metrics.Set' or 'metrics.Set':" rg --type php -i "'(Metrics\.Set|metrics\.Set)'" -g '!src/Metrics.php' rg --type php -i '"(Metrics\.Set|metrics\.Set)"' -g '!src/Metrics.php'Length of output: 1896
60-65: Verify the impact of the RPC method name changes.The
declareandunregistermethods now callmetrics.Declareandmetrics.UnregisterRPC methods respectively, instead ofMetrics.DeclareandMetrics.Unregister. These changes likely reflect a reorganization of the RPC service methods to better categorize or namespace them under "metrics". However, they might affect other parts of the codebase that depend on theMetrics.DeclareandMetrics.Unregistermethods.Run the following script to verify the usage of
Metrics.DeclareandMetrics.Unregisterin the codebase:Also applies to: 77-77
Verification successful
Changes to RPC method names have been applied consistently.
The verification process confirms that the RPC method name changes from 'Metrics.Declare' and 'Metrics.Unregister' to 'metrics.Declare' and 'metrics.Unregister' have been applied consistently throughout the codebase. All metrics-related RPC calls now use the 'metrics.' prefix, maintaining a uniform naming convention. The changes are reflected in the main implementation files (src/Metrics.php, src/MetricsIgnoreResponse.php) as well as in the test files, indicating proper test coverage for these modifications.
No instances of the old method names were found, suggesting a thorough update process. The 'declare' and 'unregister' methods are implemented in various classes (SuppressExceptionsMetrics, RetryMetrics, Metrics, MetricsIgnoreResponse), which indicates a well-structured and flexible design that accommodates these changes.
Based on this analysis, it appears that the changes have been implemented correctly and consistently, with no apparent issues or inconsistencies in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Metrics.Declare` and `Metrics.Unregister` in the codebase. # Test: Search for the usage of `Metrics.Declare`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'Metrics\.Declare' # Test: Search for the usage of `Metrics.Unregister`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'Metrics\.Unregister'Length of output: 82
Script:
#!/bin/bash # Description: Verify the usage of new and old Metrics RPC method names in the codebase. # Search for new method names (case-insensitive) echo "Searching for new method names:" rg -i --type php -A 5 'metrics\.(declare|unregister)' # Search for any Metrics related RPC calls echo "Searching for Metrics related RPC calls:" rg --type php -A 5 '\b(metrics?|Metrics?)\.(\w+)' # Check for declare or unregister methods within Metrics class or related files echo "Searching for declare or unregister methods in Metrics related files:" rg --type php -A 5 '(declare|unregister).*\(' $(rg -l 'class.*Metrics' --type php)Length of output: 22233
14-18: Verify the impact of the class inheritance change.The
Metricsclass now extendsAbstractMetricsinstead of implementingMetricsInterface. This change indicates a shift towards a more structured inheritance model. However, it might affect other parts of the codebase that depend on theMetricsInterface.Run the following script to verify the usage of
MetricsInterfacein the codebase:Verification successful
To ensure that the change in the
Metricsclass doesn't introduce any compatibility issues, we need to verify two things:
- The
Metricsclass still implements all methods required byMetricsInterface.- There are no type hints in the codebase that specifically require
MetricsInterfaceand not a class implementing it.Let's run some additional scripts to gather this information:
It seems our previous attempt didn't yield the expected results. Let's try a different approach to gather the necessary information:
This script should help us locate the relevant files, display their contents, and search for specific patterns within them. This will give us a clearer picture of the current state of the
Metricsclass andMetricsInterface, allowing us to better assess the impact of the changes.
Verification successful: Class inheritance change is compatible
The change in the
Metricsclass from implementingMetricsInterfaceto extendingAbstractMetricsdoes not introduce any compatibility issues. All methods defined inMetricsInterfaceare properly implemented in theMetricsclass. The inheritance change maintains the interface contract, ensuring that existing code depending onMetricsInterfacewill continue to work as expected.
- The
Metricsclass insrc/Metrics.phpextendsAbstractMetricsand implements all required methods.- The
MetricsInterfaceinsrc/MetricsInterface.phpdefines the expected methods.- No direct usage of
MetricsInterfacetype hints were found, reducing the likelihood of compatibility issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `MetricsInterface` in the codebase. # Test: Search for the usage of `MetricsInterface`. Expect: No occurrences outside of the `Metrics` class. rg --type php -A 5 $'MetricsInterface'Length of output: 3711
Script:
#!/bin/bash # Description: Verify Metrics class implementation and MetricsInterface usage # Test 1: Check if Metrics class implements all methods from MetricsInterface echo "=== Metrics class methods ===" ast-grep --lang php --pattern 'class Metrics extends AbstractMetrics { $$$1 }' echo -e "\n=== MetricsInterface methods ===" ast-grep --lang php --pattern 'interface MetricsInterface { $$$1 }' # Test 2: Check for type hints specifically requiring MetricsInterface echo -e "\n=== Type hints for MetricsInterface ===" rg --type php -A 2 -B 2 '\b(function|method).*\(.*MetricsInterface\s+\$'Length of output: 470
Script:
#!/bin/bash # Description: Locate and analyze Metrics and MetricsInterface files # Find relevant PHP files echo "=== Searching for Metrics and MetricsInterface files ===" fd -e php -t f '(Metrics|MetricsInterface)\.php$' # Display contents of Metrics.php if found echo -e "\n=== Contents of Metrics.php ===" fd -e php -t f '^Metrics\.php$' -x cat {} # Display contents of MetricsInterface.php if found echo -e "\n=== Contents of MetricsInterface.php ===" fd -e php -t f '^MetricsInterface\.php$' -x cat {} # Search for method definitions in Metrics class echo -e "\n=== Methods in Metrics class ===" rg -U --type php 'class Metrics.*?\{.*?public function.*?\}' -A 10 # Search for method definitions in MetricsInterface echo -e "\n=== Methods in MetricsInterface ===" rg -U --type php 'interface MetricsInterface.*?\{.*?public function.*?\}' -A 10 # Search for type hints using MetricsInterface echo -e "\n=== Type hints for MetricsInterface ===" rg --type php -A 2 -B 2 '\b(function|method).*\(.*MetricsInterface\s+\$'Length of output: 5315
src/MetricsIgnoreResponse.php (6)
16-23: LGTM!The method logic is correct, and the implementation is accurate. It correctly uses the
callIgnoreResponsemethod to add a value to a metric and handles theServiceExceptionby rethrowing it asMetricsException.
25-32: LGTM!The method logic is correct, and the implementation is accurate. It correctly uses the
callIgnoreResponsemethod to subtract a value from a metric and handles theServiceExceptionby rethrowing it asMetricsException.
34-41: LGTM!The method logic is correct, and the implementation is accurate. It correctly uses the
callIgnoreResponsemethod to observe a value for a metric and handles theServiceExceptionby rethrowing it asMetricsException.
43-50: LGTM!The method logic is correct, and the implementation is accurate. It correctly uses the
callIgnoreResponsemethod to set a value for a metric and handles theServiceExceptionby rethrowing it asMetricsException.
52-67: LGTM!The method logic is correct, and the implementation is accurate. It correctly uses the
callmethod to declare a new metric collector and handles theServiceExceptionby suppressing the error if it's a duplicate metric error and rethrowing other errors asMetricsException.
69-76: LGTM!The method logic is correct, and the implementation is accurate. It correctly uses the
callmethod to unregister a metric and handles theServiceExceptionby rethrowing it asMetricsException.tests/Unit/MetricsIgnoreResponseTest.php (9)
28-35: LGTM!The test method is correctly verifying the behavior of the
addmethod ofMetricsIgnoreResponseclass.
37-44: LGTM!The test method is correctly verifying the behavior of the
submethod ofMetricsIgnoreResponseclass.
46-53: LGTM!The test method is correctly verifying the behavior of the
observemethod ofMetricsIgnoreResponseclass.
55-62: LGTM!The test method is correctly verifying the behavior of the
setmethod ofMetricsIgnoreResponseclass.
64-77: LGTM!The test method is correctly verifying the behavior of the
declaremethod ofMetricsIgnoreResponseclass.
79-95: LGTM!The test method is correctly verifying the error handling of the
declaremethod ofMetricsIgnoreResponseclass.
97-109: LGTM!The test method is correctly verifying the suppression of certain errors in the
declaremethod ofMetricsIgnoreResponseclass.
111-119: LGTM!The test method is correctly verifying the behavior of the
unregistermethod ofMetricsIgnoreResponseclass.
121-134: LGTM!The test method is correctly verifying the error handling of the
unregistermethod ofMetricsIgnoreResponseclass.tests/Unit/MetricsFactoryTest.php (4)
21-28: LGTM!The changes to the
testCreatemethod look good. The additional$rpcInterfaceClassparameter allows for more flexible testing with different RPC interface implementations.
31-40: Great addition!The new
testCreateStaticmethod is a valuable addition to the test suite. It ensures that the staticcreateMetricsmethod ofMetricsFactorybehaves as expected with the provided options and RPC interface.
42-52: Excellent test case!The new
testLogsIfIgnoreResponseButNoAsyncRPCInterfacemethod is a great addition to the test suite. It verifies that a warning is logged when theignoreResponsesWherePossibleoption is set to true without an accompanyingAsyncRPCInterface. This helps ensure that the logging behavior ofMetricsFactoryis correctly validated.
54-64: Another great test case!The new
testLogsIfAsyncRPCInterfaceButNoIgnoreResponsesmethod is another valuable addition to the test suite. It verifies that a warning is logged when anAsyncRPCInterfaceis provided but theignoreResponsesWherePossibleoption is false. This helps ensure that the logging behavior ofMetricsFactoryis correctly validated under different configurations.tests/Unit/MetricsTest.php (5)
31-31: LGTM!The change to include the "metrics" service prefix in the RPC method name is consistent with the overall modifications made to the test file and enhances the specificity of the method call.
56-56: LGTM!The change to include the "metrics" service prefix in the RPC method name is consistent with the overall modifications made to the test file and enhances the specificity of the method call.
81-81: LGTM!The change to include the "metrics" service prefix in the RPC method name is consistent with the overall modifications made to the test file and enhances the specificity of the method call.
106-106: LGTM!The change to include the "metrics" service prefix in the RPC method name is consistent with the overall modifications made to the test file and enhances the specificity of the method call.
136-136: LGTM!The changes to include the "metrics" service prefix in the RPC method names for both the
testDeclareandtestUnregistermethods are consistent with the overall modifications made to the test file and enhance the specificity of the method calls.Also applies to: 178-178
|
@L3tum Thanks for the work you've done! |
This is a follow-up PR for roadrunner-php/goridge#22 implementing the new "async" RPC calls in the Metrics class.
It may be an option to instead modify theMetricsFactoryto return anAsyncMetricsclass instead if it detects an async RPC interface, or something like that. Please lmk if you'd like me to change that. Once the implementation is good to go I'll add tests for it as well.EDIT: I've switched to the
MetricsFactorybecause implementing it into our service was kinda weird otherwise. I've also added a few tests and fixed the Psalm complaint.For further info please read the linked PR in the goridge repo.
Same PR in the KV repo: roadrunner-php/kv#36
Summary by CodeRabbit
New Features
AbstractMetricsclass for structured metrics functionality.MetricsIgnoreResponseclass to handle metrics operations while ignoring RPC responses.MetricsOptionsfor improved configurability during metric collection.Improvements
Metricsclass to streamline method calls and enhance error handling.MetricsFactoryto support flexible metrics creation with new options.Tests
MetricsFactoryandMetricsIgnoreResponseclasses to ensure robust functionality.