Set active to true only when a serverurl is present

[dstepe]

Looking at the default behavior, is it not the case that the Agent will immediately try to send to http://127.0.0.1:8200 since that's the default serverUrl and active is true? Would it make sense to set the active indicator to true only when config('elastic-apm.active') and there is a non-empty serverUrl?

https://github.com/philkra/elastic-apm-laravel/blob/master/src/Providers/ElasticApmServiceProvider.php#L48

Something like:

            $active = config('elastic-apm.active') && config('elastic-apm.server.serverUrl');
            return new Agent(
                array_merge(
                    [
                        'framework' => 'Laravel',
                        'frameworkVersion' => app()->version(),
                    ],
                    ['active' => $active],
                    $this->getAppConfig(),
                    config('elastic-apm.env'),
                    config('elastic-apm.server')
                )
            );

I can make a PR for this if you agree.

[philkra]

I gave this some time to reflect and honestly I don't think it's necessary. I guess it's better to have the Agent throw an exception in order to notice something is off rather than make someone wonder why no data is being send..

[dstepe]

I work with several teams each responsible for multiple Laravel applications. We're very excited about APM and the insight it can give us. My concern with this module behavior is that every developer will suddenly have their apps break in the local development environment until they disable the agent. We will never want data from those local installs going to our APM service, so in our case, the least disruptive behavior is to default the agent to disabled and then enable it during the test and production deployments.

I see your point as well and I concur that obvious errors are better than quiet failures. However, if I have not configured my own APM service URL, I should not reasonably expect the agent to be successfully sending data anyway. So for an initial adopter, I don't see much difference between:

1. it throws an error until I configure it or
2. it does nothing until I configure it.

Having the APM_ACTIVE environment variable and updated documentation helps address my concerns and I'm happy to defer to you in this matter. This is just my opinion.

[philkra]

My concern with this module behavior is that every developer will suddenly have their apps break in the local development environment until they disable the agent. We will never want data from those local installs going to our APM service, so in our case, the least disruptive behavior is to default the agent to disabled and then enable it during the test and production deployments.

I don't see a difference, when a developer sets the semantically correct Agent = disabled on his/her local machine, they would need to otherwise set the APM_SERVERURL to ''. A change for the inactive Agent to work would be to return true instead of false if the Agent is disabled, https://github.com/philkra/elastic-apm-php-agent/blob/master/src/Agent.php#L219

What do you think about this solution? It wouldn't break on the local instances, yet it would ensure a thrown error when sending traces is required and the server url is missing.

[dstepe]

I think my concern is with the assertion 'when sending traces is required'. Because the config defaults to the agent being active, it is not safe to conclude that sending traces is the required behavior since it may just indicate incomplete configuration. If the user were required to enable the agent, then it would be clear that a failure to send the trace should be an error condition.

I was looking for a solution that would not break a new install until configured, but also did not require changing the current default active value. But my suggestion does overly complicate the behavior, so that's probably not a good direction.

I think that the recent changes may provide reasonable mitigations for my concern. It's easy to tell people to simply add the APM_ACTIVE=false to the environment. If necessary, the elastic-apm.php config file can published, modified and packaged as part of the project. Either solution enables a team to collaborate easily.

I've pretty much talked myself into just closing this.

[dstepe]

@philkra I owe you an apology (actually a couple I think). I made a functional change in a pull request which was nominally for updating the documentation (https://github.com/philkra/elastic-apm-laravel/pull/29). I should not have done that for one thing. That functional change also affects the behavior you expressed as desirable, which I was not clear on at the time.

The specific commit is https://github.com/philkra/elastic-apm-laravel/pull/29/commits/77d607b49c75b202c071e2f00aca7d25746b55ba.

I was experiencing failures in my application when the APM service was not available for some reason. Since the APM data is a secondary goal, my preference is to have it fail quietly without impacting the application. However, it is important that the condition be logged. That's the change I made, but it's counter to your point that it is "better to have the Agent throw an exception in order to notice something is off rather than make someone wonder why no data" is sent to APM.

At this point, the change to log the condition and suppress exceptions is in the release. How would you like to proceed? I do feel strongly that a disruption to our APM service should not cause applications to throw an error, so I will continue to advocate something like this solution. A useful enhancement would be to provide the user more control over behavior, possibly by requiring a positive assertion to suppress exceptions or something.

[philkra]

I'm aware of the commit 77d607b and was fine with it in a laravel context. The error is logged which should serve the purpose.

That's the change I made, but it's counter to your point that it is "better to have the Agent throw an exception in order to notice something is off rather than make someone wonder why no data" is sent to APM.

In a non-laravel context, the developer should keep control of the code, hence take care of the exception catching him/herself.

my preference is to have it fail quietly without impacting the application

The is generally the preferable goal :)

I do feel strongly that a disruption to our APM service should not cause applications to throw an error, so I will continue to advocate something like this solution.

I second that! It kills two birds with one stone.

BTW: no need to apologize!

[dstepe]

Great! I think we are on the same page with this then.

Thanks for all your work!