Send compressed histograms with system logs when sending feedback.

Send compressed histograms with system logs when sending feedback.

Send the metrics in JSON format so they can be imported and displayed
by the histograms page (in a forthcoming CL).

BUG=237288
R=rkc@chromium.org, xiyuan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235981

[michaelpg, 2013-11-01 00:42:32]

rkc: everything
jar: base/metrics
asargent: extensions
xiyuan: resources

Want to get this in for M32! Thanks.

[Alexei Svitkine (slow), 2013-11-01 05:10:37]

Has this gone through Chrome Privacy review?

[asargent_no_longer_on_chrome, 2013-11-01 18:15:03]

extensions parts rubber-stamp LGTM assuming rkc@ is ok with them, and the
question about privacy concerns is resolved.

[rkc, 2013-11-01 19:57:39]

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/feedback_private/feedback_private_api.cc
(right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/feedback_private/feedback_private_api.cc:220: if
(feedback_info.send_histograms) {
As mentioned in the IDL, this could just be a check the canary key in system
info.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_data.h (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.h:41: void SetAndCompressHistograms(const
std::string& histograms);
This should be a scoper for the same reasons why we use a scoper for
AttachAndCompressFileData. The data can potentially be very large, you don't
want to be copying that large a string if it can be avoided. Considering
GetHistograms returns a scoper, it should be trivial to change this to be a
scoper too.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.h:138: bool
histograms_compression_complete_;
Put a TODO over this block of booleans indicating that this needs to be
refactored to be a cleaner solution. Feel free to TODO(rkc) it.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/feedback/js/feedback.js (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
chrome/browser/resources/feedback/js/feedback.js:78: function
openHistogramsWindow() {
Move this code to a common function which both openHistogramsWindow and
openSystemInfoWindow can call.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
chrome/browser/resources/feedback/js/feedback.js:160:
feedbackInfo.sendHistograms = useHistograms;
See my comment on the idl.

https://codereview.chromium.org/55363003/diff/30001/chrome/common/extensions/...
File chrome/common/extensions/api/feedback_private.idl (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/common/extensions/...
chrome/common/extensions/api/feedback_private.idl:51: boolean sendHistograms;
I would rather not modify the .idl for this. We can instead add a canary field
to the system information itself like,
"sendHistograms: 'true'"

Then in the sendFeedback implementation, check if that field exists in system
information, if so, remove it from system information and attach the histograms.

(In fact, later we should move the traceId field also out of this idl and find a
cleaner solution for these extra payloads for the feedback report - but that is
a separate task that isn't in the scope of this CL).

[michaelpg, 2013-11-01 22:14:49]

@asvitkine No, this does not have a privacy signoff yet.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_data.h (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.h:41: void SetAndCompressHistograms(const
std::string& histograms);
On 2013/11/01 19:57:39, Rahul Chaturvedi wrote:
> This should be a scoper for the same reasons why we use a scoper for
> AttachAndCompressFileData. The data can potentially be very large, you don't
> want to be copying that large a string if it can be avoided. Considering
> GetHistograms returns a scoper, it should be trivial to change this to be a
> scoper too.

Done. Not sure why a scoped_ptr is preferred to a reference though.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.h:138: bool
histograms_compression_complete_;
On 2013/11/01 19:57:39, Rahul Chaturvedi wrote:
> Put a TODO over this block of booleans indicating that this needs to be
> refactored to be a cleaner solution. Feel free to TODO(rkc) it.

Done.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/feedback/js/feedback.js (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
chrome/browser/resources/feedback/js/feedback.js:78: function
openHistogramsWindow() {
On 2013/11/01 19:57:39, Rahul Chaturvedi wrote:
> Move this code to a common function which both openHistogramsWindow and
> openSystemInfoWindow can call.

How about this, or is it too much?

https://codereview.chromium.org/55363003/diff/30001/chrome/common/extensions/...
File chrome/common/extensions/api/feedback_private.idl (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/common/extensions/...
chrome/common/extensions/api/feedback_private.idl:51: boolean sendHistograms;
On 2013/11/01 19:57:39, Rahul Chaturvedi wrote:
> I would rather not modify the .idl for this. We can instead add a canary field
> to the system information itself like,
> "sendHistograms: 'true'"
> 
> Then in the sendFeedback implementation, check if that field exists in system
> information, if so, remove it from system information and attach the
histograms.
> 
> (In fact, later we should move the traceId field also out of this idl and find
a
> cleaner solution for these extra payloads for the feedback report - but that
is
> a separate task that isn't in the scope of this CL).

As we discussed we'll keep this in the .idl for now along with traceId.

[rkc, 2013-11-01 22:28:31]

lgtm

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
File chrome/browser/feedback/feedback_data.h (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/feedback/f...
chrome/browser/feedback/feedback_data.h:41: void SetAndCompressHistograms(const
std::string& histograms);
On 2013/11/01 22:14:50, Michael Giuffrida wrote:
> On 2013/11/01 19:57:39, Rahul Chaturvedi wrote:
> > This should be a scoper for the same reasons why we use a scoper for
> > AttachAndCompressFileData. The data can potentially be very large, you don't
> > want to be copying that large a string if it can be avoided. Considering
> > GetHistograms returns a scoper, it should be trivial to change this to be a
> > scoper too.
> 
> Done. Not sure why a scoped_ptr is preferred to a reference though.

The reference is not copied only during invocation of the method; the histogram_
= histogram statement in the implementation will still do a copy of the full
string.

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
File chrome/browser/resources/feedback/js/feedback.js (right):

https://codereview.chromium.org/55363003/diff/30001/chrome/browser/resources/...
chrome/browser/resources/feedback/js/feedback.js:78: function
openHistogramsWindow() {
On 2013/11/01 22:14:50, Michael Giuffrida wrote:
> On 2013/11/01 19:57:39, Rahul Chaturvedi wrote:
> > Move this code to a common function which both openHistogramsWindow and
> > openSystemInfoWindow can call.
> 
> How about this, or is it too much?

LG.

[xiyuan, 2013-11-01 22:45:37]

LGTM after the privacy concern is addressed.

[Alexei Svitkine (slow), 2013-11-02 00:05:26]

"@asvitkine No, this does not have a privacy signoff yet."

That's fine, as long you've reached out to them and they'll review this.

https://codereview.chromium.org/55363003/diff/210001/base/metrics/statistics_...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/55363003/diff/210001/base/metrics/statistics_...
base/metrics/statistics_recorder.cc:168: void
StatisticsRecorder::WriteJSON(const std::string& query,
Can you add some unit tests for this?

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/extension...
File chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc
(right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/extension...
chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc:67:
scoped_ptr<std::string> FeedbackServiceImpl::GetHistograms() {
It's pretty weird as an API to return a scoped_ptr<std::string>, it's not
commonly done.

I suggest instead using an out param here (and in related calls), i.e.:

bool FeedbackServiceImpl::GetHistograms(std::string* histograms)

Then, you don't need to alloc the string object on the heap here.

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_data.cc:169: scoped_ptr<std::string>
compressed_histograms(compressed_histograms_ptr);
Wouldn't |compressed_histograms_ptr| be freed as soon as |compressed_histograms|
goes out of scope? (i.e. right after the task is posted and the function ends)

The posted tasks will then get a dangling pointer...

Probably, you should just use a member variable to store this.

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_util.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_util.cc:232:
*(feedback_data->add_product_specific_binary_data()) = attachment;
Nit: Why not:

userfeedback::ProductSpecificBinaryData* attachment =
    feedback_data->add_product_specific_binary_data();
attachment->set_mime_type(kArbitraryMimeType);
attachment->set_name(name);
attachment->set_data(*data);

[michaelpg, 2013-11-02 17:39:08]

"That's fine, as long you've reached out to them and they'll review this."

Yes, we've started discussions with sumitg@/chrome-privacy-core@.

https://codereview.chromium.org/55363003/diff/210001/base/metrics/statistics_...
File base/metrics/statistics_recorder.cc (right):

https://codereview.chromium.org/55363003/diff/210001/base/metrics/statistics_...
base/metrics/statistics_recorder.cc:168: void
StatisticsRecorder::WriteJSON(const std::string& query,
On 2013/11/02 00:05:26, Alexei Svitkine wrote:
> Can you add some unit tests for this?

Done.

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/extension...
File chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc
(right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/extension...
chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc:67:
scoped_ptr<std::string> FeedbackServiceImpl::GetHistograms() {
On 2013/11/02 00:05:26, Alexei Svitkine wrote:
> It's pretty weird as an API to return a scoped_ptr<std::string>, it's not
> commonly done.
> 
> I suggest instead using an out param here (and in related calls), i.e.:
> 
> bool FeedbackServiceImpl::GetHistograms(std::string* histograms)
> 
> Then, you don't need to alloc the string object on the heap here.

Replaced scoped_ptr with out param.

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_data.cc:169: scoped_ptr<std::string>
compressed_histograms(compressed_histograms_ptr);
On 2013/11/02 00:05:26, Alexei Svitkine wrote:
> Wouldn't |compressed_histograms_ptr| be freed as soon as
|compressed_histograms|
> goes out of scope? (i.e. right after the task is posted and the function ends)
> 
> The posted tasks will then get a dangling pointer...
> 
> Probably, you should just use a member variable to store this.

ln 177: base::Passed(&compressed_histograms) lets us pass the compressed
histograms to OnCompressedHistogramsComplete as a scoped_ptr, in doing so it
nullifies compressed_histograms.

That's why we create compressed_histograms_ptr for use at ln 174, because
compressed_histograms.get() wouldn't work if the args to the second base::Bind
is evaluated first.

The same pattern has been in use for SetAndCompressSystemInfo, above.

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_util.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_util.cc:232:
*(feedback_data->add_product_specific_binary_data()) = attachment;
I'm not familiar with protobuf. It looks like RepeatedField is basically a
vector and doesn't pre-emptively allocate new memory except when adding data, so
your version would be safe provided the attachment pointer isn't used after the
next call to add_product_specific_binary_data(). Which is true here, since
AddAttachment is the only one using that function. But is it risky to rely on
those assumptions or is that okay?

[Alexei Svitkine (slow), 2013-11-04 15:26:40]

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_data.cc:169: scoped_ptr<std::string>
compressed_histograms(compressed_histograms_ptr);
On 2013/11/02 17:39:09, Michael Giuffrida wrote:
> On 2013/11/02 00:05:26, Alexei Svitkine wrote:
> > Wouldn't |compressed_histograms_ptr| be freed as soon as
> |compressed_histograms|
> > goes out of scope? (i.e. right after the task is posted and the function
ends)
> > 
> > The posted tasks will then get a dangling pointer...
> > 
> > Probably, you should just use a member variable to store this.
> 
> ln 177: base::Passed(&compressed_histograms) lets us pass the compressed
> histograms to OnCompressedHistogramsComplete as a scoped_ptr, in doing so it
> nullifies compressed_histograms.
> 
> That's why we create compressed_histograms_ptr for use at ln 174, because
> compressed_histograms.get() wouldn't work if the args to the second base::Bind
> is evaluated first.
> 
> The same pattern has been in use for SetAndCompressSystemInfo, above.

I see - thanks for the explanation. This pattern definitely looks very confusing
- and I don't like it, but if it's correct, then I guess its fine.

Though, it would also be good to have some tests that exercise this codepath, so
that the non-trivial memory ownership gets coverage on the memory bots. (Even if
it's correct now, this will ensure that a subtle refactor won't break it.)

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_util.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_util.cc:232:
*(feedback_data->add_product_specific_binary_data()) = attachment;
On 2013/11/02 17:39:09, Michael Giuffrida wrote:
> I'm not familiar with protobuf. It looks like RepeatedField is basically a
> vector and doesn't pre-emptively allocate new memory except when adding data,
so
> your version would be safe provided the attachment pointer isn't used after
the
> next call to add_product_specific_binary_data().

I don't think your statement is correct (that it's unsafe to use the pointer
after another add_* call). Per the docs, its managed as a vector of pointers:
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...

... which means that a grow of the array will not touch where the allocated
sub-elements are and thus shouldn't invalidate the pointers.

Also, the way I suggest is how its done elsewhere in Chrome (e.g. UMA logs
uploads) and is more efficient (avoids a copy of the element).

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/extension...
File
chrome/browser/extensions/api/feedback_private/feedback_service_nonchromeos.cc
(right):

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/extension...
chrome/browser/extensions/api/feedback_private/feedback_service_nonchromeos.cc:37:
virtual void GetHistograms(std::string*) OVERRIDE;
Nit: name the param

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_data.cc:95: if (histograms->empty() ||
Can the |histograms| param be null? If it can, please check for it here.

If it can't, then change it to const std::string& histograms to document that.

[michaelpg, 2013-11-04 17:25:18]

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_data.cc:170:
BrowserThread::PostBlockingPoolTaskAndReply(
I'll look into testing this.

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_util.cc (right):

https://codereview.chromium.org/55363003/diff/210001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_util.cc:232:
*(feedback_data->add_product_specific_binary_data()) = attachment;
You're right, I was thinking a RepeatedField rather than a RepeatedPtrField and
that the pointer pointed into the RepeatedField's allocated space somehow which
doesn't actually happen. Thanks.

On 2013/11/04 15:26:40, Alexei Svitkine wrote:
> On 2013/11/02 17:39:09, Michael Giuffrida wrote:
> > I'm not familiar with protobuf. It looks like RepeatedField is basically a
> > vector and doesn't pre-emptively allocate new memory except when adding
data,
> so
> > your version would be safe provided the attachment pointer isn't used after
> the
> > next call to add_product_specific_binary_data().
> 
> I don't think your statement is correct (that it's unsafe to use the pointer
> after another add_* call). Per the docs, its managed as a vector of pointers:
>
https://developers.google.com/protocol-buffers/docs/reference/cpp/google.prot...
> 
> ... which means that a grow of the array will not touch where the allocated
> sub-elements are and thus shouldn't invalidate the pointers.
> 
> Also, the way I suggest is how its done elsewhere in Chrome (e.g. UMA logs
> uploads) and is more efficient (avoids a copy of the element).

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/extension...
File
chrome/browser/extensions/api/feedback_private/feedback_service_nonchromeos.cc
(right):

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/extension...
chrome/browser/extensions/api/feedback_private/feedback_service_nonchromeos.cc:37:
virtual void GetHistograms(std::string*) OVERRIDE;
On 2013/11/04 15:26:40, Alexei Svitkine wrote:
> Nit: name the param

Done.

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/feedback/...
File chrome/browser/feedback/feedback_data.cc (right):

https://codereview.chromium.org/55363003/diff/390001/chrome/browser/feedback/...
chrome/browser/feedback/feedback_data.cc:95: if (histograms->empty() ||
On 2013/11/04 15:26:40, Alexei Svitkine wrote:
> Can the |histograms| param be null? If it can, please check for it here.
> 
> If it can't, then change it to const std::string& histograms to document that.

It shouldn't be null, so I've changed ZipHistograms and also ZipLogs, above.

[grt (UTC plus 2), 2013-11-18 15:32:21]

https://codereview.chromium.org/55363003/diff/720001/chrome/browser/extension...
File chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc
(right):

https://codereview.chromium.org/55363003/diff/720001/chrome/browser/extension...
chrome/browser/extensions/api/feedback_private/feedback_service_chromeos.cc:68:
base::StatisticsRecorder::WriteJSON(std::string(), histograms);
I landed a variation on your StatisticsRecorder change in
http://crrev.com/235191. Please rebase, drop your base/ changes, and change this
to:

  *histograms = base::StatisticsRecorder::ToJSON(std::string());

Thanks.

[commit-bot: I haz the power, 2013-11-19 07:35:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelpg@chromium.org/55363003/860015

[commit-bot: I haz the power, 2013-11-19 12:04:19]

Change committed as 235981