nipwaayoni / elastic-apm-php-agent

PHP Agent for Elastic APM
Other
28 stars 15 forks source link

AgentBuilder Config #40

Closed science695 closed 4 years ago

science695 commented 4 years ago

With the added AgentBuilder, I can see some of the benefits with frameworks resolving the classes, and not being easy to directly call the new Agent().

But is there a benefit to requiring the caller to create the Config() object? instead of just the array like earlier versions did?

It seems that it is making framework usage easier, while making it more complicated for direct calling. Is there a middle ground that would help both cases? (allowing either array or Config object?)

dstepe commented 4 years ago

There is a trade off. My concern for the Agent class is that it's the core of this package. To me, it's much better if anything passed to the Agent is known to be correct. In addition, anything the Agent depends on should be provided to it rather than being created by it. That enables providing alternate implementations for testing.

That said, I do understand the concern that the package could become more difficult to use in some ways. I can probably be convinced that a simpler implementation is warranted, but I think that will come with refactoring how configuration is handled overall. I'm open to suggestions.

dstepe commented 4 years ago

Is something like this suitable?

$agent = AgentBuilder::create(['appName' => 'Test Created App']);

The replicates the previous simple creation using the AgentBuilder and has no impact on the Agent class itself. I open #42 to implement this in the 7.2 release, but I'm still open to other creational patterns.

science695 commented 4 years ago

So that lets you keep the new features, while still leaving a simple constructor available.

I think that is a great option.