Release 10.18 #840
Merged
Release 10.18 #840
Conversation
This file contains 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
…server API (#501) * feat(agent): Registered function begin/end and error handlers with Observer API 1) Registered function begin/end handlers with Observer API 2) Created the function begin/end handler stubs. Full functionality is schedule for another ticket. 3) Registered currently existing error handler with Observer API 4) created php_observer.c/h module to contain the observer api logic. Testing: 1) Verified new function begin/end handlers were registered correctly and received zend_execute_data from PHP engine. 2) Current test cases verified that registering our current error handler directly caused no change in functionality. Additional * feat(agent): Don't call handlers for internal functions. Currently internal functions are not handled by OAPI, but they will be in 8.2. These functions are tailored to USER functions (similar to nr_php_execute) and we don't want internal functions filtered to these handlers. This will default to INTERNAL functions being handled by the current implementation. To change in the future, it's possible we'd need to implement handlers specific for internal functions (similar to nr_php_execute_internal).
…ta. (#505) * feat(agent): Add/Update functions to utilize the OAPI zend_execute_data. 1) Added 4 new functions for use with OAPI instrumentation: extern const char* nr_php_zend_execute_data_function_name( const zend_execute_data* execute_data); extern const char* nr_php_zend_execute_data_filename( const zend_execute_data* execute_data); extern const char* nr_php_zend_execute_data_scope_name( const zend_execute_data* execute_data); extern uint32_t nr_php_zend_execute_data_lineno( const zend_execute_data* execute_data); 2) Added test cases to test new functionality. 3) Updated the following to reflect new OAPI paradigm and add additional checks for robustness: * #define NR_PHP_USER_FN_THIS() * nr_php_execute_scope 3) Added unit test cases to test new functions 4) Also verified functionality via integration tests. * refactor(agent): If called correctly, new functions should work with PHP 7+.
…ctions. (#516) * feat(agent): Propagate OAPI return values and update return value functions. 1) updated macros to pass the OAPI given return value throughout the userland system. 2) changed nr_php_get_return_value to return oapi given pointer 3) Added the OVERWRITE_ZEND_EXECUTE_DATA to allow us to toggle off during feature addition, but toggle off to maintain CI as long as possible. It also gives the flexibility to revert to the instrumentation prior to OAPI. 4) macro to toggle between just using the OAPI return value when OAPI is enabled or calling nr_php_get_return_value_ptr when it is not OAPI.
…d zed. (#517) * feat(agent): Propagate OAPI return values and update return value functions. 1) updated macros to pass the OAPI given return value throughout the userland system. 2) changed nr_php_get_return_value to return oapi given pointer 3) Added the OVERWRITE_ZEND_EXECUTE_DATA to allow us to toggle off during feature addition, but toggle off to maintain CI as long as possible. It also gives the flexibility to revert to the instrumentation prior to OAPI. 4) macro to toggle between just using the OAPI return value when OAPI is enabled or calling nr_php_get_return_value_ptr when it is not OAPI.
* feat(agent): Add code level metrics functionality. 1) Add INI value to disable/enable code level metrics. 2) Create new function `nr_php_txn_add_code_level_metrics` that uses nr_php_zend_execute_data_* family of functions to extract CLM and add it as an `agent attribute`. 3) Added new NR_TXN_ATTRIBUTES for `code.namespace`, `code.lineno`, `code.filepath`, and `code.function`. 4) Added call to php_execute_enabled to call the new CLM function 4) Added new integration tests to exercise the new functionality. Note: CLM functionality is only compatible with PHP 7+. * feat(agent): Code level metrics functionality. 1) updated `nr_php_execute_metadata_t` to hold additional information and moved the definition of the struct to php_execute.h. 2) `nr_php_execute_enabled` uses the metadata at the beginning so now we can simply release at the end. 3) In the case of a super short segment that would have been ignored, we do NOT add CLM (otherwise it wouldn't be ignored). We wait until after we decide to ignore or not to add the CLM data. * feat(agent): Updated test scripts. * fix(agent): check clm string lengths will not be truncated * fix(agent): check for empty/null CLM attributes * style(agent): clang-format php_execute * fix(agent): fix logic handling CLM string length * refactor(agent): abstract out CLM while-loop code for readability
Features: * implement user functions instrumentation via PHP's Observer API * remove function hooks that overwrite `zend_execute_ex` for user functions * add `special_instrumentation_before` callback to `nruserfn_t` for user functions that need instrumentation to happen before the function executes. * add `nr_php_wrap_user_function_before_after` to install Observer API's before and after function callbacks (to be used in lib_*.c and/or fw_*.c) * add `nr_zend_call_oapi_special_before` to call Observer APIs before function callback (to be used in `observer_fcall_begin` handler). * store txn_start_time in a segments to so that it is available in `observer_fcall_end` handler to verify and end the segment * add reportedclass to nruserfn_t to account for functions existing in one class table while reporting they belong to another class (details commented in code). Refactorings: * add `nr_php_wraprec_matches` helper function * add `wraprec` to segments to only make the call to get_wraprec_by_name once (in the `observer_fcall_begin` handler and not again in the `observer_fcall_end` handler) Tests: * update unit tests to test new features and changed functionality
* feat(agent): implement php_execute_show functionality for OAPI * Update agent/php_execute.c Co-authored-by: Michal Nowacki <[email protected]> * style(agent): PR feedback, remove comment * Update agent/php_execute.c Co-authored-by: Michal Nowacki <[email protected]>
Framework detection (nr_execute_handle_framework) not only adds wraprecs but also sets NRPRG(current_framework). It is important to set it before function is called because its value affects other instrumentation, e.g. when datastore segment for SQL operation is ended, a table name modify function (nr_php_modify_table_name_fn) to shorten table name is selected based on NRPRG(current_framework). This fixes frameworks/magento/test_temp_tables.php and frameworks/wordpress/test_site_specific_tables.php integration tests.
…563) * fix(agent): fix check for max strlen when generating clm attributes * style(tests): follow php function naming convention (camelCase)
Fix `drupal_http_request` instrumentation for PHP 8.0+ by using Observer APIs `before` callback to add New Relic headers and `after` callback to finalize external segment with metrics. Observer API instrumentation requires the external request segment to be created in `before` callback but available in `after` callback. Therefore it is made a NRPRG global.
1. Renamed nr_php_execute_metadata_add_code_level_metrics to nr_php_observer_metadata_init as this will be the way we populate the metadata. We don't need to do duplicate effort with `nr_php_execute_metadata_init` anymore. 2. Since nr_php_execute_metadata_init is now for populating metadata, moved all CLM checks out nr_php_observer_metadata_init and into nr_php_txn_add_code_level_metrics where it is more appropriate. 3. Modified stacked segments to have a pointer to the metadata so if they are closed off by an exception, they will properly propogate the information. Added initializations/deninitializations. 4. For php_execute_enabled, only call nr_php_observer_metadata_init if we need to. 5. Added nr_php_observer_exception_segment_end To handled closing off observer segments if an exception occurs. 6. Now use zend_throw_exception_hook (more info: https://www.phpinternalsbook.com/php7/extensions_design/hooks.html Note: This ONLY notifies when an exception is thrown. It gives no indication if that exception was subsequently caught or not. 8. OAPI leaves dangling segments in the case of exceptions. These need to be cleaned up for functions that rely on the current segment (includes begin/end functions, stacked segment unwinding, and API calls) 9. New inline function re`nr_php_api_ensure_current_segment` to account for dangling segments when calling our API. 10. TXN globals to keep track of the exception 11. Change in php_txn turn off recording after unwind the segments to give timer to attach exception to dangling segment(s). 12. Modify stacked segments init/denit to handle additional segment metadata variable. 13. Functions to clear/set TXN uncaught_exception variables. 14. Update metadata struct to retain more context of the segment. 15. Removed legacy exception code that wasn't getting called anymore. 16. Added tests. 17. Added a `clean`callback to the wraprec functionality and associated unit tests. With OAPI exceptions, the registered function handler (nr_php_observer_fcall_end) doesn't get called when an uncaught exception occurs, and therefore doesn't decrement the stack_depth counter. All OAPI unhandled exception cleanup filters through: nr_php_observer_segment_end so we decrement php_cur_stack_depth there when we cleanup orphaned segments. Additionally, since nr_php_observer_segment_end is an exit path, also call nr_php_show_oapi_metadata New function nr_php_show_oapi_metadata called via the segment exception handling exit path (to correspond to nr_php_show_exec_return) to show the all the available function details when the special_flags.show_execute_* is toggled on. This will help when debugging. Added additional test cases to ensure proper php_cur_stack_depth counting
…tack when using OAPI (#582) Symfony1 instrumentation utilizes the call stack, but should never be run with PHP8+.
Occasionally, we want to instrument functions called with call_user_func_array, when we otherwise wouldn't instrument that function. To do this, we instrument the internal function call_user_func_array and check for contexts in which we want to begin instrumentation. However, call_user_func_array can be inlined by the zend compiler. Previously, we were detecting inlined calls by overwriting the DO_FCALL opcode. With OAPI, we no longer want to touch the opcodes and are instead checking for the inlined calls during observer_do_fcall.
Consistent with other functions in php_execute, check if txn exists before trying to use it. The issue occurred during the first request, and because it is the first, the appname is unknown and we don't create a txn. However, an exception happened in that first request, our exception_hook handler got triggered, and if that is triggered, we always try to close off dangling segments. In this case, because we didn't check for txn==null, it segfaulted as we tried to access txn elements. Pr fixes that in two ways, 1) check for txn==null before closing off segments. 2) don't record uncaught exception info generated from our exception_hook handler if a txn is null. ``` 2023-01-26 21:03:16.816 +0000 (29813 29813) verbosedebug: RINIT processing started 2023-01-26 21:03:16.816 +0000 (29813 29813) debug: added app='PHP Test Apps Laravel ads v2' license='07...4a' 2023-01-26 21:03:16.816 +0000 (29813 29813) verbosedebug: querying app='PHP Test Apps Laravel ads v2' from parent=4 2023-01-26 21:03:16.816 +0000 (29813 29813) verbosedebug: sending appinfo message, len=6492 2023-01-26 21:03:16.817 +0000 (29813 29813) debug: APPINFO reply unknown app='PHP Test Apps Laravel ads v2' 2023-01-26 21:03:16.817 +0000 (29813 29813) debug: unable to begin transaction: app 'PHP Test Apps Laravel ads v2' is unknown ``` note the last line: debug: unable to begin transaction: app 'PHP Test Apps Laravel ads v2' is unknown
…#601) move NR_GET_RETURN_VALUE_PTR before NR_PHP_WRAPPER_CALL Cleanup conditional compilation to be more readable
…oapi func_begin for txn naming. (#615) For OAPI, wrapping default makes all wrapped special functions execute during func_end. This caused issues since to customize txn naming per framework, PHP agent uses the order in which functions are processed, NR_NOT_OK_TO_OVERWRITE/NR_OK_TO_OVERWRITE, and whether it is called either before or after NR_PHP_WRAPPER_CALL (for pre PHP 8+) or whether it is called in func_begin or func_end (for PHP 8+ / OAPI). By defaulting to calling all callbacks "after" during func_end, it changed the order in which nr_txn_set_path was being set. To fix this, we need to verify which callback functions set the txn before NR_PHP_WRAPPER_CALL and which set it after, and have the wrapped functions act similarly to call before/after. This PR 1) reviewed all frameworks (exceptions listed below) and use of nr_txn_set_path on a case by case basis. 2) Explicitly commented to detail the naming schemes in use with each use of nr_txn_set_path in all frameworks 3) used the before/after/clean wrapping function as needed when txn naming needed to occur before function execution to match what is done in legacy instrumentation. 4) for functions not needing to be called before, comments were still added noting that it was being called by the default (after callback/func_end) Calling the wrapper special function by default before a function executes was a solution initially considered; however, after investigation/testing, it was deemed inappropriate for multiple reasons: 1)too invasive 2)some wrapped functions do not have all the values they need before a function is executed, for example functions that need the return value (I.e. all those special functions that continue to do things after `NR_PHP_WRAPPER_CALL` 3) Additionally, each framework has individualized, specialized ways of setting the txn name via `nr_txn_set_path`. As such, calling a wrapper function in func_end as default is still the most appropriate choice. These frameworks are either unsupported, do not support PHP 8+, or both and no change to the txn naming scheme: fw_kohana.c, fw_joomla.c, fw_magneto.c(magento 1.x), fw_silex.c, fw_symfony.c, fw_symfony2.c, fw_zend.c fw_zend2.c Unit tests that run on all versions of PHP ensure the txn naming behavior is identical.
Modify newly added oapi tests to conform with CLM on by default.
Removes support for ZTS installs in `newrelic-install.sh` as disables building ZTS artifacts. --------- Co-authored-by: Amber Sistla <[email protected]>
Empty files don't get executed by PHP 8.2 when the agent uses Observer API to hook into Zend engine, therefore mocks needs to do something - enhance them with a 'noop' statement - `echo "";`
A call to `zend_map_ptr_reset` in `zend_deactivate` triggers `Invalid read of size 8` valgrind error in test_redis during `tlib_php_request_eval_expr` and `nr_php_zval_free` in the last php request lifecycle when Observer API is used to hook into Zend engine. This error is triggered on all PHPs with this patch: php/php-src@ff62d11
When wrapping a user function do not add special instrumentation if ANY special instrumentation (before/after/cleanup) has already been set. This approach matches pre-oapi instrumentation.
In addition to OAPI Drupal 9.4 support, this PR does some refactoring: 1. The hook stacks have been renamed from `module_invoke_all` to `invoke_all` to be more aligned with the name of Drupal functions that can now push to those stacks. 2. The generic wrapper function (originally added when Drupal 9.4 instrumentation was introduced) has been updated to support OAPI. 3. `nr_php_wrap_callable_before_after_clean` has been introduced as a parallel for `nr_php_wrap_user_function_before_after_clean`. To facilitate this, many of the internals have been extracted to a common function. 4. Speaking of, lots of the maintenance of the Drupal's hook stacks has been extracted to common functions to reduce code reuse --------- Co-authored-by: Michal Nowacki <[email protected]>
PHP embed SAPI, used by unit tests, demonstrates memory issues when PHP code executed by way of `nr_php_call` or `tlib_php_request_eval` throws a PHP Exception. This functionality is best suited to be exercised via integration test.
After #798 there's no need to pass any wraprec options when wraprecs are created with nr_php_add_custom_tracer_named because it is no longer used to create different types of wraprecs. It always creates non-transient wraprecs and therefore always needs to generate instrumented function metrics.
After #768, filter hook's callback function instrumentation needs to create wordpress metrics when hook callback function throws an exception. Therefore its 'after' wrappers (after and clean) need to be set to do it on hook's callback function either normal return or when it returns via an exception.
After #798, the tests expect Drupal\page_cache\StackMiddleware\PageCache::get to be instrumented so that transient instrumentation will not get installed. But empty files don't get executed by PHP 8.2+ when the agent uses Observer API to hook into Zend engine, therefore mocks needs to do something in order for the non-transient instrumentation to be installed (classes and class' methods need to be available at the time non-transient instrumentation is installed). Enhance Drupal\page_cache\StackMiddleware\PageCache with a 'noop' statement - echo "";`
zend_try/zend_catch is to handle Zend Exceptions not PHP exceptions - see [here](https://github.com/newrelic/newrelic-php-agent/blob/320ea571a11bc469d7d8179dfe51577b54df11df/agent/php_user_instrument.c#L17-L50) for more details. They were added in this [commit](66eccf7) to handle misbehaving PHP embed SAPI that threw Zend Exception when PHP code that was called via nr_php_call threw PHP Exception. PHP CLI or CGI SAPIs don’t throw Zend Exception when PHP code that was called threw PHP Exception and therefore zend_try/zend_catch is effectively a dead code.
Removing the need to manual handling of dangling segments, because Zend calls all of the hooks we need. - adds a boolean argument to nr_php_error_record_exception to control whether we add the error to the current segment. This is needed because the OAPI context of when the above is called is no longer during a segment with an uncaught exception and was incorrectly adding the error to the root segment. - removes all language of "dangling segments". These no longer exist. Zend calls all of the necessary `fcall_end`'s, even when an exception is thrown. When this happens,` func_return_value` is a C `NULL` pointer which is distinct from a `NULL` (but valid) zval when there is no return value from a function. We use this `NULL` value to determine the presence of an uncaught exception. - no longer overwrites the exception hook; no longer stores a copy of exceptions locally - replace storing metadata in the segment, which was used to pair segments from func_begin with func_end, with logic that always creates segment in func_begin Mostly undoes the work of #580 --------- Co-authored-by: Michal Nowacki <[email protected]> Co-authored-by: bduranleau-nr <[email protected]> Co-authored-by: Amber Sistla <[email protected]>
Move exception handler instrumentation from fcall_begin to fcall_end. This requires a change of how we are obtaining the fields of the exception, because calling `nr_php_call` during the handling of an exception does not play nicely with PHP. --------- Co-authored-by: Michal Nowacki <[email protected]>
Because OAPI doesn't create a nice callstack that replicates the PHP callstack, stacked segments make little/no sense in its context. Instead, we want to utilize the segment pool available in txn's. force_current_segment was how stacked segments were able to maintain their context in a txn while not being on the segment stack. For OAPI, there is no reason to use this field, as the current segment will just be the top of the txn's segment stack. --------- Co-authored-by: bduranleau-nr <[email protected]> Co-authored-by: Michal Nowacki <[email protected]>
Co-authored-by: bduranleau-nr <[email protected]>
Segment order does not matter, as the collector reconstructs the tree with timing and parenting info. As such, we can remove segments with O(1) speed instead of O(N). This is an alternative approach than #780 and only 1 of the 2 should be merged. --------- Co-authored-by: Michal Nowacki <[email protected]>
) * Enable the opcache.so extension by default in the integration_runner * Ensure the opcache INI settings in the integration_runner environment are enabled (these can be overwritten in the test case INI section if needing to test without opcache) * Added few tests to ensure compatibility with opcache disabled and to demonstrate how to overwrite the value. --------- Co-authored-by: ZNeumann <[email protected]> Co-authored-by: Zach Neumann <[email protected]> Co-authored-by: Michal Nowacki <[email protected]>
Ensure that workflows use the versions of actions that are running on Node 20. Node 16 has reached its [end of life](https://github.com/nodejs/Release/#end-of-life-releases), and GitHub [announced](https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/) its deprecation in GitHub Actions. Actions upgraded: - actions/checkout@v3 -> v4: https://github.com/actions/checkout/releases/tag/v4.0.0 * CHANGELOG: https://github.com/actions/checkout/blob/main/CHANGELOG.md - docker/login-action@v2 -> v3: https://github.com/docker/login-action/releases/tag/v3.0.0 - docker/setup-qemu-action@v2 -> v3: https://github.com/docker/setup-qemu-action/releases/tag/v3.0.0 - codecov/codecov-action@v3 -> v3.1.5: https://github.com/codecov/codecov-action/releases/tag/v3.1.5 - actions/github-script@v2 -> v7: https://github.com/actions/github-script/releases/tag/v7.0.0 * Breaking changes by version: https://github.com/actions/github-script?tab=readme-ov-file#breaking-changes * Required updating the `github` REST call to use `rest` context
Remove 'bootstrap/app.php' from the list of signature files used to detect Laravel - Laravel is detected with 'illuminate/foundation/application.php' for all supported Laravel versions: 6, 7, 8, 9 and 10. Using 'bootstrap/app.php' causes Laravel to be detected before Lumen can be detected because 'bootstrap/app.php' is the second file loaded when Lumen based app is handling the request and this results in Lumen app being detected as Laravel.
feat(agent): use Observer API to hook into Zend Engine for PHPs 8.0+ (JIT Support) fix(agent): Lumen detection
zsistla
approved these changes
Mar 1, 2024
bduranleau-nr
approved these changes
Mar 1, 2024
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.
OAPI (JIT) and Lumen detection