Auto instrumentation hooks for PHP Session #433
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
============================================
- Coverage 83.12% 82.53% -0.59%
- Complexity 1396 1983 +587
============================================
Files 97 137 +40
Lines 5599 8084 +2485
============================================
+ Hits 4654 6672 +2018
- Misses 945 1412 +467 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 38 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
This PR looks ok to me. I don't have a strong opinion, but it's quite specific - have you considered whether there are any similar functions that could also be incorporated? I think it's good to consider that now, since it could mean changing the name? |
@brettmc Yes sure , I agree that there is scope of adding more functions in the list, However seeing the list of functions I dont feel like adding hooks for these since I don't feel these will add any value in trace. for example session_gc , session_reset , session_save_path , session_get_cookie_params, session_cache_expire However current list can be expanded to session_write_close , session_unset . Do you have any suggestions/feedback ? |
I was thinking along the lines of non- So the question is: is a package just for sessions ok, or could its scope be expanded to include some other core functions? Perhaps things like |
I see , I haven't thought about adding other php internal functions in this .. I think since session is PHP extension we can keep this as separate from other php global functions |
That makes sense. Covering just the |
src/Instrumentation/Session/tests/Integration/PhpSessionInstrumentationTest.php
Outdated
Show resolved
Hide resolved
src/Instrumentation/Session/tests/Integration/PhpSessionInstrumentationTest.php
Outdated
Show resolved
Hide resolved
|
@brettmc , Thanks for reviewing it. |
|
@brettmc , Thank your for patiently reviewing this PR. I will appreciate if you can take a look again after my newer commit on requested changes. |
| $cookieKeys = []; | ||
| ksort($cookieParams); | ||
| foreach ($cookieParams as $key => $value) { | ||
| if (is_scalar($value)) { |
There was a problem hiding this comment.
why this check, if you're storing the keys? what's special about non-scalar values that makes you not want to store the associated key?
There was a problem hiding this comment.
Agree ... my bad , this can be removed since now w e just storing the keys , no check is required on value.
|
@brettmc , I would really appreciate if you can spend some time to review. |
This PR adds auto instrumentation hooks for session_start() and session_destroy().