refactor(agent): remove oapi 'clean' callback #866
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #866 +/- ##
==========================================
+ Coverage 78.54% 78.57% +0.03%
==========================================
Files 196 196
Lines 27103 27077 -26
==========================================
- Hits 21287 21275 -12
+ Misses 5816 5802 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -327,16 +327,6 @@ NR_PHP_WRAPPER(nr_drupal_http_request_after) { | |||
| } | |||
| NR_PHP_WRAPPER_END | |||
|
|
|||
| NR_PHP_WRAPPER(nr_drupal_http_request_clean) { | |||
There was a problem hiding this comment.
With this removal, it is failing some drupal integration tests, because the tests were changed for PHP 8.0+ to not create an external segment after an exception. A simple if can be added to the "after" instrumentation if this behavior is still desired. Or do we want to go back to what the agent was seemingly previously doing?
(Also, this old "clean" function was leaving a dangling segment on the transaction. Not a memory leak because the transaction handles its own segment memory, but scary nonetheless)
There was a problem hiding this comment.
Although the tests do raise an interesting point. There are some tests that test for "bad" parameters; these cause an exception. But because our "after" code isn't perfectly checking arguments, it crashes.
|
ZNeumann
force-pushed
the
zjn/oapi/remove-clean
branch
from
88de064
to
7923273
Compare
|
@ZNeumann , this looks close to ready. |
| [{"name": "Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name": "Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"Errors/OtherTransaction/php__FILE__"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"Errors/all"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"Errors/allOther"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"Supportability/framework/Drupal/forced"}, [1, 0, 0, 0, 0, 0]] | ||
| [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"Errors/OtherTransaction/php__FILE__"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"Errors/all"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"Errors/allOther"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], | ||
| [{"name":"Supportability/framework/Drupal/forced"}, [1, 0, 0, 0, 0, 0]], | ||
| [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]] |
There was a problem hiding this comment.
Are these expectations changed at all or are the elements just reordered?
lavarou
force-pushed
the
zjn/oapi/remove-clean
branch
from
7923273
to
42797df
Compare
lavarou
force-pushed
the
zjn/oapi/remove-clean
branch
from
42797df
to
8b2ec4b
Compare
The "clean" callback is not needed. In order to get rid of it, we can just call the "after" callback instead, simplifying the API and future wrappers. To achieve this removal from our current code, some auditing is required. Every single use of
zval *func_return_valuemust be first checked forNULL. Furthermore, in wrappers that used to have a "clean" callback, it must be ensured that the logic that was present in the "clean" callback is still executed in the "after" callback, even when the return value isNULL.