Ship guzzlehttp/guzzle by default #1539
Comments
|
I have to be sincere, I don't like this change very much, but I understand the concern; also, #1538 seems to require those packages with a wide range (6 and 7) and still leaves in place all the PSR-18 leveraging stuff, so it seems a nice compromise. cc @nicolas-grekas, since this kind of feedback on the HTTPlug plugin is useful. |
|
While I understand the reasons that brought to this point, I also don't like this change at all. This is not to say that there aren't things to improve, I want to make this clear, but I think we have the opportunity to still release an SDK that is decoupled as much as possible from the HTTP client, thanks to recent development efforts that were not available in the past. However, what's happening here imho is that we're choosing the easy way out, without remembering that recent issues with the dependencies should not cancel years of work and successful distribution of this package. |
|
Thanks for sharing your input. The v3 of this SDK will move forward as is. For v4, we'll solely rely on Guzzle and remove most other dependencies. |
|
Understood, I don't intend to push to change that; but please keep in mind that in PHP we have the PSR, and with PSR-7 (HTTP messages) & PSR-18 (HTTP Client) it's already possible (in v3 too) to easily swap out the HTTP client of the SDK; I would advise against rolling that back, since depending on those two standards only means depending on a couple of interfaces, which are widely accepted in the ecosystem. |
|
As an outsider, this change seems weird and ill-advised to me. It's probably too late at this point to change your mind (and who am I anyway), but I'd still like to make a case against this decision: I'm not sure what "concerning movement" you saw in the PHP ecosystem, but there is a widely respected consensus on semantic versioning which as you probably know dictates no breaking changes in major versions. The PSR's that Sentry is currently relying on also adheres to this, and does not change versions much at all: PSR-18 is still at version 1 for example. Therefore it's not clear to me how behaviour in dependencies could change this much for the SDK to cause problems. It looks more like you've got bitten by a couple of bugs and decided to narrow the possible dependencies to 1 to limit the possibility of new bugs. However, Guzzle can also introduce bugs which will break things for customers. You mentioned the Dependencies Cost principle as a reason. I understand that as part of that rule, you want to limit dependencies as much as possible. However this change will still cause a dependency to be added. You just narrow the exact name and version of the dependency. This in turn can actually cause an increase of dependencies for your customers. In our case, we use Symfony's PSR18 Client for Http requests and because of this we will now get an extra dependency. This is exactly why the PSR (and the PHP-FIG for that matter) was introduced in the first place. As a result we would now have 2 dependencies to keep track of and configure. Also when debugging we would have to understand both clients and their differences and idiosyncrasies, which adds to the cognitive load for developing. Another example is when you actually do use Guzzle, but want to use a different version than the SDK allows. This is often the case when you want to adopt a new (major) release or PHP version before either Guzzle or Sentry has been updated to be compatible with it. It may seem like a non-issue since Guzzle has a wide version support range, and maybe you update this dependency constraint as soon as possible. But is my experience with similar situations that it can take weeks of even months in some cases before you can upgrade, which is made only more likely when you have other packages that rely on Guzzle and you get this "dependency entanglement" situation. So with this change, I think you are enforcing the first principle too rigid by actually narrowing compatibility, and thereby breaking the second one (Compatibility is King). Like @Jean85 and @ste93cry I agree that this change is throwing the baby out with the bathwater (not speaking for them of course, apologies if I misinterpreted 😅). Like I said, I doubt my plea will matter much in the end, but I hope that as a library maintainer you at least consider the opinion of your actual customers too. |
|
Thanks for your feedback. You are correct that adding Guzzle to the current dependencies won't fix any issues we face. As mentioned, for v4, we do indeed plan to change this. I agree that issues might arise because of future updates to Guzzle. Other possible solutions are vendoring-in certain packages or implementing our own HTTP client. In the end, we do want to take full control over the HTTP client we ship with the SDK. |
I highly discourage this. In the past, this package had its own HTTP client cURL-based and it was a mess, full of problems. The PHP ecosystem doesn’t need yet another client, especially because it’s rather complicate to build a well implemented solution. |
|
We already discussed this topic multiple times. I'm ok with leaving it by that. @stayallive and I will consider all concerns and trade-offs mentioned and will come up with a solution that we feel confident with and solves the 95% use-case well. |
|
Forcing a dependency is contributing to dependency hell. @pkruithof gave some nice examples. It's also preventing tighter integration with the framework when using one. For example proxy configuration is something that's better handled in a centralized way. With either forcing guzzle or implementing your own client, you're going to forbid such nice integrations. It's playing against your users, especially when they voice their preference since years... and might end up as a good reason to either move on to another provider or provide a community-contributed client with eg nice integration with Symfony, in my case. All this is especially disappointing to me because I specifically worked and contributed all the required changes to make this decoupled plan a reality 🤷 Of course this aspect doesn't really matter, so I'm happy that others than me raise the same concern independently. I'm looking forward to more opinions, in the hope they could be listened to. |
I understand this is the reasoning behind it, but I still don't understand which problem you are actually trying to solve with this. In fact, I would argue the SDK will be a bit worse off with this change, rather than fixing a problem. |
|
We will listen to all of course and will also not discard any solution yet (still very much in the planning / looking for feedback phase) but the current state of the HTTP client is not attainable if issues like we’ve had the past few weeks keep popping up. Sentry SDK needs to work and not need the problems forced on us by abandoned packages we can’t upgrade from and abstractions creating bugs we can’t control. Telling customers they just need to sit tight until a dependency of our dependency get’s an update is not ideal. The effort that has been taken to make a centralized way to configure HTTP clients is excellent but just browsing to the past issues here will show that it’s long from perfect and there is only so much we can do to influence it since we are also dependent on the hard work of those open-source maintainers which might have other priorities than us. I personally feel like Sentry is a component that should never cause issues or crashes (like the request integration did in numerous occasions) and also work in any, no matter the complexity, environments. From frameworks with the plumbing for centralized HTTP clients to plain index.php sites to WordPress without a 3 page document describing how to setup the HTTP client using all kinds of factories and abstractions. Please also keep in mind that we are discussing this in the base SDK. What we do here is not necessarily what we will do in the framework integrations where we could hook into the plumbing available to us provided by the framework (like request objects and HTTP clients). So this work does not mean (if we go with Guzzle) the Symfony package will also need to use Guzzle, it might use the client packaged with the framework instead for example. |
Just curious (not trying to snark here), but did you discuss it with actual library users (customers) as well? |
|
Yes, Sentry is present at many PHP conferences where we talk to our customers (SaaS or self-hosted, the SDK concerns both equally), and we're active on our Discord and other social channels. There is also a lot of communication happening via our support channels, which none Sentry employees have very little insight into for obvious reasons. When issues like the latest libCurl update happen, we receive many reports that people cannot use the product anymore. While other open-source projects follow a more structured release process, we have to fix issues extremely fast, often pushing a new release in a matter of hours the same day an issue was reported. For a lot of our customers, Sentry is mission-critical to their success. This then clashes with how other open-source packages schedule their bug-fix releases because many of our dependencies are not maintained by people being paid to maintain these. The more abstractions and dependencies we add, the harder it gets to fix these issues. It's, therefore, in our desire to control the software we ship more tightly, resulting in the often mentioned SDK Philosophy. What I want to say is that GitHub issues are just a small part of the bigger picture regarding data points we have at our disposal to come to such conclusions. |
While I agree with this, I think everyone knows it is utopic. No matter who implemented what or where, the chances that it will crash because of exceptional conditions will always exist. Yes, there are implementations that are more stable than others of course, and great care should be used to choose the best among all of them, but I don't think it's correct to point the finger.
This is only partially true. Yes, nothing prevents the framework package to use the framework-provided HTTP client as long as the SDK works using the PSR interfaces and let people switch the underlying implementation, but people are still forced to install a dependency they might not even use. We nowadays have a way for packages to require the presence of an implementation during the installation, but it looks like Sentry must always be the nanny for their customers, and as a customer I personally dislike it.
You mention this problem, but it could have happened with any other implementation, even your very own one. Bug exists, and instead of pointing the finger you can always work with the upstream vendor to solve them in a faster way, like when people contribute changes to the SDK itself. |
Also, both httplug and Symfony http-client provide alternatives over curl. So customers wouldn't have had to wait for Sentry to fix this for them but could instead (temporarily) switch to a different client implementation. I think Sentry could provide a useful service in that case by recommending this fix/workaround rather than enforcing their choice. |
There is no finger pointing here, not my intention just backing up my claims. And while yes it is probably a pipe dream with less upstream dependencies we have the control to roll out quicker patches to fix things if needed which is not always possible with external dependencies (not blaming).
Ther are many ways and options to prevent this within composer and I'm sure we can figure one out to not have to install the default HTTP client for the framework integrations, which I totally agree that if we went that route would not be very nice 👍
I agree here too, however we also don't always have control over this and even submitting issues and PRs doesn't always move us forward quicker. This is a trade-off we have to figure out how much dependant we want to be on others and how much control we want to take for ourself. |
It makes sense, but I still fail to see how bundling Guzzle would help. That implementation is still a dependency, which even if well maintained, it is subject to the same problems as any other opensource project. If you want full control, then the answer is to build your own implementation, and we all know it would be more harmful than the other options.
Absolutely true. That's why I was mentioning that we should choose carefully the dependencies. At that time, |
|
It's motivating to see so many people care about how we move forward and let me be clear, we take all the feedback & input very seriously. This is not a decision we have to make today, we will discuss it internally and communicate what the way forward looks like. One thing is for sure, there is no perfect solution everything here comes with a tradeoff, but we will make ensure whatever decision we take doesn't go against what our users want/need to use Sentry. Thank you everyone ❤️ |
|
In the light of Dependency Cost and Compatibility is King it would make sense to me personally to use PSR-7 and PSR-18 instead (with proper versioning constraints). If this was the case for the Sentry SDK, it means we have to maintain both our internally chosen HTTP client (or any future HTTP client we want to use) ánd Guzzle HTTP Client. For specific reasons we have opted to not use Guzzle when ever possible, and, for the past year, we have worked hard to get it out of our lock file. E.g., by updating internal composer packages, but also by creating PRs in upstream packages to allow the support of PSR-7 and PSR-18 instead. By maintaining just PSR-7 and PSR-18 support, it would also greatly widen the compatibility of the SDK for other clients. When requiring the I sincerely hope this can be reconsidered |
|
I sympathise with the intentions behind this change: wanting to be as stable as possible for your customers is something nobody here argues with, I think. Maybe the solution lies somewhere in the middle: when you provide Guzzle as the preferred and default option while still providing a way to use a different client, I think you are adhering to your design principles. You could then make it clear that if you use a different client, you cannot rely on any support for third-party caused issues. People that choose this are typically well aware of the intricacies of handling dependencies and are probably willing to fix or work around bugs like the one mentioned. I believe this setup is currently already the default for Sentry. So my question is, could this intention be met if you simply define your responsibilities, and make it clear for customers what they can expect from you? I think that would benefit all angles here. |
|
Shouldn't you just use https://docs.php-http.org/en/latest/discovery.html instead of doing anything on your own? From my personal PoV adding any library as dependency is a problem, best libraries has very low amount of external deps. |
|
When reading this post, I was wondering why sentry, whose first customers are developpers, does not rely on their customers capacity to choose what's best in their case and only suggest a library which will be tests and approve by sentry ? And I just see above that @pkruithof thinks someway like I did In fact, making my builds larger, my downloads longer, and expose me to bugs/security issues in a library I don't need is a strange consideration upon you customers. Just
10 minutes to deploy and no angry customers |
Doesn't this change directly contradict the philosphy at absolutely no added benefit?
This isn't solved by shipping guzzle by default. If it's the HTTP Clients of users that are affected by changes in minor version updates, it's on the users to pick a HTTP Client that follows PSR 7/18. This seems like a very strange decision that seems wholly unrelated to the "reason" provided. I would appreciate if you reconsidered this move. If you really want users to use guzzle, for some reason, I agree with remi-parallel that you should put it in the recommended list. |
This is what's actually happening in the SDK: Lines 28 to 33 in b311b54 And this issue is discussing moving away from that approach; that's exactly what's been debated here. |
|
First of all, thank you @cleptric, @stayallive and @HazAT for allowing this discussion to happen in the open. ❤️ Here is a quick recap of the current situation:
This means that:
What the community wants is to not be forced to install deps they'd better not use. In some recent PRs, I proposed to retire sentry-php-sdk entirely and rely solely on sentry-php + php-http/discovery to auto-provide the bestfit PSR7+17+18 implementation. I totally get that you want to provide the best possible out-of-the-box experience also. I share the same concern for every PHP SDK and that's the reason why I contributed the composer plugin to php-http/discovery, so that SDK authors could both decouple from any specific client and still ensure all their required deps are installed. In my mind, this problem is solved now and all sentry packages should rely on php-http/discovery. Best of all, this is already the case for your main library: sentry-php. I clicked on the links you provided to document the recent issues you had with libcurl and as far as I got it correctly, those issues came from php-http/curl-client. I would not recommend using this client anywhere so 🤷 Guzzle + Symfony's HttpClient don't have these issues and are very well maintained. You (and everybody in the community) should tell ppl that use php-http/curl-client that they should use another client. This solves the argument about the reliance on the community to me (I don't think Sentry can provide its own built-in client. It'd be unreasonable to expect a single vendor to be more reliable than a widely used client.) So, my initial proposal was to make the laravel + symfony bridges rely on php-http/discovery's composer plugin to get Sentry in a fully working state. Both projects enable the plugin by default for new apps since laravel/laravel#6106 and symfony/skeleton#214 and the union of them should cover a significant chunk of your user base, so that less and less end users should be asked by composer to explicitly allow the plugin to run (which is one reason against the plugin that has been mentioned before). Still, if you think that's not enough, what about this plan?
Of course, this alt. plan is needed only if you do not want to rely on php-http/discovery. If you're fine with the plugin (as your are right now, and as would be my preference), then this plan is not needed and I'd recommend following up with the plan I proposed in the last months. But please don't follow up with #1538. 🙏 |
|
Version 4.0.0 of the SDK ships with a default cURL HTTP client. |
Dependencies Cost, Degrade Gracefully & Compatibility is King.
The above are all part of Sentry's SDK Philosophy and Design Principles .
In the last few weeks, we saw a concerning movement in the PHP ecosystem, where behavioural changes in dependencies we rely on were released in minor releases.
While some issues are our fault due to relying on transient dependencies, others hit us and our users by surprise.
Moving forward, the Sentry PHP SDK will ship with an HTTP client by default. We decided to take this step to introduce more stability to the SDK and allow ourselves to focus on new features instead of catching up with an ever-faster-moving ecosystem.
Our HTTP client of choice will be Guzzle, a stable and mature HTTP client that moves slowly and still supports PHP 7.2 and up in the latest major release, the same PHP versions this SDK supports today.
This is a first, cautious change, as we keep most dependencies around for the time being.
If you want to help us test this, have a look at #1538.
You can try out the change by requiring
"sentry/sentry": "hello-guzzle as 3.18.2"locally.Please let us know if you run into any issues.
The text was updated successfully, but these errors were encountered: