Skip to content

Conversation

@spawnia
Copy link
Contributor

@spawnia spawnia commented Aug 1, 2023

This improves upon the storage integration as described in #745 (comment).

I believe extending the existing Laravel classes is valuable. We saw in #745 that users are expecting to receive specific classes instead of the insufficient interfaces.

Requiring users to do manual configuration of their disks is fine, but we can make it easier and less error-prone by offering wrapper methods.

During testing, I found that because the configuration is now static, it is always required to register the sentry driver. Otherwise, environments that do not set the DSN fail to access storage with Driver [sentry] is not supported.. First I thought this change was somewhat of a hack, but then realized it is actually a good thing; it means that the errors reported in #745 would now be found in local or testing environments already.

Testing in our application went fine, I successfully got the following spans and breadcrumbs:

image

image

@spawnia spawnia marked this pull request as ready for review August 2, 2023 09:58
@cleptric cleptric self-requested a review August 3, 2023 08:26
@spawnia
Copy link
Contributor Author

spawnia commented Aug 3, 2023

Thanks for the updates @stayallive. The new inheritance/implementation structure makes sense to me.

private function registerDiskDriver(): void
protected function registerDriver(): void
{
$this->container()->afterResolving(FilesystemManager::class, function (FilesystemManager $filesystemManager): void {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this line of code was written by you in the previous PR, I think it is not a good idea to extend the filesystemManager call when afterResolving it.
This is because the FilesystemManager class may be resolved before the SentryServiceProvider, and therefore, the callback will never be called.

Suggested change
$this->container()->afterResolving(FilesystemManager::class, function (FilesystemManager $filesystemManager): void {
$callback = function (FilesystemManager $filesystemManager): void {
$filesystemManager->extend(self::STORAGE_DRIVER_NAME, function (Application $application, array $config) use ($filesystemManager): Filesystem {
//...
});
};
if ($this->container()->resolved(FilesystemManager::class)) {
$callback($this->container()->get(FilesystemManager::class));
} else {
$this->container()->afterResolving(FilesystemManager::class, $callback);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should never happen. We register in the register callback of a service provider. And resolving should have never happened there so this should be perfectly fine unless somethin wonky is going on. Do you have a specific use case in mind where this would actually fail?

Copy link

@yankewei yankewei Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it actually happend in our project, but I need time to reproduce in a simple Laravel project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you have a service provider that is not well behaved, meaning that it does more than just register bindings.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you register the Sentry service provider manually and from inside another service provider, but we use the afterResolving callback on more places and I've never seen the issues you describe or heard people having them.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we have a service provider that calls $this->app->make('filesystem') in the boot method.

Copy link
Collaborator

@stayallive stayallive Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, we are doing this in the boot not register. I'll see what we can do to change that. But your suggestion wouldn't currently fix the problem since it will still be too late, but I see where the problem is.

Edit: working on solution here #759.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yankewei it took a bit, but I think we solved the issue and now register the driver before Laravel is booted which should solve the issues you were seeing!

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, still easy to enable but a little more "involved" that hopefuly makes people pay atttentiont to see if it works with their code 💪 Thanks all!

@stayallive stayallive merged commit cbec306 into getsentry:master Aug 29, 2023
@spawnia spawnia deleted the improve-storage-integration branch August 29, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants