search

nipwaayoni / elastic-apm-php-agent

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

Implement AgentBuilder #31

Closed dstepe closed 4 years ago

dstepe commented 4 years ago

Closes #30

dstepe commented 4 years ago

Good point on the placement of the Config class. However, I think now is the time to change it. In the previously releases, the Agent class created it directly so there should be very little reason for anyone to have ever created a Config object directly. Since we're now changing that and expecting it to be constructed outside of the Agent, moving it now makes sense. I'll do that.

The other changes you listed are all useful improvements. I'll have to work through them in detail later, but nothing jumped out as a "must do" before the next release. They look like incremental improvements that would not cause major breaks. If you think of a case in which any of the suggestions would be better done now, let's make sure to prioritize them.

I want to get a stable release out soon, so we will want to be careful about what we commit to. I'm updating the Laravel client, and that's what caused me to want the AgentBuilder for example. I have a use case that requires me to customize the http client and that was getting hard to manage in the Agent constructor. Now seemed like the time to make the change.

tsantos84 commented 4 years ago

The other changes you listed are all useful improvements. I'll have to work through them in detail later, but nothing jumped out as a "must do" before the next release. They look like incremental improvements that would not cause major breaks. If you think of a case in which any of the suggestions would be better done now, let's make sure to prioritize them.

I agree that adding an Enum class to list all available options could be done later, but the another one is so small and adding then now could avoid some BC breaks in the future.

dstepe commented 4 years ago

The Config class has been moved out of the Helper namespace.

dstepe commented 4 years ago

@tsantos84 When you have a chance review this again for merging to the 7.1-dev branch. It's quite large and I would prefer to limit any additional changes to those closely related to existing changes. Any other changes can be logged as new issues. There are a couple of other existing issues which I think can done quickly, and then we can test the 7.1-dev branch and make a release soon.

dstepe commented 4 years ago

I'll work on cleaning up based on your comments.

Regarding discussing other issues, have a look at the projects:

https://github.com/nipwaayoni/elastic-apm-php-agent/projects

You could create a To-Do card for items we need to discuss or tasks to do. Then we can order them and pull them to In-Progress as we work on them. We can create more projects, not just for releases.

dstepe commented 4 years ago

I changed that method name as suggested. Is there anything else you see on this PR? If not, I'll merge it when all the review conversations are closed.

tsantos84 commented 4 years ago

It's ok to me.