Depending on `error_log` may result in unseen errors

[timrourke]

php-datadogstatsd/src/DogStatsd.php

Line 397 in 5b89937

error_log($e->getMessage());

I've worked on PHP applications where error_log either wasn't properly configured, or the application used a completely separate logging infrastructure to log errors, exceptions, etc. It is probably a good idea to avoid letting exceptions be raised from DogStatsd::event, but I suspect it's less good to rely on error_log alone, as the client using the library may lose the error message, and/or lose the opportunity to do anything about the fact that the curl call failed.

One alternative could be to allow the client to inject an optional PSR-3 logger interface in the constructor, and then log to that on calls to DogStatsd::event. This is a very common strategy that provides the user of library code to make their own decisions about how to deal with errors, etc. Monolog, for example, provides a way to log to error_log, among a plethora of other things.

[jian-wu]

Is anyone looking at this? This is a serious issue with the SDK for the users. The SDK shouldn't silence any exception that's raised because there's an issue with the service. The SDK must report such issues to the end users so that they don't scratch their heads and pulling their hair out thinking THEY did something wrong.

With or without implementing PSR-3 logger interface, first thing first, the try and catch blocks should be removed and the exceptions should be bubbled up. Second, instead of using plain CURL functions, you should use GuzzleHttp to make the HTTP request.

I'm more than happy to send a pull request if you don't have time or need help.

Thanks.

[timrourke]

I agree with @jian-wu on both points.

@jian-wu now that unit tests have landed it's probably safe to do this level of refactoring.

@masci could you provide guidance on contributing a change that would affect backwards compatibility for consumers of this library? i think both changes are important:

1. do not catch exceptions
2. add support for an optional HTTP client

both are significant changes, so i would propose they be done as separate efforts, but they will definitely impact users, so guidance about contributing changes is important, at least from the standpoint of documenting a changelog so nobody's caught off guard.

btw @masci it's also not clear to me who should do releasing/tagging - is that reserved for you as DataDog to manage? presumably yes? just want to be clear on that as well, since enforcing a disciplined approach to versioning will be important for changes of this magnitude.

thanks!

[timrourke]

@jian-wu as per the http client, i think a good strategy would be to extract http logic into a small service class that is backed by a very simple interface, and then define a "default" implementation that uses curl, and perhaps a modern guzzle one as well, though i'm hesitant to support adding guzzle as a dependency to this client. it needs to support PHP 5.3+, so guzzle isn't a candidate just yet.

however, an example showing how one would use guzzle would be of tremendous value.

in any event because this library is so super simple, avoiding the dependency is prob worth it in my opinion.

@jian-wu could you open your own issue related to HTTP? maybe your needs and desires differ from my own, and i want to make sure we capture everyone's requirements before starting to implement something

[jian-wu]

IMHO, PHP versions below 5.6 should no longer be supported because they are dead. Versions 5.6 and 7 are not going to be supported by end of this year 2018. If for some reason the dead versions must be supported, I would suggest to cut a release version 2.0.0 that would support PHP 5.6 and above.

[timrourke]

@jian-wu that seems sensible to me. @masci any opinion there? @jian-wu is absolutely correct that PHP 5.3 is quite dead to the majority of the PHP community, cutting a 2.0 that is 5.6 would make sense, but it might even be sensible to drop 5.x support, since 5.6 is coming up on end of life.

@jian-wu what does your gut say about just going right to 7.x support?

[jian-wu]

7.0 is going to be unsupported before 5.6 so yes to supporting 7.1+. However, that wouldn't make sense right away because they are still supported. It would be better to start with the 2.0 branch first supporting 5.6 and above then drop 5.6 and 7 when they reach EOL. That can be minor version bump IMO.

Forgot to add, what should we do to version 1 since it currently supports all versions? Should we remove version 5.6 and above once we have the 2.0 release?

[timrourke]

@jian-wu sorry, i don't follow your question about version 1, what do you think should happen regarding version compatibility? what are you asking? want to be sure i'm on the same page

[jian-wu]

@timrourke the current release (version 1) of this SDK supports PHP versions 5.3 and above. I was asking if we should make a change to that so when there's a version 2 that supports PHP version 5.6 and above. Just so no one would keep using version 1 since it supports PHP 7.x.

[jian-wu]

It's not a big deal really, just wondering.

[timrourke]

@jian-wu FYI, this PR proposes a change to essentially drop official support for PHP 5.6: #71