Client side crash sampling in Chrome.

45 views
Skip to first unread message

Jesse Doherty

unread,
Jul 28, 2016, 10:12:53 AM7/28/16
to crashp...@chromium.org, Alexei Svitkine, Gayane Petrosyan
Hey,

We're implementing client side sampling of UMA reports and crash reports, for windows and android.

I have 2 questions about windows.

  1. On windows, it looks like enabling uploads at initialization are gated on the crash reporter client's GetCollectStatsConsent in crashpad.cc, which is based off install_static::GetCollectStatsConsent() in chrome_crash_reporter_client_win.cc
    Would it make sense to add logic to the windows reporter client's GetCollectStatsConsent to return false when the client is sampled out, even though they have given consent? Here is the cl where I add the UMA sampling.
  2. I notice on mac crash_reporter::SetUploadsEnabled is used to update the crash reported in GoogleUpdateSettings::SetCollectStatsConsent, which is called from InitiateMetricsReportingChange, which is affected by the settings UI to turn crash and metrics on and off. Is there a reason why this wasn't done on windows?

Android currently looks like it's running it's own crash upload code, and is controlled by the same prefs that control UMA.

Mark Mentovai

unread,
Jul 28, 2016, 1:49:13 PM7/28/16
to Jesse Doherty, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.

You should be able to make this sampling change without making any changes to Crashpad internals. It should be possible to just grant or withhold upload permission to Crashpad to achieve this.

However, we should discuss the implications of this change on what we’d like to do in bug 620762. There, we want to be able to have the user request crash report upload from the about:crashes page, even if the user didn’t opt in to crash reporting (or, now, if they opted out). What should happen there if a sampled-out user requests report upload?

--
You received this message because you are subscribed to the Google Groups "Crashpad-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to crashpad-dev...@chromium.org.
To post to this group, send email to crashp...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/crashpad-dev/CA%2BZejxtt9rLMG_1r57SG0mtt40KSYtzOyQXfRXZGnJvmfBNsEw%40mail.gmail.com.

Jesse Doherty

unread,
Jul 28, 2016, 4:13:58 PM7/28/16
to Mark Mentovai, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
On Thu, Jul 28, 2016 at 1:49 PM Mark Mentovai <ma...@chromium.org> wrote:
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?
Previously, The main UMA data didn't require a restart, but other UMA data did, e.g. RAPPOR. I landed a CL this week to fix that.

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.
Are you suggesting put the check in crash_reporter::SetUploadsEnabled?

You should be able to make this sampling change without making any changes to Crashpad internals. It should be possible to just grant or withhold upload permission to Crashpad to achieve this.
I'm hoping for not changing crashpad internals...

However, we should discuss the implications of this change on what we’d like to do in bug 620762. There, we want to be able to have the user request crash report upload from the about:crashes page, even if the user didn’t opt in to crash reporting (or, now, if they opted out). What should happen there if a sampled-out user requests report upload?
 That bug was actually filed due to a conversation about sampling! We'd like for the case of a sampled-out user requesting a report upload to result in it being uploaded. Otherwise we think it would be confusing for a user that wants to upload.

Mark Mentovai

unread,
Jul 28, 2016, 4:19:08 PM7/28/16
to Jesse Doherty, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
Jesse Doherty wrote:
On Thu, Jul 28, 2016 at 1:49 PM Mark Mentovai <ma...@chromium.org> wrote:
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?
Previously, The main UMA data didn't require a restart, but other UMA data did, e.g. RAPPOR. I landed a CL this week to fix that.

Then we should definitely make Windows not require a restart in order for the Crashpad upload setting change to take effect, and we should get rid of the “requires restart” text there too.

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.
Are you suggesting put the check in crash_reporter::SetUploadsEnabled?

Yes, seems like the best spot in the funnel for it, since crash_reporter::SetUploadsEnabled can be reached both from crash_reporter::InitializeCrashpadImpl() and Mac’s GoogleUpdateSettings::SetCollectStatsConsent() (and, per above, soon the Windows equivalent too).

You should be able to make this sampling change without making any changes to Crashpad internals. It should be possible to just grant or withhold upload permission to Crashpad to achieve this.
I'm hoping for not changing crashpad internals...

However, we should discuss the implications of this change on what we’d like to do in bug 620762. There, we want to be able to have the user request crash report upload from the about:crashes page, even if the user didn’t opt in to crash reporting (or, now, if they opted out). What should happen there if a sampled-out user requests report upload?
 That bug was actually filed due to a conversation about sampling! We'd like for the case of a sampled-out user requesting a report upload to result in it being uploaded. Otherwise we think it would be confusing for a user that wants to upload.

OK, that’s good. It’s probably the easiest to implement on the Crashpad side too, since we were already anticipating handling “checkbox is off + user-requested upload = upload”. Since the bit that goes into Crashpad will now be “checkbox is off or user is sampled out”, the rest of the logic can remain the same and we’ll still reach the desired end result (upload).

Is there a plan to do something to the about:crashes page? Presently, we show both uploaded and not-uploaded crashes there. As things stand now, we’d probably look broken to users in the checkbox on, sampled-out state, because according to the UI and their preference, uploads should be on, but according to about:crashes (and reality), nothing will be uploaded.

Jesse Doherty

unread,
Aug 3, 2016, 12:02:56 PM8/3/16
to Mark Mentovai, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
On Thu, Jul 28, 2016 at 4:19 PM Mark Mentovai <ma...@chromium.org> wrote:
Jesse Doherty wrote:
On Thu, Jul 28, 2016 at 1:49 PM Mark Mentovai <ma...@chromium.org> wrote:
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?
Previously, The main UMA data didn't require a restart, but other UMA data did, e.g. RAPPOR. I landed a CL this week to fix that.

Then we should definitely make Windows not require a restart in order for the Crashpad upload setting change to take effect, and we should get rid of the “requires restart” text there too.

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.
Are you suggesting put the check in crash_reporter::SetUploadsEnabled?

Yes, seems like the best spot in the funnel for it, since crash_reporter::SetUploadsEnabled can be reached both from crash_reporter::InitializeCrashpadImpl() and Mac’s GoogleUpdateSettings::SetCollectStatsConsent() (and, per above, soon the Windows equivalent too).
I've been looking into how the SetUploadsEnabled (through crash_reporter::InitializeCrashpadImpl) is reached in windows. The only place I can see this coming from is through chrome_elf_main, am I missing something? My understanding was that chrome_elf created a bare-bones reporter, which then chrome starts it's own reported and replaces the one from elf.


You should be able to make this sampling change without making any changes to Crashpad internals. It should be possible to just grant or withhold upload permission to Crashpad to achieve this.
I'm hoping for not changing crashpad internals...

However, we should discuss the implications of this change on what we’d like to do in bug 620762. There, we want to be able to have the user request crash report upload from the about:crashes page, even if the user didn’t opt in to crash reporting (or, now, if they opted out). What should happen there if a sampled-out user requests report upload?
 That bug was actually filed due to a conversation about sampling! We'd like for the case of a sampled-out user requesting a report upload to result in it being uploaded. Otherwise we think it would be confusing for a user that wants to upload.

OK, that’s good. It’s probably the easiest to implement on the Crashpad side too, since we were already anticipating handling “checkbox is off + user-requested upload = upload”. Since the bit that goes into Crashpad will now be “checkbox is off or user is sampled out”, the rest of the logic can remain the same and we’ll still reach the desired end result (upload).

Is there a plan to do something to the about:crashes page? Presently, we show both uploaded and not-uploaded crashes there. As things stand now, we’d probably look broken to users in the checkbox on, sampled-out state, because according to the UI and their preference, uploads should be on, but according to about:crashes (and reality), nothing will be uploaded.
There are some plans, nothing very concrete though. It's not clear to me that we should communicate anything about sampling to the user, but it could dispel some confusion...

Do you know what milestone the manual upload change is being targeted for?


Scott Graham

unread,
Aug 3, 2016, 12:12:17 PM8/3/16
to Jesse Doherty, Mark Mentovai, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
On Wed, Aug 3, 2016 at 9:02 AM, 'Jesse Doherty' via Crashpad-dev <crashp...@chromium.org> wrote:


On Thu, Jul 28, 2016 at 4:19 PM Mark Mentovai <ma...@chromium.org> wrote:
Jesse Doherty wrote:
On Thu, Jul 28, 2016 at 1:49 PM Mark Mentovai <ma...@chromium.org> wrote:
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?
Previously, The main UMA data didn't require a restart, but other UMA data did, e.g. RAPPOR. I landed a CL this week to fix that.

Then we should definitely make Windows not require a restart in order for the Crashpad upload setting change to take effect, and we should get rid of the “requires restart” text there too.

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.
Are you suggesting put the check in crash_reporter::SetUploadsEnabled?

Yes, seems like the best spot in the funnel for it, since crash_reporter::SetUploadsEnabled can be reached both from crash_reporter::InitializeCrashpadImpl() and Mac’s GoogleUpdateSettings::SetCollectStatsConsent() (and, per above, soon the Windows equivalent too).
I've been looking into how the SetUploadsEnabled (through crash_reporter::InitializeCrashpadImpl) is reached in windows. The only place I can see this coming from is through chrome_elf_main, am I missing something? My understanding was that chrome_elf created a bare-bones reporter, which then chrome starts it's own reported and replaces the one from elf.

That was the case for a while: it used to be that chrome_elf used Breakpad during its initialization, and then when Chrome initialized it switched over to Crashpad. But after the changes in https://bugs.chromium.org/p/chromium/issues/detail?id=604923 there's only one registration (to Crashpad) which chrome_elf does at its initialization time.
 



You should be able to make this sampling change without making any changes to Crashpad internals. It should be possible to just grant or withhold upload permission to Crashpad to achieve this.
I'm hoping for not changing crashpad internals...

However, we should discuss the implications of this change on what we’d like to do in bug 620762. There, we want to be able to have the user request crash report upload from the about:crashes page, even if the user didn’t opt in to crash reporting (or, now, if they opted out). What should happen there if a sampled-out user requests report upload?
 That bug was actually filed due to a conversation about sampling! We'd like for the case of a sampled-out user requesting a report upload to result in it being uploaded. Otherwise we think it would be confusing for a user that wants to upload.

OK, that’s good. It’s probably the easiest to implement on the Crashpad side too, since we were already anticipating handling “checkbox is off + user-requested upload = upload”. Since the bit that goes into Crashpad will now be “checkbox is off or user is sampled out”, the rest of the logic can remain the same and we’ll still reach the desired end result (upload).

Is there a plan to do something to the about:crashes page? Presently, we show both uploaded and not-uploaded crashes there. As things stand now, we’d probably look broken to users in the checkbox on, sampled-out state, because according to the UI and their preference, uploads should be on, but according to about:crashes (and reality), nothing will be uploaded.
There are some plans, nothing very concrete though. It's not clear to me that we should communicate anything about sampling to the user, but it could dispel some confusion...

Do you know what milestone the manual upload change is being targeted for?


--
You received this message because you are subscribed to the Google Groups "Crashpad-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to crashpad-dev...@chromium.org.
To post to this group, send email to crashp...@chromium.org.

Jesse Doherty

unread,
Aug 3, 2016, 3:26:34 PM8/3/16
to Scott Graham, Mark Mentovai, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
On Wed, Aug 3, 2016 at 12:12 PM Scott Graham <sco...@chromium.org> wrote:
On Wed, Aug 3, 2016 at 9:02 AM, 'Jesse Doherty' via Crashpad-dev <crashp...@chromium.org> wrote:


On Thu, Jul 28, 2016 at 4:19 PM Mark Mentovai <ma...@chromium.org> wrote:
Jesse Doherty wrote:
On Thu, Jul 28, 2016 at 1:49 PM Mark Mentovai <ma...@chromium.org> wrote:
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?
Previously, The main UMA data didn't require a restart, but other UMA data did, e.g. RAPPOR. I landed a CL this week to fix that.

Then we should definitely make Windows not require a restart in order for the Crashpad upload setting change to take effect, and we should get rid of the “requires restart” text there too.

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.
Are you suggesting put the check in crash_reporter::SetUploadsEnabled?

Yes, seems like the best spot in the funnel for it, since crash_reporter::SetUploadsEnabled can be reached both from crash_reporter::InitializeCrashpadImpl() and Mac’s GoogleUpdateSettings::SetCollectStatsConsent() (and, per above, soon the Windows equivalent too).
I've been looking into how the SetUploadsEnabled (through crash_reporter::InitializeCrashpadImpl) is reached in windows. The only place I can see this coming from is through chrome_elf_main, am I missing something? My understanding was that chrome_elf created a bare-bones reporter, which then chrome starts it's own reported and replaces the one from elf.

That was the case for a while: it used to be that chrome_elf used Breakpad during its initialization, and then when Chrome initialized it switched over to Crashpad. But after the changes in https://bugs.chromium.org/p/chromium/issues/detail?id=604923 there's only one registration (to Crashpad) which chrome_elf does at its initialization time.
Ok, good, I'm not crazy.

So, my current plan is to add a GetCollectStatsInSample method to CrashReporterClient (with it returning true by default).  Windows will implement it by querying a registry value. This registry value will be set and updated at the same time that we update the running state of metrics services (at startup and when toggling the consent state), I'll also have to call SetUploadsEnabled in those places to get crashpad in the right state. This has the consequence that first-run crashes that occur before the rest of metrics services are initialized will not have sampling applied (since the reg value won't be setup). We're currently believing this won't be a big problem. It has the positive consequence that crash reporting on windows wouldn't need a restart to toggle (assuming SetUploadsEnabled just works).

Mark had suggested putting the sampling check inside crash_reporter::SetUploadsEnabled. Is this still sounding like a good idea, or should the value passed to SetUploadsEnabled already have the sampling state incorporated?

Scott Graham

unread,
Aug 3, 2016, 4:32:39 PM8/3/16
to Jesse Doherty, Mark Mentovai, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
On Wed, Aug 3, 2016 at 12:26 PM, Jesse Doherty <j...@google.com> wrote:


On Wed, Aug 3, 2016 at 12:12 PM Scott Graham <sco...@chromium.org> wrote:
On Wed, Aug 3, 2016 at 9:02 AM, 'Jesse Doherty' via Crashpad-dev <crashp...@chromium.org> wrote:


On Thu, Jul 28, 2016 at 4:19 PM Mark Mentovai <ma...@chromium.org> wrote:
Jesse Doherty wrote:
On Thu, Jul 28, 2016 at 1:49 PM Mark Mentovai <ma...@chromium.org> wrote:
Today, toggling the upload pref has an immediate effect on Mac but still requires a browser restart on Windows. There’s no reason it can’t have an immediate effect on both platforms, and in fact making it more “live” is desirable. See bug 629618. I think that we didn’t do it there because Breakpad didn’t do it either and we didn’t want to change too much at once, although Breakpad didn’t do it for Mac and we had no problem changing it there. Be aware that if we change this, we also need to make sure to get rid of the “requires restart” text from the checkbox on Windows. (It’s gone on Mac.) See here. Does toggling that pref have an immediate effect on UMA without requiring restart?
Previously, The main UMA data didn't require a restart, but other UMA data did, e.g. RAPPOR. I landed a CL this week to fix that.

Then we should definitely make Windows not require a restart in order for the Crashpad upload setting change to take effect, and we should get rid of the “requires restart” text there too.

From Crashpad’s perspective, this is all very simple: it’s told that it’s either allowed or not allowed to upload. This is done by calling crashpad::Settings::SetUploadsEnabled(). On the Chrome side, the argument to that function is basically the value of the metrics/crash reporting checkbox (with twists for policy and headless mode). I imagine that you’ll want to change it to be (checkbox_checked && !is_sampled_out). Chrome has a single entry point to that function, crash_reporter::SetUploadsEnabled(). Because SetUploadsEnabled currently basically expects to see the value of the checkbox, it’s probably the best spot to check for sampled-out, unless you have a more invasive refactoring in mind.
Are you suggesting put the check in crash_reporter::SetUploadsEnabled?

Yes, seems like the best spot in the funnel for it, since crash_reporter::SetUploadsEnabled can be reached both from crash_reporter::InitializeCrashpadImpl() and Mac’s GoogleUpdateSettings::SetCollectStatsConsent() (and, per above, soon the Windows equivalent too).
I've been looking into how the SetUploadsEnabled (through crash_reporter::InitializeCrashpadImpl) is reached in windows. The only place I can see this coming from is through chrome_elf_main, am I missing something? My understanding was that chrome_elf created a bare-bones reporter, which then chrome starts it's own reported and replaces the one from elf.

That was the case for a while: it used to be that chrome_elf used Breakpad during its initialization, and then when Chrome initialized it switched over to Crashpad. But after the changes in https://bugs.chromium.org/p/chromium/issues/detail?id=604923 there's only one registration (to Crashpad) which chrome_elf does at its initialization time.
Ok, good, I'm not crazy.

So, my current plan is to add a GetCollectStatsInSample method to CrashReporterClient (with it returning true by default).  Windows will implement it by querying a registry value. This registry value will be set and updated at the same time that we update the running state of metrics services (at startup and when toggling the consent state), I'll also have to call SetUploadsEnabled in those places to get crashpad in the right state. This has the consequence that first-run crashes that occur before the rest of metrics services are initialized will not have sampling applied (since the reg value won't be setup). We're currently believing this won't be a big problem. It has the positive consequence that crash reporting on windows wouldn't need a restart to toggle (assuming SetUploadsEnabled just works).

Mark had suggested putting the sampling check inside crash_reporter::SetUploadsEnabled. Is this still sounding like a good idea,

I think that sounds reasonable still.

Mark Mentovai

unread,
Aug 3, 2016, 4:48:34 PM8/3/16
to Scott Graham, Jesse Doherty, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
Scott Graham wrote:
Mark had suggested putting the sampling check inside crash_reporter::SetUploadsEnabled. Is this still sounding like a good idea,

I think that sounds reasonable still.

If the sampling check itself can’t live in components/crash, then perhaps it can be a callback in CrashReporterClient, but I still think that making the check (whether it’s directly or calling a client-provided callback) should happen from crash_reporter::SetUploadsEnabled().

Jesse Doherty

unread,
Aug 3, 2016, 5:37:30 PM8/3/16
to Mark Mentovai, Scott Graham, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
I don't think it can be in components/crash because I think the only thing I can depend on for the state at chrome_elf time is the registry, and I don't know of any way to get the registry value that's valid at chrome_elf time that's accessible from components. Admittedly, my knowledge is pretty limited here.

To make sure I'm understanding your callback suggestion, I'd add a function, e.g. crash_reporter::SetSamplingCallback (probably need a better name), and the client would be responsible for setting it. This would I guess be similar to the SetCrashReporterClient function?

Mark Mentovai

unread,
Aug 3, 2016, 5:52:25 PM8/3/16
to Jesse Doherty, Scott Graham, Crashpad-dev, Alexei Svitkine, Gayane Petrosyan
For the callback, I mean that you’d add a new bool-returning virtual method to CrashReporterClient (see components/crash/content/app/crash_reporter_client.h). Take a look at how crashpad.cc uses GetCrashReporterClient(). crashpad_win.cc and crashpad_mac.mm do, too. For that matter, so do the Breakpad crash component implementations.
Reply all
Reply to author
Forward
0 new messages