Refactor extension's code (v2) #26
Conversation
|
@SergeyKleyman, @olksdr now I can pull from the remote repository of Sergey, thanks! |
|
@ezimuel yeah, we had to go over the settings and change few things for @SergeyKleyman . I'm glad it helped 😄 |
There was a problem hiding this comment.
@SergeyKleyman, I really liked the way you organized the code, awesome! There are some points that I commented and I think we should change it, but we can discuss during our 1:1 meeting. Let me know if you need more information about my review. Thanks!
src/ext/public_api.h
Outdated
|
|
||
| #include "php_elasticapm.h" | ||
|
|
||
| static inline const char* getCurrentTransactionId() |
There was a problem hiding this comment.
As coding standard in PHP, all the function names for extension are prefixed by the name of the extension (e.g. elasticapm). Moreover, the function names use underscores between words, while class names use both the camelCase and PascalCase rules. See https://www.php.net/manual/en/userlandnaming.rules.php for more details.
In our case, we should have:
elasticapm_get_transcation_id();elasticapm_get_trace_id();elasticapm_is_enabled();
I'm proposing to omit current in the name since it's always the current transaction and trace. WDYT?
There was a problem hiding this comment.
Sounds good - I've changed the names to use underscores.
As we discussed, I think we should keep current part. The reason is that public API of the PHP library part, that I plan to implement next, will have functions such as beginTransaction(), beginCurrentTransaction() and getCurrentTransaction() (similar to Active Span concept in OpenTracing API for PHP) and you are correct about the fact that elasticapm extension only cares about transactions created with beginCurrentTransaction() and it doesn't need to track transactions created with beginTransaction() API but I think it will be easier to follow the code if GlobalTracer::get()->getCurrentTransaction()->getId() PHP function calls elasticapm_get_current_transaction_id() instead elasticapm_get_transaction_id(). Of course, it's just a matter of naming and we can do it the way you suggest (without the current part) but I propose to wait until we decide on initial public API of PHP library part and then we can re-evaluate this approach. WDYT?
| By default, the extension is disabled. You need to enable it setting `elasticapm.enable=1`. | ||
| The required settings are: `service_name` and `host`. You need to specify a | ||
| service name and the APM server host where to send the data. | ||
| By default, the extension is enabled. You can disable it by setting `elasticapm.enabled=false`. |
There was a problem hiding this comment.
Are you sure we want to enable it by default? This is the same behaviour of other APM agent?
There was a problem hiding this comment.
As we discussed, the default of elasticapm.enabled configuration is true. BTW you can find a detailed discussion about its meaning here.
|
|
||
| static void genRandomIdBinary( Byte* buffer, UInt8 idSizeBytes ) | ||
| { | ||
| FOR_EACH_INDEX( UInt8, i, idSizeBytes ) buffer[ i ] = (Byte) ( php_mt_rand_range( 0, UINT8_MAX ) ); |
There was a problem hiding this comment.
Why are you using UINT8_MAX here? I think it's enough to generate from 0 to 255 values.
Btw, if you want to generate random bytes you can also use the random_bytes() function of PHP here.
To call it in C, you can use the following function (#include "ext/standard/php_random.h") :
int php_random_bytes(void *bytes, size_t size, zend_bool should_throw);it will generate size bytes. If should_throw == 1 it will generate an error in case the random generator is not secure.
You can read the php source code here for more information.
There was a problem hiding this comment.
We discussed and decided to stay with php_mt_rand_range because random_bytes generates cryptographically secure pseudo-random bytes which is most likely is more costly from performance point of view but we don't need cryptographically secure part so we don't want to pay for it with performance impact.
|
|
||
| static inline void getCurrentTime( TimePoint* result ) | ||
| { | ||
| gettimeofday( &( result->systemClockTime ), /* timezone_info: */ NULL ); |
| config = getCurrentConfig(); | ||
|
|
||
| // __asm__("int3"); | ||
| if ( !config->enabled ) |
There was a problem hiding this comment.
If we want to enable/disable the elasticapm extension from a PHP script, using ini_set(), we cannot add this check here. The elasticApmModuleInit() is executed before any execution of PHP code, so if you have the elasticapm.enabled=false (in your php.ini) and you want to enable it only for some PHP script, using ini_set() you will never execute the elasticApmModuleInit() function.
There was a problem hiding this comment.
As we discussed, according enabled specification it's should be read by agent only once at the start. I've created a follow up issue to mark it as not modifiable ini entry - #28.
|
@ezimuel I think I've finished all the changes based on the review feedback. Please take a look. |
This PR replaces PR #25.