Skip to content
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): Slim txn naming updates #917

Merged
merged 5 commits into from
Jun 14, 2024
Merged

fix(agent): Slim txn naming updates #917

merged 5 commits into from
Jun 14, 2024

Conversation

zsistla
Copy link
Contributor

@zsistla zsistla commented Jun 13, 2024

  1. the scheme for route run to correlate with proper OAPI naming was incorrectly listed and therefore txn naming precedence was incorrectly handled
  2. this PR kept same naming scheme in route:run putting it before wrapper_call and saying ok to overwrite (instead of after and not ok to overwrite)
  3. number 2 needed to incorporate fallback naming scheme
  4. added fallback naming scheme (since some errors and middleware intervention give the wrong txn name) using dispatcher:dispatch.

When there is an exception thrown in a route that is handled by the slim routing and error middleware, it is superseded by: Uncaught exception 'Slim\Exception\HttpNotFoundException' with message 'Not found.' and transformed into an http 404 error and is shown in the http.statuscode field as 404. If the middleware isn't loaded, it is recorded as an uncaught exception and status code is 200.

Wrap function Slim\Routing\Dispatcher::dispatch
It will name set the txn_name in case we hit an error and Slim\Routing\Route::run isn't called. It is set to be overwritable do that if we encounter Slim\Routing\Route::run, it can overwrite with the most accurate info (this is only relevant in the case of redirects). This allows us to give the txn_name even in error cases when Slim\Routing\Route::run isn't called.

Corresponds with multiverse updates which passed against this branch.

1) the scheme for route run to correlate with proper OAPI naming was incorrectly listed
2) kept same naming scheme in route:run by putting it before wrapper_call and saying ok to overwrite
3) number 2 needed to incorporate fallback naming scheme
4) added fallback naming scheme (since some errors and middleware intervention hides the txn name) using dispatcher:dispatch.

When there is an exception thrown in a route that is handled by the slim routing and error middleware, it is superseded by:
Uncaught exception 'Slim\\Exception\\HttpNotFoundException' with message 'Not found.' and transformed into an http 404 error and is shown in the http.statuscode field as 404.  If the middleware isn't loaded, it is recorded as an uncaught exception and status code is 200.

Wrap function Slim\\Routing\\Dispatcher::dispatch
It will name set the txn_name in case we hit an error and Slim\\Routing\\Route::run isn't called.  It is set to be overwritable do that if we encounter Slim\\Routing\\Route::run, it can overwrite with the most accurate info (this is only relevant in the case of redirects).  This allows us to give the txn_name even in error cases when Slim\\Routing\\Route::run isn't called.
@newrelic-php-agent-bot
Copy link

newrelic-php-agent-bot commented Jun 13, 2024

Test Suite Status Result
Multiverse 4/9 passing
SOAK 53/56 passing

@zsistla zsistla requested a review from ZNeumann June 13, 2024 02:48
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.

Project coverage is 78.74%. Comparing base (bc0766b) to head (b30ce13).

Files Patch % Lines
agent/fw_slim.c 0.00% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #917      +/-   ##
==========================================
- Coverage   78.79%   78.74%   -0.05%     
==========================================
  Files         193      193              
  Lines       27214    27230      +16     
==========================================
  Hits        21443    21443              
- Misses       5771     5787      +16     
Flag Coverage Δ
agent-for-php-7.0 77.49% <0.00%> (-0.04%) ⬇️
agent-for-php-7.1 77.24% <0.00%> (-0.04%) ⬇️
agent-for-php-7.2 78.16% <0.00%> (-0.04%) ⬇️
agent-for-php-7.3 78.18% <0.00%> (-0.04%) ⬇️
agent-for-php-7.4 77.89% <0.00%> (-0.04%) ⬇️
agent-for-php-8.0 77.95% <0.00%> (-0.04%) ⬇️
agent-for-php-8.1 77.94% <0.00%> (-0.04%) ⬇️
agent-for-php-8.2 77.54% <0.00%> (-0.04%) ⬇️
agent-for-php-8.3 77.54% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lavarou lavarou added this to the next-release milestone Jun 13, 2024
agent/fw_slim.c Outdated Show resolved Hide resolved
agent/fw_slim.c Outdated Show resolved Hide resolved
agent/fw_slim.c Outdated Show resolved Hide resolved
agent/fw_slim.c Outdated Show resolved Hide resolved
Copy link
Member

@lavarou lavarou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

@zsistla zsistla merged commit 8c16b70 into dev Jun 14, 2024
64 checks passed
@zsistla zsistla deleted the slim_txn_naming branch June 14, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants