dd-trace-php icon indicating copy to clipboard operation
dd-trace-php copied to clipboard

[BUG] datadog-setup.php fails if "Scan for additional .ini files in" is not set

Open smottt opened this issue 3 years ago • 4 comments

Bug description

Running the install script for a PHP installation that was compiled without the --with-config-file-scan-dir=... does not really work.

Here's a snippet of the output (absolute paths redacted):

Installed required source files to '/opt/datadog/dd-library/0.77.0'
Installing to binary: php (/bin/php)
Copied '/tmp/dd-install/dd-library-php/trace/ext/20190902/ddtrace.so' to '/lib/php/extensions/no-debug-non-zts-20190902/ddtrace.so'
Copied '/tmp/dd-install/dd-library-php/profiling/ext/20190902/datadog-profiling.so' to '/lib/php/extensions/no-debug-non-zts-20190902/datadog-profiling.so'
Copied '/tmp/dd-install/dd-library-php/appsec/ext/20190902/ddappsec.so' to '/lib/php/extensions/no-debug-non-zts-20190902/ddappsec.so'
Created INI file '(none)/98-ddtrace.ini'
Installation to 'php (/bin/php)' was successful
--------------------------------------------------
SUCCESS

The interesting part here being the Created INI file '(none)/98-ddtrace.ini'.

PHP INI

# php --ini
Configuration File (php.ini) Path: /lib
Loaded Configuration File:         /lib/php.ini
Scan for additional .ini files in: (none)
Additional .ini files parsed:      (none)

PHP version

# php -v
PHP 7.4.30 (cli) (built: Aug  1 2022 11:44:12) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.30, Copyright (c), by Zend Technologies
    with Xdebug v3.1.5, Copyright (c) 2002-2022, by Derick Rethans
    with blackfire v1.80.0~linux-x64-non_zts74, https://blackfire.io, by Blackfire

Basically the setup script seems to take the (none) as a valid path and creates the file there.

smottt avatar Aug 04 '22 06:08 smottt

The installer does indeed assume that the scan directory is configured and that we own the configuration file. The last part (ownership) allows the installer to (simply) change the configuration file for ddtrace.

Two approaches I can think of: We either declare in our documentation that the scan directory setting is a pre-requisite of installation, or we develop the installer to have the ability to edit the global configuration file for the SAPI (ie, php.ini, php-cli.ini).

Editing the global configuration file is problematic for upgrades - our edits will easily be wiped out during routine upgrades - as well as harming our efforts to be a nice neighbour to other extensions.

I'm heavily in favour of taking the documentation change route; In addition we should update the installer to fail when pre-requisites are not met.

krakjoe avatar Aug 04 '22 11:08 krakjoe

The other alternative would be: We only insert the extension=ddtrace entry, but don't add the default ini (99-ddtrace-custom.ini). So we would just do a default installation, without providing the ini template. After all, it's just a courtesy. An user may have to upgrade the ini themselves. But probably better than just outright failing.

bwoebi avatar Aug 04 '22 12:08 bwoebi

That's problematic for upgrades in the same way, a routine upgrade breaking installed features is not very desirable.

It does seem like an acceptable pragmatic approach, so long as the user is warned during installation about the kind of install being performed (and possibly advised to use scandir?).

krakjoe avatar Aug 04 '22 13:08 krakjoe

Yes, you can't have it all ways at once :-) But yes, we can definitely warn them that we did not touch their inis and using a scandir would be advisable, but it works.

bwoebi avatar Aug 04 '22 13:08 bwoebi