-
-
Notifications
You must be signed in to change notification settings - Fork 202
Improve storage integration #750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
stayallive
merged 17 commits into
getsentry:master
from
spawnia:improve-storage-integration
Aug 29, 2023
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
1ed014a
Extend Storage classes instead of implementing interfaces
spawnia db3be5f
Simplify configuration
spawnia 25308fb
Document improved configuration mechanism
spawnia a9d5817
Improve changelog
spawnia 0ad5bd2
Enable selective disk configuration
spawnia 4709fe1
Merge branch 'master' into improve-storage-integration
spawnia 618434c
Always register driver
spawnia 5b20c5f
Merge branch 'master' into improve-storage-integration
stayallive 0b7fe98
Be even more specific with type of filesystem
stayallive 694f106
Woops
stayallive 8884fd2
Remove duplicated comment
stayallive 33f464f
Move methods to specific decorators when applicable
stayallive e552877
Merge branch 'master' into improve-storage-integration
spawnia 18dea52
Merge branch 'master' into improve-storage-integration
stayallive 9b3e4dc
Test invalid configs throw exception
stayallive 4567d86
Merge branch 'master' into improve-storage-integration
stayallive 91da750
Merge branch 'master' into improve-storage-integration
stayallive File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 changes: 13 additions & 0 deletions
13
src/Sentry/Laravel/Features/Storage/CloudFilesystemDecorator.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| <?php | ||
|
|
||
| namespace Sentry\Laravel\Features\Storage; | ||
|
|
||
| trait CloudFilesystemDecorator | ||
| { | ||
| use FilesystemDecorator; | ||
|
|
||
| public function url($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
| } |
33 changes: 33 additions & 0 deletions
33
src/Sentry/Laravel/Features/Storage/FilesystemAdapterDecorator.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| <?php | ||
|
|
||
| namespace Sentry\Laravel\Features\Storage; | ||
|
|
||
| trait FilesystemAdapterDecorator | ||
| { | ||
| use CloudFilesystemDecorator; | ||
|
|
||
| public function assertExists($path, $content = null) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function assertMissing($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function assertDirectoryEmpty($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function temporaryUrl($path, $expiration, array $options = []) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path', 'expiration', 'options')); | ||
| } | ||
|
|
||
| public function temporaryUploadUrl($path, $expiration, array $options = []) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path', 'expiration', 'options')); | ||
| } | ||
| } |
188 changes: 188 additions & 0 deletions
188
src/Sentry/Laravel/Features/Storage/FilesystemDecorator.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| <?php | ||
|
|
||
| namespace Sentry\Laravel\Features\Storage; | ||
|
|
||
| use Illuminate\Contracts\Filesystem\Filesystem; | ||
| use Sentry\Breadcrumb; | ||
| use Sentry\Laravel\Integration; | ||
| use Sentry\Laravel\Util\Filesize; | ||
| use Sentry\Tracing\SpanContext; | ||
| use function Sentry\trace; | ||
|
|
||
| /** | ||
| * Decorates the underlying filesystem by wrapping all calls to it with tracing. | ||
| * | ||
| * Parameters such as paths, directories or options are attached to the span as data, | ||
| * parameters that contain file contents are omitted due to potential problems with | ||
| * payload size or sensitive data. | ||
| */ | ||
| trait FilesystemDecorator | ||
| { | ||
| /** @var Filesystem */ | ||
| protected $filesystem; | ||
|
|
||
| /** @var array */ | ||
| protected $defaultData; | ||
|
|
||
| /** @var bool */ | ||
| protected $recordSpans; | ||
|
|
||
| /** @var bool */ | ||
| protected $recordBreadcrumbs; | ||
|
|
||
| /** | ||
| * Execute the method on the underlying filesystem and wrap it with tracing and log a breadcrumb. | ||
| * | ||
| * @param list<mixed> $args | ||
| * @param array<string, mixed> $data | ||
| * | ||
| * @return mixed | ||
| */ | ||
| protected function withSentry(string $method, array $args, ?string $description, array $data) | ||
| { | ||
| $op = "file.{$method}"; // See https://develop.sentry.dev/sdk/performance/span-operations/#web-server | ||
| $data = array_merge($data, $this->defaultData); | ||
|
|
||
| if ($this->recordBreadcrumbs) { | ||
| Integration::addBreadcrumb(new Breadcrumb( | ||
| Breadcrumb::LEVEL_INFO, | ||
| Breadcrumb::TYPE_DEFAULT, | ||
| $op, | ||
| $description, | ||
| $data | ||
| )); | ||
| } | ||
|
|
||
| if ($this->recordSpans) { | ||
| $spanContext = new SpanContext; | ||
| $spanContext->setOp($op); | ||
| $spanContext->setData($data); | ||
| $spanContext->setDescription($description); | ||
|
|
||
| return trace(function () use ($method, $args) { | ||
| return $this->filesystem->{$method}(...$args); | ||
| }, $spanContext); | ||
| } | ||
|
|
||
| return $this->filesystem->{$method}(...$args); | ||
| } | ||
|
|
||
| public function exists($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function get($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function readStream($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function put($path, $contents, $options = []) | ||
| { | ||
| $description = is_string($contents) ? sprintf('%s (%s)', $path, Filesize::toHuman(strlen($contents))) : $path; | ||
|
|
||
| return $this->withSentry(__FUNCTION__, func_get_args(), $description, compact('path', 'options')); | ||
| } | ||
|
|
||
| public function writeStream($path, $resource, array $options = []) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path', 'options')); | ||
| } | ||
|
|
||
| public function getVisibility($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function setVisibility($path, $visibility) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path', 'visibility')); | ||
| } | ||
|
|
||
| public function prepend($path, $data, $separator = PHP_EOL) | ||
| { | ||
| $description = is_string($data) ? sprintf('%s (%s)', $path, Filesize::toHuman(strlen($data))) : $path; | ||
|
|
||
| return $this->withSentry(__FUNCTION__, func_get_args(), $description, compact('path')); | ||
| } | ||
|
|
||
| public function append($path, $data, $separator = PHP_EOL) | ||
| { | ||
| $description = is_string($data) ? sprintf('%s (%s)', $path, Filesize::toHuman(strlen($data))) : $path; | ||
|
|
||
| return $this->withSentry(__FUNCTION__, func_get_args(), $description, compact('path')); | ||
| } | ||
|
|
||
| public function delete($paths) | ||
| { | ||
| if (is_array($paths)) { | ||
| $data = compact('paths'); | ||
| $description = sprintf('%s paths', count($paths)); | ||
| } else { | ||
| $data = ['path' => $paths]; | ||
| $description = $paths; | ||
| } | ||
|
|
||
| return $this->withSentry(__FUNCTION__, func_get_args(), $description, $data); | ||
| } | ||
|
|
||
| public function copy($from, $to) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), sprintf('from "%s" to "%s"', $from, $to), compact('from', 'to')); | ||
| } | ||
|
|
||
| public function move($from, $to) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), sprintf('from "%s" to "%s"', $from, $to), compact('from', 'to')); | ||
| } | ||
|
|
||
| public function size($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function lastModified($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function files($directory = null, $recursive = false) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $directory, compact('directory', 'recursive')); | ||
| } | ||
|
|
||
| public function allFiles($directory = null) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $directory, compact('directory')); | ||
| } | ||
|
|
||
| public function directories($directory = null, $recursive = false) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $directory, compact('directory', 'recursive')); | ||
| } | ||
|
|
||
| public function allDirectories($directory = null) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $directory, compact('directory')); | ||
| } | ||
|
|
||
| public function makeDirectory($path) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $path, compact('path')); | ||
| } | ||
|
|
||
| public function deleteDirectory($directory) | ||
| { | ||
| return $this->withSentry(__FUNCTION__, func_get_args(), $directory, compact('directory')); | ||
| } | ||
|
|
||
| public function __call($name, $arguments) | ||
| { | ||
| return $this->filesystem->{$name}(...$arguments); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
registercallback 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?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
afterResolvingcallback on more places and I've never seen the issues you describe or heard people having them.There was a problem hiding this comment.
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 thebootmethod.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
bootnotregister. 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.
There was a problem hiding this comment.
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!