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

electron - limit crash reporter to few insiders users #132961

Closed
wants to merge 2 commits into from
Closed

Conversation

bpasero
Copy link
Member

@bpasero bpasero commented Sep 13, 2021

This PR fixes #120902 by limiting the crash reporter to:

  • insiders only
  • users who's crash reporter ID ends with 0

@bpasero bpasero added this to the September 2021 milestone Sep 13, 2021
@bpasero bpasero self-assigned this Sep 13, 2021
product.quality === 'insider' && // only for insiders
typeof crashReporterId === 'string' && // only if we have a crash reporter ID
uuidPattern.test(crashReporterId) && // only if the crash reporter ID is valid
crashReporterId.endsWith('0') // only if the crash reporter ID ends with `0`
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a heuristic I picked, but maybe you have a better idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

@deepak1556 I ran some experiments generating 30000 UUIDs to roughly simulate our number of present insider users and typically end up with a number around ~1900 that match. Would you think that number if sufficient to find crashes?

crashReporterStartOptions.uploadToServer = true;
const crashReporterDirectory = this._environmentService.crashReporterDirectory;
const crashReporterId = this._environmentService.crashReporterId;
if (crashReporterDirectory || crashReporterId) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Now using these properties as a hint whether crash reporting is enabled at all.

@sadeghzare2580

This comment has been minimized.

Copy link
Contributor

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Discussions pending offline related to some logistic concerns with disabling the reporter in stable.

@bpasero bpasero modified the milestones: September 2021, October 2021 Sep 20, 2021
@bpasero bpasero modified the milestones: October 2021, Backlog Oct 4, 2021
@bpasero bpasero marked this pull request as draft October 4, 2021 16:53
@bpasero bpasero added the *out-of-scope Posted issue is not in scope of VS Code label Feb 14, 2022
@bpasero bpasero closed this Feb 14, 2022
@bpasero bpasero deleted the ben/120902 branch February 14, 2022 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
*out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electron: crash reporter slows down startup
3 participants