Skip to content
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

Capture command arguments #351

Closed
ostrolucky opened this issue Jun 28, 2020 · 13 comments · Fixed by #352
Closed

Capture command arguments #351

ostrolucky opened this issue Jun 28, 2020 · 13 comments · Fixed by #352
Milestone

Comments

@ostrolucky
Copy link
Contributor

CLI Commands are usually run with certain arguments. Error is also most of the time being triggered with certain CLI arguments only, this is why it's important to have it. Would you welcome contribution to log not only command name, but command arguments in your ConsoleListener?

@stayallive
Copy link
Collaborator

I'd be +1 for this since it's useful context, for example in Laravel we add the full input as context (looks like this) and also the exit code.

Code context (artisan being the name of Laravel CLI tool):

https://github.com/getsentry/sentry-laravel/blob/2dfcf9e0bd83856ee7abba25bb3ade8960081430/src/Sentry/Laravel/EventHandler.php#L472-L480
https://github.com/getsentry/sentry-laravel/blob/2dfcf9e0bd83856ee7abba25bb3ade8960081430/src/Sentry/Laravel/EventHandler.php#L492-L502

@Jean85
Copy link
Collaborator

Jean85 commented Jun 29, 2020

This was requested in the past, see #60 (comment).

It must not be the default behavior, to avoid exposing sensitive information.

@Jean85 Jean85 added this to the 3.5 milestone Jun 29, 2020
@ostrolucky
Copy link
Contributor Author

Isn't this a problem already in stack traces in general? Sentry should have a generic way how to combat exposing sensitive information.

@stayallive
Copy link
Collaborator

Yes, this value should already be sanitised. I believe both client and server side.

In Laravel we made a configuration option to disable these kind of "auto-breadcrumbs" (per type) and that could maybe be an option, but we do have it enabled by default there.

@Jean85
Copy link
Collaborator

Jean85 commented Jul 1, 2020

I would consider enabling it by default only in a major version bump, which is coming by the way thanks to #337.

Maybe we can add the feature off by default in 3.x and switch it to on by default in 4?

@ostrolucky
Copy link
Contributor Author

I would rather not add more toggles. So if adding it in major version is your only concern, let's just merge it to 4.

@Jean85 Jean85 modified the milestones: 3.5, 4.0 Jan 19, 2021
@michondr
Copy link

some time has passed, and I'm curious if this is available at the moment? I'd like to know how to reprocess these failed messages

image

@ostrolucky
Copy link
Contributor Author

There is #352, but for some unknown reason, this wasn't merged for 4.0.

@Jean85
Copy link
Collaborator

Jean85 commented Feb 22, 2021

Sorry, we missed the PR while refactoring toward 4.0. I'm happy to re-review it once it's rebased and the comments are addressed.

@ostrolucky
Copy link
Contributor Author

Rebased and comments addressed

@michondr
Copy link

michondr commented Apr 4, 2021

Rebased and comments addressed

any info where to find snippets of how to tweak it? or how to use it? how it works?

@Jean85
Copy link
Collaborator

Jean85 commented Apr 6, 2021

@michondr it's all in the linked PR: #352

@Jean85
Copy link
Collaborator

Jean85 commented Apr 19, 2021

Closing as #352 got merged.

@Jean85 Jean85 closed this as completed Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants