-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add full command to extras #352
Conversation
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.
As said in the related issue, it can't be implemented like this, as a default behavior.
I'm generally favourable to adding logging of the command. Regarding the sanitization, As it's a full string I don't think that Sentry can hide or detect specific portions of it so that values of certain command arguments or options will be hidden... I wonder anyway why someone would pass sensitive data as an argument or option, since it would be visible in the CLI as much as in any log system. Bonus question: I'm really doubtful about whether the command should be logged in the extra context or as a breadcrumb |
According link in linked issue, laravel also passes whole thing as string, so I don't believe this is an issue. As for breadcrumbs, in my pov breadcrumbs are something which happens in sequence, like a log. I think command name does not quite fit in that definition, similarly like request url and so on. Especially when command name already is a tag instead of bread crumb and I'm implementing now just logging of full command name - but I deemed full command name being too unique for being a tag, hence second best was extra. |
True, but in Symfony the concept of subrequest exists as it exists the concept of a command started and running within another. I would say that logging as bredcrumbs both the start of a request and of a command would be useful. Anyway, I agree that at the moment logging the full command in the extras is enough
Absolutely agree with this claim |
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.
LGTM 👍 but please add a changelog entry
Build should be fixed by #480, but there are conflicts now. |
rebased |
Fixes #351