Add crash report size histogram

Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/metrics.cc?q=Crashpad.CrashReportSize&sq=package:chromium&l=24.

R=asvitkine@chromium.org
BUG=crashpad:100
Committed: https://crrev.com/7debf2dff50a1680fca4796516872bff67e5f44d
Cr-Commit-Position: refs/heads/master@{#419547}

[scottmg, 2016-09-19 17:16:15]

Description was changed from

==========
Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/....

BUG=crashpad:100
==========

to

==========
Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/....

R=asvitkine@chromium.org
BUG=crashpad:100
==========

[scottmg, 2016-09-19 17:16:15]

scottmg@chromium.org changed reviewers:
+ asvitkine@chromium.org

[scottmg, 2016-09-19 17:16:34]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 17:16:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 18:18:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 18:18:27]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-19 19:01:29]

lgtm % comment

FYI: In the future, it's a good idea to have both the XML and the code CL to be
reviewed at the same time - so that if the reviewer discovers an issue in the
code it can be addressed before landing.

https://codereview.chromium.org/2349993003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2349993003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:7401: +  <summary>The size of a crash
report minidump on disk.</summary>
Nit: Mention when it's recorded. At the time of sending?

[scottmg, 2016-09-19 19:06:04]

On 2016/09/19 19:01:29, Alexei Svitkine (very slow) wrote:
> lgtm % comment
> 
> FYI: In the future, it's a good idea to have both the XML and the code CL to
be
> reviewed at the same time - so that if the reviewer discovers an issue in the
> code it can be addressed before landing.

Yeah, I'll do it with the roll in the future (?). Unfortunately, it's a bit
tedious since Crashpad and histograms.xml are in different repos and use
different code review tools.

Would it be difficult to have Crashpad have its own .xml file that gets merged
with the main one in tools/metrics/histograms?

> 
>
https://codereview.chromium.org/2349993003/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2349993003/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:7401: +  <summary>The size of a crash
> report minidump on disk.</summary>
> Nit: Mention when it's recorded. At the time of sending?

[scottmg, 2016-09-19 19:06:10]

https://codereview.chromium.org/2349993003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2349993003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:7401: +  <summary>The size of a crash
report minidump on disk.</summary>
On 2016/09/19 19:01:29, Alexei Svitkine (very slow) wrote:
> Nit: Mention when it's recorded. At the time of sending?

Done.

[scottmg, 2016-09-19 19:06:23]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-09-19 19:06:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2349993003/#ps20001
(title: ".")

[commit-bot: I haz the power, 2016-09-19 19:06:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-09-19 19:33:58]

"Would it be difficult to have Crashpad have its own .xml file that gets
merged
with the main one in tools/metrics/histograms?"

From infrastructure side, it's not too difficult but it requires some work
on the server-side to set up. The other aspect of this is we (metrics team)
want to review all histograms changes and this is done via OWNERS in
Chromium. So if the histograms.xml is in another repository, then we'd need
to do something to support this (not sure how OWNERS work in Crashpad) and
also we'd need to start reviewing changes in this other repository - which
I'm not sure how much overhead there is to - is it using the same review UI
as Chromium?

Anything, not impossible but some effort to set up - and kind of low
priority for our team given the other things on our plate. (But if it's
something you're interested in helping set up, happy to guide you here.)

On Mon, Sep 19, 2016 at 3:06 PM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> CQ is trying da patch. Follow status at
>
> https://chromium-cq-status.appspot.com/v2/patch-status/
> codereview.chromium.org/2349993003/20001
>
>
> https://codereview.chromium.org/2349993003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2016-09-19 19:38:00]

On 2016/09/19 19:33:58, Alexei Svitkine (very slow) wrote:
> "Would it be difficult to have Crashpad have its own .xml file that gets
> merged
> with the main one in tools/metrics/histograms?"
> 
> From infrastructure side, it's not too difficult but it requires some work
> on the server-side to set up. The other aspect of this is we (metrics team)
> want to review all histograms changes and this is done via OWNERS in
> Chromium. So if the histograms.xml is in another repository, then we'd need
> to do something to support this (not sure how OWNERS work in Crashpad) and
> also we'd need to start reviewing changes in this other repository - which
> I'm not sure how much overhead there is to - is it using the same review UI
> as Chromium?

Crashpad is dogfooding PolyGerrit, which is what Chromium will switch to in the
future. So they're different right now, but eventually will be the same.

I guess for now, I'll add someone from tools/metrics/OWNERS to the code review
on the Crashpad side. (There will be one more "unreviewed" one, as it's already
landed, but not rolled yet. I'll make the histograms change during the roll for
that one.)

> 
> Anything, not impossible but some effort to set up - and kind of low
> priority for our team given the other things on our plate. (But if it's
> something you're interested in helping set up, happy to guide you here.)
> 
> On Mon, Sep 19, 2016 at 3:06 PM, mailto:commit-bot@chromium.org via
> http://codereview.chromium.org
<mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > CQ is trying da patch. Follow status at
> >
> > https://chromium-cq-status.appspot.com/v2/patch-status/
> > codereview.chromium.org/2349993003/20001
> >
> >
> > https://codereview.chromium.org/2349993003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-09-19 21:08:20]

Description was changed from

==========
Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/....

R=asvitkine@chromium.org
BUG=crashpad:100
==========

to

==========
Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/....

R=asvitkine@chromium.org
BUG=crashpad:100
==========

[commit-bot: I haz the power, 2016-09-19 21:08:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-19 21:10:54]

Description was changed from

==========
Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/....

R=asvitkine@chromium.org
BUG=crashpad:100
==========

to

==========
Add crash report size histogram

Code was added already:
https://cs.chromium.org/chromium/src/third_party/crashpad/crashpad/util/misc/....

R=asvitkine@chromium.org
BUG=crashpad:100

Committed: https://crrev.com/7debf2dff50a1680fca4796516872bff67e5f44d
Cr-Commit-Position: refs/heads/master@{#419547}
==========

[commit-bot: I haz the power, 2016-09-19 21:10:55]

Patchset 2 (id:??) landed as
https://crrev.com/7debf2dff50a1680fca4796516872bff67e5f44d
Cr-Commit-Position: refs/heads/master@{#419547}