-
Notifications
You must be signed in to change notification settings - Fork 258
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
feat: Sentry logs #792
feat: Sentry logs #792
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.
Looks good to me. A couple of questions left.
- Is the logging of errors optional in the CLI? I seem to have missed the option to opt-out
- There are a couple of calls that have the 'suppressError' option. That means the call will never fail. yet you have added the sentry stuff there.
- The key for sentry seems hardcoded in Scully? shouldn't we use an environment var or something?
- can you explain the flush in
pluginwrap.ts
? I think it's not needed. The pluginsError is an option that when set, the program exists. Normally it just ignores plugin errors and carries on. (perhaps handle the flush in 'launchedBrowser.ts` where the whole exit is handled?
@aaronfrost should we make this mandatory? |
@amycdurbin should we make WHAT mandatory? That they opt into errors? Yes. It is not required to get permission for errors (I was told by Patrick). It is, however, required to get permission for telemetry data. So these errors are not telemetry. So let's do it. Also, let's get this merged already... I want to see the errors that the users are seeing. We are wasting daylight by not merging this PR. Get it done already. |
@SanderElias please see Frosty's response. |
@aaronfrost I'm pretty sure collecting any data (including errors) from a user's system goes right into GRPD. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
no error logging for the cli or docs
Issue Number: N/A
What is the new behavior?
error logging for the docs and cli
Does this PR introduce a breaking change?
Other information
fixes #783