arkaitzgarro / elastic-apm-laravel

Laravel APM agent for Elastic v2 intake API
MIT License
79 stars 17 forks source link

artisan commands fail with "No service name registered in agent config." #122

Closed dstepe closed 4 years ago

dstepe commented 4 years ago

We have observed that artisan commands in the composer post-install scripts fail in our CI/CD pipeline with the error:

   Nipwaayoni\Exception\MissingServiceNameException 

  No service name registered in agent config.

  at projects/elastic-apm-php-agent/src/Config.php:105
    101|     {
    102|         $this->resolveLegacyOptions();
    103| 
    104|         if (empty($this->config['serviceName'])) {
  > 105|             throw new MissingServiceNameException();
    106|         }
    107| 
    108|         // TODO validate serviceName matches ^[a-zA-Z0-9 _-]+$
    109|         // TODO support list of server URLs

  1   projects/elastic-apm-php-agent/src/Config.php:70
      Nipwaayoni\Config::validateConfig()

  2   projects/elastic-apm-laravel/src/ServiceProvider.php:109
      Nipwaayoni\Config::__construct(["Laravel", "7.28.4", Object(Monolog\Logger), "error"])

This can be reproduced by removing the local .env file and running:

php artisan package:discover

I believe we have identified the source of the problem and I'll be making a PR with a fix later today.

arkaitzgarro commented 4 years ago

It might be related to this random (or not) failing unit test?

dstepe commented 4 years ago

I would say there is a very good chance. I'm having a difficult time explaining the "random" part, but I think I understand the cause. I need a little time to confirm and put a change together, but should have something within a day.

arkaitzgarro commented 4 years ago

The randomness most probably can be explain by codeception settings, where we shuffle the test execution, to actually spot situations like that. I can imagine that depending on the order, the configuration is available or not.

Running the tests (2.x branch) with a failing seed makes them always fail:

 vendor/bin/codecept run --seed 2103579227
dstepe commented 4 years ago

The specific bug we found does produce the same error, but I have not yet determine why the order of the tests causes this behavior to manifest. It is rooted in the ServiceProvider, and the one test which fails with this error is the one testing the ServiceProvider. There must be some connection, but the fix for the bug does not fix the tests when using a seed that previously failed. There are also failures regarding the Log class, which makes no sense at all. I'll keep digging on that.

dstepe commented 4 years ago

I think I finally figured it out. Two things came together to create those random failures. First, we expanded the test suite to include the config tests which have a side affect of setting environment variables during execution. Second, on the v2.x branch, I added a simple test to validate the Agent can be created with different methods to get the Monolog instance. That was the first use of orchestra/testbench to actually load the ServiceProvider and test it.

The issue appears to be the order of execution of the config tests and that use of _before to clear the environment variables. Since the variables are cleared before a test, the any variables set in the final test to be run will remain set. When that final test was one which set some specific variable (and I have not bothered to track that down), the configuration was impacted when testbench bootstrapped the Laravel framework.

Since we have not added any ServiceProvider tests to other users of testbench to the v3.x line, we have not seen the problem there.

The solution is to use _after in the config tests to clear the environment variables after each test is run, thereby leaving the environment in a clean state.

I'll include this in the current PRs for this bug, even though they are not directly related.