-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(agent): improve exception handler instrumentation for PHPs 8.0+ #877
Merged
lavarou
merged 9 commits into
dev
from
fix/agent/oapi-exception-handler-instrumentation
Apr 19, 2024
Merged
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
b6191bd
add a test that reproduces the issue
lavarou 12f8ce7
fix recording errors for PHPs 8.0+
lavarou 5c7f58d
simplify exception handler check in func_end
lavarou 1d080ab
test with closure as user exception handler
lavarou ceeb424
improve testing of oapi exception handlers instrumentation
lavarou bd384cb
cleanup tests of dead code
lavarou 4646c9c
improve test cases for PHP 8.3.x
lavarou e8669dd
improve test filenames
lavarou 5013256
improve test filenames for PHP 8.3.x
lavarou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
182 changes: 182 additions & 0 deletions
182
tests/integration/errors/test_oapi_uncaught_handled_exception_04.php83.php
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| <?php | ||
| /* | ||
| * Copyright 2020 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /*DESCRIPTION | ||
| When a user exception handler unregisters itself as an exception handler when it handles uncaught exception, | ||
| the agent should record the error and add error attributes on all spans leading to uncaught exception as | ||
| well as the one throwing the exception. Error attributtes are not expected on the root span (because | ||
| the exception has been handled) as well as on the span created for exception handler. | ||
| */ | ||
|
|
||
| /*INI | ||
| newrelic.distributed_tracing_enabled=1 | ||
| newrelic.transaction_tracer.threshold = 0 | ||
| newrelic.span_events_enabled=1 | ||
| newrelic.code_level_metrics.enabled = 0 | ||
| display_errors=1 | ||
| log_errors=0 | ||
| */ | ||
|
|
||
|
|
||
| /*SKIPIF | ||
| <?php | ||
| if (version_compare(PHP_VERSION, "8.3", "<")) { | ||
| die("skip: PHP < 8.3.0 not supported\n"); | ||
| } | ||
| // Fix for https://github.com/php/php-src/issues/13446, released in 8.3.5, | ||
| // causes infinite recursion in this test. | ||
| if (version_compare(PHP_VERSION, "8.3.5", ">=")) { | ||
| die("skip: PHP >= 8.3.5 not supported\n"); | ||
| } | ||
| */ | ||
|
|
||
|
|
||
| /*EXPECT_ERROR_EVENTS*/ | ||
| /* | ||
| [ | ||
| "?? agent run id", | ||
| { | ||
| "reservoir_size": "??", | ||
| "events_seen": 1 | ||
| }, | ||
| [ | ||
| [ | ||
| { | ||
| "type": "TransactionError", | ||
| "timestamp": "??", | ||
| "error.class": "RuntimeException", | ||
| "error.message": "Uncaught exception 'RuntimeException' with message 'Expected unexpected happened' in __FILE__:??", | ||
| "transactionName": "OtherTransaction\/php__FILE__", | ||
| "duration": "??", | ||
| "nr.transactionGuid": "??", | ||
| "guid": "??", | ||
| "sampled": true, | ||
| "priority": "??", | ||
| "traceId": "??", | ||
| "spanId": "??" | ||
| }, | ||
| {}, | ||
| "??" | ||
| ] | ||
| ] | ||
| ] | ||
| */ | ||
|
|
||
| /*EXPECT_SPAN_EVENTS*/ | ||
| /* | ||
| [ | ||
| "?? agent run id", | ||
| { | ||
| "reservoir_size": 10000, | ||
| "events_seen": 4 | ||
| }, | ||
| [ | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "OtherTransaction\/php__FILE__", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "nr.entryPoint": true, | ||
| "transaction.name": "OtherTransaction\/php__FILE__" | ||
| }, | ||
| {}, | ||
| {} | ||
| ], | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "Custom\/call_throw_it", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "parentId": "??" | ||
| }, | ||
| {}, | ||
| { | ||
| "error.message": "Uncaught exception 'RuntimeException' with message 'Expected unexpected happened' in __FILE__:??", | ||
| "error.class": "RuntimeException" | ||
| } | ||
| ], | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "Custom\/throw_it", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "parentId": "??" | ||
| }, | ||
| {}, | ||
| { | ||
| "error.message": "Uncaught exception 'RuntimeException' with message 'Expected unexpected happened' in __FILE__:??", | ||
| "error.class": "RuntimeException" | ||
| } | ||
| ], | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "Custom\/user_exception_handler_02", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "parentId": "??" | ||
| }, | ||
| {}, | ||
| {} | ||
| ] | ||
| ] | ||
| ] | ||
| */ | ||
|
|
||
|
|
||
| /*EXPECT_REGEX | ||
| 01 Handled uncaught exception | ||
| */ | ||
|
|
||
| function user_exception_handler_01(Throwable $ex) { | ||
| echo "01 Handled uncaught exception"; | ||
| } | ||
|
|
||
| function user_exception_handler_02(Throwable $ex) { | ||
| restore_exception_handler(); | ||
| throw new RuntimeException("Not able to handle, throwing another exception from handler"); | ||
| echo "Should never see this"; | ||
| } | ||
|
|
||
| function throw_it() { | ||
| throw new RuntimeException('Expected unexpected happened'); | ||
| } | ||
|
|
||
| function call_throw_it() { | ||
| throw_it(); | ||
| } | ||
|
|
||
| set_exception_handler('user_exception_handler_01'); | ||
| set_exception_handler('user_exception_handler_02'); | ||
|
|
||
| call_throw_it(); |
176 changes: 176 additions & 0 deletions
176
tests/integration/errors/test_oapi_uncaught_handled_exception_04.php835.php
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| <?php | ||
| /* | ||
| * Copyright 2020 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| /*DESCRIPTION | ||
| When a user exception handler unregisters itself as an exception handler when it handles uncaught exception, | ||
| the agent should record the error and add error attributes on all spans leading to uncaught exception as | ||
| well as the one throwing the exception. Error attributtes are not expected on the root span (because | ||
| the exception has been handled) as well as on the span created for exception handler. | ||
| */ | ||
|
|
||
| /*INI | ||
| newrelic.distributed_tracing_enabled=1 | ||
| newrelic.transaction_tracer.threshold = 0 | ||
| newrelic.span_events_enabled=1 | ||
| newrelic.code_level_metrics.enabled = 0 | ||
| display_errors=1 | ||
| log_errors=0 | ||
| */ | ||
|
|
||
|
|
||
| /*SKIPIF | ||
| <?php | ||
| if (version_compare(PHP_VERSION, "8.3", "<")) { | ||
| die("skip: PHP < 8.3.0 not supported\n"); | ||
| } | ||
| */ | ||
|
|
||
|
|
||
| /*EXPECT_ERROR_EVENTS*/ | ||
| /* | ||
| [ | ||
| "?? agent run id", | ||
| { | ||
| "reservoir_size": "??", | ||
| "events_seen": 1 | ||
| }, | ||
| [ | ||
| [ | ||
| { | ||
| "type": "TransactionError", | ||
| "timestamp": "??", | ||
| "error.class": "RuntimeException", | ||
| "error.message": "Uncaught exception 'RuntimeException' with message 'Expected unexpected happened' in __FILE__:??", | ||
| "transactionName": "OtherTransaction\/php__FILE__", | ||
| "duration": "??", | ||
| "nr.transactionGuid": "??", | ||
| "guid": "??", | ||
| "sampled": true, | ||
| "priority": "??", | ||
| "traceId": "??", | ||
| "spanId": "??" | ||
| }, | ||
| {}, | ||
| "??" | ||
| ] | ||
| ] | ||
| ] | ||
| */ | ||
|
|
||
| /*EXPECT_SPAN_EVENTS*/ | ||
| /* | ||
| [ | ||
| "?? agent run id", | ||
| { | ||
| "reservoir_size": 10000, | ||
| "events_seen": 4 | ||
| }, | ||
| [ | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "OtherTransaction\/php__FILE__", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "nr.entryPoint": true, | ||
| "transaction.name": "OtherTransaction\/php__FILE__" | ||
| }, | ||
| {}, | ||
| {} | ||
| ], | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "Custom\/call_throw_it", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "parentId": "??" | ||
| }, | ||
| {}, | ||
| { | ||
| "error.message": "Uncaught exception 'RuntimeException' with message 'Expected unexpected happened' in __FILE__:??", | ||
| "error.class": "RuntimeException" | ||
| } | ||
| ], | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "Custom\/throw_it", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "parentId": "??" | ||
| }, | ||
| {}, | ||
| { | ||
| "error.message": "Uncaught exception 'RuntimeException' with message 'Expected unexpected happened' in __FILE__:??", | ||
| "error.class": "RuntimeException" | ||
| } | ||
| ], | ||
| [ | ||
| { | ||
| "category": "generic", | ||
| "type": "Span", | ||
| "guid": "??", | ||
| "traceId": "??", | ||
| "transactionId": "??", | ||
| "name": "Custom\/user_exception_handler_02", | ||
| "timestamp": "??", | ||
| "duration": "??", | ||
| "priority": "??", | ||
| "sampled": true, | ||
| "parentId": "??" | ||
| }, | ||
| {}, | ||
| {} | ||
| ] | ||
| ] | ||
| ] | ||
| */ | ||
|
|
||
|
|
||
| /*EXPECT_REGEX | ||
| 01 Handled uncaught exception | ||
| */ | ||
|
|
||
| function user_exception_handler_01(Throwable $ex) { | ||
| echo "01 Handled uncaught exception"; | ||
| } | ||
|
|
||
| function user_exception_handler_02(Throwable $ex) { | ||
| set_exception_handler('user_exception_handler_01'); | ||
| throw new RuntimeException("Not able to handle, throwing another exception from handler"); | ||
| echo "Should never see this"; | ||
| } | ||
|
|
||
| function throw_it() { | ||
| throw new RuntimeException('Expected unexpected happened'); | ||
| } | ||
|
|
||
| function call_throw_it() { | ||
| throw_it(); | ||
| } | ||
|
|
||
| set_exception_handler('user_exception_handler_02'); | ||
|
|
||
| call_throw_it(); | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test is named php835, but this just checks for 8.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to properly name the test files that need to run for PHPs 8.3.0-8.3.4 (currently the test is named
tests/integration/errors/test_oapi_uncaught_handled_exception_04.php83.php) vs test files that run correctly for all 8.3.x PHPs. I'm open to suggestions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 835 naming convention was perfectly fine, but the 835 test only had this check:
shouldn't it be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed 835 meant php 8.3.5 and above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of in the name, update the description inside the test explaining why the skipif and which versions we expect it to run on.
Then you can have an 83a, 83b, etc to indicate it is the same test but differences are needed due to variants 8.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions. See 5013256.