Symfony SDK 5.0.0
cleptric opened this issue · 33 comments
Due to high demand, we decided to ship a new major version.
We are not planning to add any new features and will only bump the PHP SDK to version 4.0.0 and remove three deprecations.
- Bump the PHP SDK to v4.0
- Improve the
SentryTestCommandoutput
@Jean85 @ste93cry If you guys have something in mind and the capacity to work on it, please let us know.
We want to ship this soon, but we are ok with holding off until mid-December if you guys want to include something.
Sorry I do not have the capacity right now, but I would suggest to ship a fast fix: the test console command is right now not exactly useful, because it does not have a detailed output of the issue if the event is not going out. The new SDK major has a new Logger option that could be leveraged to redirect output to the console while running the test, making it more useful.
There are a few things I would like to talk about for the next major, personally. The first is a configuration change: I suggest disabling the tracing feature by default: I understand that Sentry wants its users to have everything configured out-of-box so they can get started right away, but the instrumentations are difficult to maintain and operate reliably without causing problems, and a quick search in this repository can prove it. In any case, nothing is sampled without manual user intervention in the configuration to set tracing_sample_rate to a value greater than 0, so in the end instrumentation already does nothing by default. Furthermore, the more integrations we add that are enabled by default, the more we impact the performance of projects without users knowing it: distributed tracing should be opt-in, not opt-out.
The second thing I would like to tackle is that our distributed tracing instrumentations are really hard to maintain because the requirements to allow them to be enabled and to be compatible with the installed dependencies cannot be specified in the composer.json. Basically, we have to resort to checking if some classes or interfaces exists to unlock the enabling of each instrumentation. Instead, a more appropriate approach would be to separate each instrumentation into its own Composer package, each with its own requirements: the package manager is good at solving dependencies and requirements, so we should let it do the job it was born for. The amount of conditionals, class aliases and polyfills tells everything of the current situation.
The third wish is a long-delayed refactoring of how we integrate with Symfony's error and exception handler (#337). We are currently using the core integrations, but they cause problems because the framework has its own handler and it doesn't really expect to be replaced. In the past, due to continued reporting of issues caused by the way we hooked into the error handling mechanism, the Flex recipe was changed to enable the bundle only in the prod environment. This in turn causes other problems, mainly the fact that since nothing is loaded in the rest of the environments, code using any of the services breaks unless the user decides to load the bundle in all environments. It's basically a dog chasing its tail. It's already possible to disable the affected integrations and manually configure Monolog to send its logs to Sentry: that's the right way to integrate with Symfony and we only have to make it the default, somehow.
The fourth thing, no less important, is the elimination of support for previous versions of Symfony. It is becoming increasingly difficult to support old versions, and the maintenance burden increases with each new version released, be it minor or major. We simply don't have the resources to support all of these versions, so letting go of at least Symfony 4.4 would be a great help. For example, to fix the HTTP client distributed tracing instrumentation for Symfony 4.4, I'm spending a huge amount of time to investigate what changed between 3 major versions of the framework, which is simple madness.
Most of this sounds reasonable, but this is sadly out of scope. I can only focus on the Symfony SDK for a short amount of time at the moment, hence these are topics more fitting for a v6.0.
However, if you want to work on this, we would need a rough timeline.
I would be fine using monolog by default in v5.0, but this would be a community effort.
Our main objective right now is to bump the PHP SDK and move on.
-
Disabling the tracing feature by default: Most stuff should no op when tracing is disabled. If there is a perf overhead, we should instead fix them. UX is important.
-
Distributed Tracing: Trace Propagation is no longer tied to performance/tracing and is enabled in all SDKs by default, besides JS due to CORS.
-
Each instrumentation into its own Composer package: Fine by me, but only as a mono repo.
-
Symfony 4: We can consider dropping this version, but I would need to check some numbers first.
- Each instrumentation into its own Composer package: Fine by me, but only as a mono repo.
This is a no-go because you would lose the main benefit: the separate repo would allow multiple branches to target multiple majors of the related dependencies (i.e. DBAL v2 vs v3)
There are some tools like https://github.com/symplify/monorepo-builder that make this possible. Not going to add more PHP repositories, sorry.
I'm not sure I understand how such tool would solve our issue. The requirement would be having cross compatibility between our Symfony SDK profiling and multiple versions of other supported libraries at the same time..
I would be fine using monolog by default in v5.0, but this would be a community effort.
There are some factors to consider to switch by default to Monolog, mainly around the fact that the logger configuration is not known in advance and so the risk of changing/breaking user settings is really high. There needs to be a thorough investigation, but if time is a key factor then I doubt we can make it in time for December. But it's definitely something we'll have to address sooner or later because it's a pain point for all users.
Disabling the tracing feature by default: Most stuff should no op when tracing is disabled. If there is a perf overhead, we should instead fix them. UX is important.
There's always a little overhead, no matter if most things are no-op when tracing is disabled. In most cases, when an instrumentation is disabled, the related services are removed from the container and therefore the performance impact is absolutely 0 simply because they don't even get instantiated. However, since by default everything is enabled, even if the event is eventually discarded, all the collection is still done. But what is more worrying is that the instrumentations hook into critical paths of their dependencies, and the risk of breaking the application is really high. We've had a few cases where things weren't working correctly, so it makes sense to disable instrumentation by default and let users choose, consciously, what they want to use. Right now, once the bundle is installed it hooks into everything it can and the user doesn't even know about it, finding out issues later on.
Each instrumentation into its own Composer package: Fine by me, but only as a mono repo.
Some good examples of what I meant are the transport packages for symfony/messenger: they are independent, they are developed in the monorepo and somehow the changes are replicated in the individual repositories. This ensures that we can support each package independently and that we can rely on Composer to manage dependency compatibility.
Symfony 4: We can consider dropping this version, but I would need to check some numbers first.
I would say that if the only reason for the existence of a v5 of this bundle is to allow the use of the latest SDK, then allowing Symfony 4.x or not doesn't really matter. However, it is not feasible to support all versions forever: at the moment, for every thing we do, be it a bugfix or a new feature, we have to spend a lot of time and energy to ensure that it works on 4 different versions of the framework. I understand that Sentry as a company wants to achieve the widest possible audience, but there are not enough resources to do so here, at least for now.
Ok, then I'll move forward with a slim v5.0. Thanks!
However, since by default everything is enabled, even if the event is eventually discarded, all the collection is still done.
Then we need to add more checks if a transaction is on the scope and return early. We do this in Laravel, and it works fine. Again, to make this very clear, we're not degrading UX because there is a hypothetical problem. If we break users' code, we fix it. If we slow down things too much, we fix it.
Again, to make this very clear, we're not degrading UX because there is a hypothetical problem.
I don't think that we're degrading any UX: as I explained, the user has to explicitly enable the tracing already by configuring the sample_tracer or the traces_sample_rate options, hence any instrumentation he wants to use already requires a manual intervention to be "enabled". The only thing that would change is that along with setting a sample rate, the user is also in control of which instrumentation he wants to activate in an explicit manner. When and if we will be moving the instrumentations to their own packages, it will be even easier: since it's the user that is in control of installing the package, if he does then it's fine to enable the instrumentation by default. But right now, it makes no sense because it takes away people's free will, and history teaches it's never a good thing.
But right now, it makes no sense because it takes away people's free will, and history teaches it's never a good thing.
lol
I don't know what I should look at, besides a documentation page that says that Sentry decided for me that instrumentation must be enabled by default, which again is what I'm saying shouldn't decide on my behalf. Unless you set the traces_sample_rate to a sensible value, those instrumentations are no-op, but are still hooking into the critical paths of the framework and of the dependencies. Exactly, which UX would be degraded more than now by documenting instead that along with the option mentioned before, you also have to set a flag to true for the integrations you are interested in?
We are not changing that.
Thanks for the constructive feedback, it's always a pleasure to talk with someone so open minded.
I suggest dropping any unsupported Symfony version, so set the minimum Symfony version to ^5.4
Also, supporting Symfony 7 would be nice, since it's about to be released in the next few days (it's in RC currently) and it's already supported in sentry/sentry
I saw the original time frame was mid-december 2023, I was wondering if there is any progress on this issue?
No progress as I had to shift focus onto other things.
Symfony 4: We can consider dropping this version, but I would need to check some numbers first.
It's hard to imagine any benefit to supporting Symfony 4. If you're releasing a new version, I think even dropping Symfony 5 would be okay. Anyone on the older versions can keep using the existing release.
Please keep Symfony 5 support since it is still maintained until November 2025 (see also: https://symfony.com/releases/5.4) and still used by some projects
Sure, Symfony 5 will continue to be supported, I question if it's worth the effort to update the Symfony 5 version to SDK 5. If people are on old versions of one library, they shouldn't expect the latest of another.
I don't know if Symfony 5 is holding back the integration of Sentry SDK 5, but if it is, I'd advocate for a new release that only supported PHP ^8.1 and Symfony ^6.4.
Maybe only 5.4?
Please keep Symfony 5 support since it is still maintained until November 2025 (see also: https://symfony.com/releases/5.4) and still used by some projects
as mentioned before, support for Symfony 5 will always be granted by older Sentry versions.
There's no need to support it in the last Sentry version.
Any idea when sentry 5 will be released?
Started working on v5 this week, so shouldn't be that much longer 🤞
Why does the project still require sentry/sdk 3.6 in the composer.json?
Why does the project still require sentry/sdk 3.6 in the composer.json?
Because it's still a work in progress?
Can we expect Sentry 5.0 release still in March?
Yes, March for sure! I‘m actually planning to ship this next week.
Yes, March for sure! I‘m actually planning to ship this next week.
bump after 2 weeks. whats up?
@cleptric do you need help with any problems? I appreciate that you are actively working on this code.
To be fully transparent, it's a bit of a hectic time at the moment, with my priorities shifting a lot. The PR is almost ready, with the remaining task of adding the newly added options from major version 5.0 of the PHP SDK needing to be completed. Thereafter, we have to update docs and get a new version of our flex recipe published.
🙏
Great news, when can we expect a new release to be published?
Version 5.0.0 has been released. Please consult the upgrade guide and changelog.