Add TranslateEventProto to UMA metrics

Add TranslateEventProto.

BUG=653700, 634961
Committed: https://crrev.com/01e12a12485f430701ec9c47974e71b3ecc99563
Cr-Commit-Position: refs/heads/master@{#423969}

[hamelphi, 2016-10-04 21:06:55]

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

[hamelphi, 2016-10-04 21:06:55]

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

[hamelphi, 2016-10-04 21:06:55]

This CL depends on the google3 pending CL. I figured that we may start doing the
review process to import the proto in Chrome so that we are ready to commit as
soon as we got LGTM in google3.

[hamelphi, 2016-10-04 21:06:55]

This CL depends on the google3 pending CL. I figured that we may start doing the
review process to import the proto in Chrome so that we are ready to commit as
soon as we got LGTM in google3.

[Alexei Svitkine (slow), 2016-10-05 17:18:46]

Looks fine. Will wait for the google3 CL to be submitting before stamping
just to make sure you have all the changes synced.

On Tue, Oct 4, 2016 at 5:06 PM, <hamelphi@chromium.org> wrote:

> Reviewers: Alexei Svitkine (slow)
> CL: https://codereview.chromium.org/2394643002/
>
> Message:
> This CL depends on the google3 pending CL. I figured that we may start
> doing the
> review process to import the proto in Chrome so that we are ready to
> commit as
> soon as we got LGTM in google3.
>
> Description:
> Add TranslateEventProto.
>
> BUG=
>
> Affected files (+140, -1 lines):
> M components/metrics/proto/BUILD.gn
> M components/metrics/proto/chrome_user_metrics_extension.proto
> A components/metrics/proto/translate_event.proto
>
>
>

-- 
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.

[hamelphi, 2016-10-06 21:11:01]

Proto landed in Google3.

PTAL.

[Alexei Svitkine (slow), 2016-10-06 21:21:36]

LGTM assuming proto corresponds to what landed in google3.

Please file a crbug for this line of work and set on the BUG= line. Thanks!

[hamelphi, 2016-10-06 22:02:10]

Description was changed from

==========
Add TranslateEventProto.

BUG=
==========

to

==========
Add TranslateEventProto.

BUG=653700
==========

[hamelphi, 2016-10-06 22:02:36]

The CQ bit was checked by hamelphi@chromium.org

[commit-bot: I haz the power, 2016-10-06 22:03:01]

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-10-06 22:55:51]

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

[commit-bot: I haz the power, 2016-10-06 22:55:52]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Roger McFarlane (Chromium), 2016-10-07 14:13:43]

rogerm@chromium.org changed reviewers:
+ rogerm@chromium.org

[Roger McFarlane (Chromium), 2016-10-07 14:13:44]

not LGTM

there's an error in the field ids.

https://codereview.chromium.org/2394643002/diff/80001/components/metrics/prot...
File components/metrics/proto/chrome_user_metrics_extension.proto (right):

https://codereview.chromium.org/2394643002/diff/80001/components/metrics/prot...
components/metrics/proto/chrome_user_metrics_extension.proto:25: 
This doesn't match the proto on the server side.

s/15/16/

https://codereview.chromium.org/2394643002/diff/80001/components/metrics/prot...
components/metrics/proto/chrome_user_metrics_extension.proto:72: repeated
TranslateEventProto translate_event = 14;
This doesn't match the proto on the server side.

s/14/15/

[hamelphi, 2016-10-07 14:37:23]

Fixed. PTAL

[Roger McFarlane (Chromium), 2016-10-07 16:06:45]

lgtm

[hamelphi, 2016-10-07 17:14:23]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2016-10-07 17:14:23]

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/2394643002/#ps100001
(title: "Fix id error in the proto to match google3.")

[commit-bot: I haz the power, 2016-10-07 17:14:51]

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-10-07 18:01:32]

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

[commit-bot: I haz the power, 2016-10-07 18:01:33]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[hamelphi, 2016-10-07 18:19:32]

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

[commit-bot: I haz the power, 2016-10-07 18:20:07]

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

[hamelphi, 2016-10-07 18:59:04]

The CQ bit was unchecked by hamelphi@chromium.org

[hamelphi, 2016-10-07 19:00:36]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2016-10-07 19:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2394643002/#ps120001
(title: "Change IGNORE to IGNORED to avoid a collision with a macro in windows
build.")

[commit-bot: I haz the power, 2016-10-07 19:01:05]

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

[hamelphi, 2016-10-07 19:04:47]

The CQ bit was unchecked by hamelphi@chromium.org

[Roger McFarlane (Chromium), 2016-10-07 19:21:01]

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
File components/metrics/proto/translate_event.proto (right):

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:116: // Feedback event received
from translate UI. If several feedback are received
s/Feedback event/Event/
s/If several feedback are/If several events are/

I'm not sure the "we keep only the last one" part is true anymore. IIRC, this
was in regard to successive modifications of the source and/or target languages?
We only generate an event when the user actually accepts/declines/etc the
translation. We won't capture meta events.

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:118: optional EventType
feedback_event = 10;
s/feedback_event/event_type/

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:127: optional int64
feedback_timestamp_sec = 11;
drop the feedback prefix?

[Roger McFarlane (Chromium), 2016-10-07 19:23:16]

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
File components/metrics/proto/translate_event.proto (right):

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:10: option java_outer_classname =
"OmniboxEventProtos";
copy paste error?

[hamelphi, 2016-10-07 19:27:05]

On 2016/10/07 19:21:01, Roger McFarlane (Chromium) wrote:
>
https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
> File components/metrics/proto/translate_event.proto (right):
> 
>
https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
> components/metrics/proto/translate_event.proto:116: // Feedback event received
> from translate UI. If several feedback are received
> s/Feedback event/Event/
> s/If several feedback are/If several events are/
> 
Done.

> I'm not sure the "we keep only the last one" part is true anymore. IIRC, this
> was in regard to successive modifications of the source and/or target
languages?
> We only generate an event when the user actually accepts/declines/etc the
> translation. We won't capture meta events.
True. Done.

> 
>
https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
> components/metrics/proto/translate_event.proto:118: optional EventType
> feedback_event = 10;
> s/feedback_event/event_type/
> 
Done.

>
https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
> components/metrics/proto/translate_event.proto:127: optional int64
> feedback_timestamp_sec = 11;
> drop the feedback prefix?
Done.

[hamelphi, 2016-10-07 19:29:50]

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
File components/metrics/proto/translate_event.proto (right):

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:10: option java_outer_classname =
"OmniboxEventProtos";
On 2016/10/07 19:23:16, Roger McFarlane (Chromium) wrote:
> copy paste error?

Done.

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:116: // Feedback event received
from translate UI. If several feedback are received
On 2016/10/07 19:21:00, Roger McFarlane (Chromium) wrote:
> s/Feedback event/Event/
> s/If several feedback are/If several events are/
> 
> I'm not sure the "we keep only the last one" part is true anymore. IIRC, this
> was in regard to successive modifications of the source and/or target
languages?
> We only generate an event when the user actually accepts/declines/etc the
> translation. We won't capture meta events.

Done.

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:118: optional EventType
feedback_event = 10;
On 2016/10/07 19:21:00, Roger McFarlane (Chromium) wrote:
> s/feedback_event/event_type/

Done.

https://codereview.chromium.org/2394643002/diff/140001/components/metrics/pro...
components/metrics/proto/translate_event.proto:127: optional int64
feedback_timestamp_sec = 11;
On 2016/10/07 19:21:01, Roger McFarlane (Chromium) wrote:
> drop the feedback prefix?

Done.

[Roger McFarlane (Chromium), 2016-10-07 19:34:15]

lgtm

[hamelphi, 2016-10-07 19:36:24]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2016-10-07 19:36:25]

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/2394643002/#ps170001
(title: "One more bug fix.")

[commit-bot: I haz the power, 2016-10-07 19:36:51]

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

[Roger McFarlane (Chromium), 2016-10-07 21:02:27]

Description was changed from

==========
Add TranslateEventProto.

BUG=653700
==========

to

==========
Add TranslateEventProto.

BUG=653700, 634961
==========

[commit-bot: I haz the power, 2016-10-07 21:27:22]

Description was changed from

==========
Add TranslateEventProto.

BUG=653700, 634961
==========

to

==========
Add TranslateEventProto.

BUG=653700, 634961
==========

[commit-bot: I haz the power, 2016-10-07 21:27:24]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2016-10-07 21:29:05]

Description was changed from

==========
Add TranslateEventProto.

BUG=653700, 634961
==========

to

==========
Add TranslateEventProto.

BUG=653700, 634961

Committed: https://crrev.com/01e12a12485f430701ec9c47974e71b3ecc99563
Cr-Commit-Position: refs/heads/master@{#423969}
==========

[commit-bot: I haz the power, 2016-10-07 21:29:06]

Patchset 10 (id:??) landed as
https://crrev.com/01e12a12485f430701ec9c47974e71b3ecc99563
Cr-Commit-Position: refs/heads/master@{#423969}