Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826013002:
List relevant IDs from Finch in snippets-internals (Closed)
DescriptionList relevant IDs from Finch in snippets-internals
"Relevant" IDs are ones that are associated with our features. This will
help us investigate quality issues on the server when we want to know
what configuration a user is using.
No explanation is given for the meaning of the IDs; we don't have that
for most of them in Chrome, nor I believe do we want to add it.
Note that this excludes variation IDs set from the command line with
--force-variation-ids; it's impossible to know with Chrome if those are
our IDs or someone else's (and showing them is mostly not important to
the debugging use case anyway). It also excludes variation IDs set from
chrome://flags, which I'm investigating.
BUG=713038
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2826013002
Cr-Commit-Position: refs/heads/master@{#466694}
Committed: https://chromium.googlesource.com/chromium/src/+/9a5bc285254c5d9cbf47ea862a64324804044be1
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #Patch Set 3 : Update string for clarity #Patch Set 4 : rebase #Patch Set 5 : rebase #
Messages
Total messages: 21 (12 generated)
Description was changed from ========== List relevant IDs from Finch in snippets-internals "Relevant" IDs are ones that are associated with our features. This will help us investigate quality issues on the server when we want to know what configuration a user is using. No explanation is given for the meaning of the IDs; we don't have that for most of them in Chrome, nor I believe do we want to add it. Note that this excludes variation IDs set from the command line with --force-variation-ids; it's impossible to know with Chrome if those are our IDs or someone else's (and showing them is mostly not important to the debugging use case anyway). BUG=713038 ========== to ========== List relevant IDs from Finch in snippets-internals "Relevant" IDs are ones that are associated with our features. This will help us investigate quality issues on the server when we want to know what configuration a user is using. No explanation is given for the meaning of the IDs; we don't have that for most of them in Chrome, nor I believe do we want to add it. Note that this excludes variation IDs set from the command line with --force-variation-ids; it's impossible to know with Chrome if those are our IDs or someone else's (and showing them is mostly not important to the debugging use case anyway). BUG=713038 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sfiera@chromium.org changed reviewers: + jkrcal@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2826013002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2826013002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:141: key, trial->trial_name(), trial->group_name()); Just want to double check: The fact that the group name is not empty implies that the group is already activated and we can thus simply call trial->group_name()?
https://codereview.chromium.org/2826013002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2826013002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:141: key, trial->trial_name(), trial->group_name()); On 2017/04/19 11:15:43, jkrcal wrote: > Just want to double check: The fact that the group name is not empty implies > that the group is already activated and we can thus simply call > trial->group_name()? Yes, from my reading of the code, a non-empty group name implies the group is already active. I'm not sure if the empty string is a valid group name; if it is, then we would miss experiment IDs from such a group. But I don't think we'll do that anyway.
https://codereview.chromium.org/2826013002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2826013002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:141: key, trial->trial_name(), trial->group_name()); On 2017/04/19 11:40:18, sfiera wrote: > On 2017/04/19 11:15:43, jkrcal wrote: > > Just want to double check: The fact that the group name is not empty implies > > that the group is already activated and we can thus simply call > > trial->group_name()? > > Yes, from my reading of the code, a non-empty group name implies the group is > already active. > > I'm not sure if the empty string is a valid group name; if it is, then we would > miss experiment IDs from such a group. But I don't think we'll do that anyway. Acknowledged.
Apparently this isn't picking up experiments from chrome://flags either, which surprises me, since they *appear* to be tied to features there. I'll look into this further, since I think that we do want experiments from flags here, but won't block submission on it if that's alright by you.
On 2017/04/20 11:21:05, sfiera wrote: > Apparently this isn't picking up experiments from chrome://flags either, which > surprises me, since they *appear* to be tied to features there. I'll look into > this further, since I think that we do want experiments from flags here, but > won't block submission on it if that's alright by you. That's alright for me. Can you update the string in the HTML accordingly? (e.g. "Server-provided experiment IDs")
Description was changed from ========== List relevant IDs from Finch in snippets-internals "Relevant" IDs are ones that are associated with our features. This will help us investigate quality issues on the server when we want to know what configuration a user is using. No explanation is given for the meaning of the IDs; we don't have that for most of them in Chrome, nor I believe do we want to add it. Note that this excludes variation IDs set from the command line with --force-variation-ids; it's impossible to know with Chrome if those are our IDs or someone else's (and showing them is mostly not important to the debugging use case anyway). BUG=713038 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== List relevant IDs from Finch in snippets-internals "Relevant" IDs are ones that are associated with our features. This will help us investigate quality issues on the server when we want to know what configuration a user is using. No explanation is given for the meaning of the IDs; we don't have that for most of them in Chrome, nor I believe do we want to add it. Note that this excludes variation IDs set from the command line with --force-variation-ids; it's impossible to know with Chrome if those are our IDs or someone else's (and showing them is mostly not important to the debugging use case anyway). It also excludes variation IDs set from chrome://flags, which I'm investigating. BUG=713038 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/04/20 12:06:23, jkrcal wrote: > On 2017/04/20 11:21:05, sfiera wrote: > > Apparently this isn't picking up experiments from chrome://flags either, which > > surprises me, since they *appear* to be tied to features there. I'll look into > > this further, since I think that we do want experiments from flags here, but > > won't block submission on it if that's alright by you. > > That's alright for me. > Can you update the string in the HTML accordingly? (e.g. "Server-provided > experiment IDs") That's a good string, done.
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2826013002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493051828060260,
"parent_rev": "1fccc0ce945b93910226f408b66db128e46acc9c", "commit_rev":
"9a5bc285254c5d9cbf47ea862a64324804044be1"}
Message was sent while issue was closed.
Description was changed from ========== List relevant IDs from Finch in snippets-internals "Relevant" IDs are ones that are associated with our features. This will help us investigate quality issues on the server when we want to know what configuration a user is using. No explanation is given for the meaning of the IDs; we don't have that for most of them in Chrome, nor I believe do we want to add it. Note that this excludes variation IDs set from the command line with --force-variation-ids; it's impossible to know with Chrome if those are our IDs or someone else's (and showing them is mostly not important to the debugging use case anyway). It also excludes variation IDs set from chrome://flags, which I'm investigating. BUG=713038 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== List relevant IDs from Finch in snippets-internals "Relevant" IDs are ones that are associated with our features. This will help us investigate quality issues on the server when we want to know what configuration a user is using. No explanation is given for the meaning of the IDs; we don't have that for most of them in Chrome, nor I believe do we want to add it. Note that this excludes variation IDs set from the command line with --force-variation-ids; it's impossible to know with Chrome if those are our IDs or someone else's (and showing them is mostly not important to the debugging use case anyway). It also excludes variation IDs set from chrome://flags, which I'm investigating. BUG=713038 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2826013002 Cr-Commit-Position: refs/heads/master@{#466694} Committed: https://chromium.googlesource.com/chromium/src/+/9a5bc285254c5d9cbf47ea862a64... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/9a5bc285254c5d9cbf47ea862a64... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
