Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1115343002:
Added a whitelist for trace events that are known to be PII-less. (Closed)
|
Created:
5 years, 7 months ago by oystein (OOO til 10th of July) Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded a whitelist for trace events that are known to have args without PII.
Companion CL: https://codereview.chromium.org/1067233002/
R=nduca,davidben,jam
BUG=466769
Committed: https://crrev.com/678b1e5a2bce8aff7b1099c001687061fc031e84
Cr-Commit-Position: refs/heads/master@{#330822}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated OWNERS #Patch Set 3 : Merged in https://codereview.chromium.org/1126553005 #Patch Set 4 : Moved whitelist to chrome #
Total comments: 2
Patch Set 5 : Always set the predicate #
Total comments: 4
Patch Set 6 : Review fixes #
Total comments: 1
Messages
Total messages: 42 (9 generated)
ptal
lgtm https://codereview.chromium.org/1115343002/diff/1/content/common/OWNERS File content/common/OWNERS (right): https://codereview.chromium.org/1115343002/diff/1/content/common/OWNERS#newco... content/common/OWNERS:81: per-file trace_event_args_whitelist*=nduca@chromium.org lets add you here too now, i don't want to review the whitelist patches every time :)
oysteine@chromium.org changed reviewers: + davidben@chromium.org
Adding +davidben for content/ OWNER. ptal :). See the two companion CLs for context. https://codereview.chromium.org/1115343002/diff/1/content/common/OWNERS File content/common/OWNERS (right): https://codereview.chromium.org/1115343002/diff/1/content/common/OWNERS#newco... content/common/OWNERS:81: per-file trace_event_args_whitelist*=nduca@chromium.org On 2015/05/01 03:09:55, nduca wrote: > lets add you here too now, i don't want to review the whitelist patches every > time :) Done, thanks!
davidben@chromium.org changed reviewers: + jam@chromium.org
+jam, are there any particular guidelines to follow about when to or not to add per-file rules to content OWNERS files?
On 2015/05/06 14:58:47, David Benjamin wrote: > +jam, are there any particular guidelines to follow about when to or not to add > per-file rules to content OWNERS files? this change is too small to review by itself. can you combine it with https://codereview.chromium.org/1126553005? then it would be obvious that content/common isn't the right place for this method, as it's called from outside content. code outside content can only call in content/public why does this method have to be in content?
On 2015/05/07 16:47:30, jam wrote: > On 2015/05/06 14:58:47, David Benjamin wrote: > > +jam, are there any particular guidelines to follow about when to or not to > add > > per-file rules to content OWNERS files? > > this change is too small to review by itself. can you combine it with > https://codereview.chromium.org/1126553005 > Done; merged into this CL. > then it would be obvious that content/common isn't the right place for this > method, as it's called from outside content. code outside content can only call > in content/public > > why does this method have to be in content? As far as I'm concerned it can be anywhere, I was told to move to content in https://codereview.chromium.org/1067233002/ . Would it be OK as-is in content/public/common?
On 2015/05/07 17:52:31, Oystein wrote: > On 2015/05/07 16:47:30, jam wrote: > > On 2015/05/06 14:58:47, David Benjamin wrote: > > > +jam, are there any particular guidelines to follow about when to or not to > > add > > > per-file rules to content OWNERS files? > > > > this change is too small to review by itself. can you combine it with > > https://codereview.chromium.org/1126553005 > > > > Done; merged into this CL. > > > then it would be obvious that content/common isn't the right place for this > > method, as it's called from outside content. code outside content can only > call > > in content/public > > > > why does this method have to be in content? > > As far as I'm concerned it can be anywhere, I was told to move to content in > https://codereview.chromium.org/1067233002/ . > > Would it be OK as-is in content/public/common? Where are the strings in this array from?
On 2015/05/07 20:04:05, jam wrote: > On 2015/05/07 17:52:31, Oystein wrote: > > On 2015/05/07 16:47:30, jam wrote: > > > On 2015/05/06 14:58:47, David Benjamin wrote: > > > > +jam, are there any particular guidelines to follow about when to or not > to > > > add > > > > per-file rules to content OWNERS files? > > > > > > this change is too small to review by itself. can you combine it with > > > https://codereview.chromium.org/1126553005 > > > > > > > Done; merged into this CL. > > > > > then it would be obvious that content/common isn't the right place for this > > > method, as it's called from outside content. code outside content can only > > call > > > in content/public > > > > > > why does this method have to be in content? > > > > As far as I'm concerned it can be anywhere, I was told to move to content in > > https://codereview.chromium.org/1067233002/ . > > > > Would it be OK as-is in content/public/common? > > Where are the strings in this array from? They're trace categories+names; events that could be coming from anywhere in the codebase (base/blink/content/chrome/v8/skia).
On 2015/05/07 20:18:48, Oystein wrote: > On 2015/05/07 20:04:05, jam wrote: > > On 2015/05/07 17:52:31, Oystein wrote: > > > On 2015/05/07 16:47:30, jam wrote: > > > > On 2015/05/06 14:58:47, David Benjamin wrote: > > > > > +jam, are there any particular guidelines to follow about when to or not > > to > > > > add > > > > > per-file rules to content OWNERS files? > > > > > > > > this change is too small to review by itself. can you combine it with > > > > https://codereview.chromium.org/1126553005 > > > > > > > > > > Done; merged into this CL. > > > > > > > then it would be obvious that content/common isn't the right place for > this > > > > method, as it's called from outside content. code outside content can only > > > call > > > > in content/public > > > > > > > > why does this method have to be in content? > > > > > > As far as I'm concerned it can be anywhere, I was told to move to content in > > > https://codereview.chromium.org/1067233002/ . > > > > > > Would it be OK as-is in content/public/common? > > > > Where are the strings in this array from? > > They're trace categories+names; events that could be coming from anywhere in the > codebase (base/blink/content/chrome/v8/skia). Are the categories and names part of //content or below, or do they depend on what events //chrome emits? If the latter, I don't think this should be in content. chrome's names at least should live in //chrome.
On 2015/05/07 20:25:22, David Benjamin wrote: > On 2015/05/07 20:18:48, Oystein wrote: > > On 2015/05/07 20:04:05, jam wrote: > > > On 2015/05/07 17:52:31, Oystein wrote: > > > > On 2015/05/07 16:47:30, jam wrote: > > > > > On 2015/05/06 14:58:47, David Benjamin wrote: > > > > > > +jam, are there any particular guidelines to follow about when to or > not > > > to > > > > > add > > > > > > per-file rules to content OWNERS files? > > > > > > > > > > this change is too small to review by itself. can you combine it with > > > > > https://codereview.chromium.org/1126553005 > > > > > > > > > > > > > Done; merged into this CL. > > > > > > > > > then it would be obvious that content/common isn't the right place for > > this > > > > > method, as it's called from outside content. code outside content can > only > > > > call > > > > > in content/public > > > > > > > > > > why does this method have to be in content? > > > > > > > > As far as I'm concerned it can be anywhere, I was told to move to content > in > > > > https://codereview.chromium.org/1067233002/ . > > > > > > > > Would it be OK as-is in content/public/common? > > > > > > Where are the strings in this array from? > > > > They're trace categories+names; events that could be coming from anywhere in > the > > codebase (base/blink/content/chrome/v8/skia). > > Are the categories and names part of //content or below, or do they depend on > what events //chrome emits? If the latter, I don't think this should be in > content. chrome's names at least should live in //chrome. They're arbitrary strings. There's generic categories like "audio" and "benchmark" which are used in multiple locations ("audio" in //media and //content, "benchmark" likely in every major subsystem in the end). Some will be more specific to one system, like "cc", "blink", "IndexedDB", etc. The whitelist can live wherever makes the most sense (I originally had it in /base/trace_event/) and can be used on Android, but I don't think it makes sense to split it up.
On 2015/05/07 20:39:39, Oystein wrote: > On 2015/05/07 20:25:22, David Benjamin wrote: > > On 2015/05/07 20:18:48, Oystein wrote: > > > On 2015/05/07 20:04:05, jam wrote: > > > > On 2015/05/07 17:52:31, Oystein wrote: > > > > > On 2015/05/07 16:47:30, jam wrote: > > > > > > On 2015/05/06 14:58:47, David Benjamin wrote: > > > > > > > +jam, are there any particular guidelines to follow about when to or > > not > > > > to > > > > > > add > > > > > > > per-file rules to content OWNERS files? > > > > > > > > > > > > this change is too small to review by itself. can you combine it with > > > > > > https://codereview.chromium.org/1126553005 > > > > > > > > > > > > > > > > Done; merged into this CL. > > > > > > > > > > > then it would be obvious that content/common isn't the right place for > > > this > > > > > > method, as it's called from outside content. code outside content can > > only > > > > > call > > > > > > in content/public > > > > > > > > > > > > why does this method have to be in content? > > > > > > > > > > As far as I'm concerned it can be anywhere, I was told to move to > content > > in > > > > > https://codereview.chromium.org/1067233002/ . > > > > > > > > > > Would it be OK as-is in content/public/common? > > > > > > > > Where are the strings in this array from? > > > > > > They're trace categories+names; events that could be coming from anywhere in > > the > > > codebase (base/blink/content/chrome/v8/skia). > > > > Are the categories and names part of //content or below, or do they depend on > > what events //chrome emits? If the latter, I don't think this should be in > > content. chrome's names at least should live in //chrome. > > They're arbitrary strings. There's generic categories like "audio" and > "benchmark" which are used in multiple locations ("audio" in //media and > //content, "benchmark" likely in every major subsystem in the end). Some will be > more specific to one system, like "cc", "blink", "IndexedDB", etc. The whitelist > can live wherever makes the most sense (I originally had it in > /base/trace_event/) and can be used on Android, but I don't think it makes sense > to split it up. I believe Android does use //chrome, by the way. Not all of it, but parts of it. So I don't think you need to put it in //content just for Android's sake. //chrome seems a fine place for something which depends on the entire stack.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
On 2015/05/07 20:45:35, David Benjamin wrote: > On 2015/05/07 20:39:39, Oystein wrote: > > On 2015/05/07 20:25:22, David Benjamin wrote: > > > On 2015/05/07 20:18:48, Oystein wrote: > > > > On 2015/05/07 20:04:05, jam wrote: > > > > > On 2015/05/07 17:52:31, Oystein wrote: > > > > > > On 2015/05/07 16:47:30, jam wrote: > > > > > > > On 2015/05/06 14:58:47, David Benjamin wrote: > > > > > > > > +jam, are there any particular guidelines to follow about when to > or > > > not > > > > > to > > > > > > > add > > > > > > > > per-file rules to content OWNERS files? > > > > > > > > > > > > > > this change is too small to review by itself. can you combine it > with > > > > > > > https://codereview.chromium.org/1126553005 > > > > > > > > > > > > > > > > > > > Done; merged into this CL. > > > > > > > > > > > > > then it would be obvious that content/common isn't the right place > for > > > > this > > > > > > > method, as it's called from outside content. code outside content > can > > > only > > > > > > call > > > > > > > in content/public > > > > > > > > > > > > > > why does this method have to be in content? > > > > > > > > > > > > As far as I'm concerned it can be anywhere, I was told to move to > > content > > > in > > > > > > https://codereview.chromium.org/1067233002/ . > > > > > > > > > > > > Would it be OK as-is in content/public/common? > > > > > > > > > > Where are the strings in this array from? > > > > > > > > They're trace categories+names; events that could be coming from anywhere > in > > > the > > > > codebase (base/blink/content/chrome/v8/skia). > > > > > > Are the categories and names part of //content or below, or do they depend > on > > > what events //chrome emits? If the latter, I don't think this should be in > > > content. chrome's names at least should live in //chrome. > > > > They're arbitrary strings. There's generic categories like "audio" and > > "benchmark" which are used in multiple locations ("audio" in //media and > > //content, "benchmark" likely in every major subsystem in the end). Some will > be > > more specific to one system, like "cc", "blink", "IndexedDB", etc. The > whitelist > > can live wherever makes the most sense (I originally had it in > > /base/trace_event/) and can be used on Android, but I don't think it makes > sense > > to split it up. > > I believe Android does use //chrome, by the way. Not all of it, but parts of it. > So I don't think you need to put it in //content just for Android's sake. > //chrome seems a fine place for something which depends on the entire stack. Done; ptal! The amount of plumbing this is starting to take is making me a sad panda though :/. Compare to the version where the list was in base/trace_event: https://codereview.chromium.org/1067233002/#ps80001 (original merged CL).
https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:102: GetContentClient()->GetTraceEventFilterPredicate()); why do you need to change content? seems like chrome can call TraceLog directly?
https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:102: GetContentClient()->GetTraceEventFilterPredicate()); On 2015/05/12 23:16:32, jam wrote: > why do you need to change content? seems like chrome can call TraceLog > directly? Tracing is primarily a content/ concept; there's no initialization of tracing done in chrome/, so I'd have to add some new hook points there. The predicate also needs to be accessible from content/public/ for the sake of components/tracing/ anyway, so I'm not sure if setting this from chrome/ gains us anything?
On 2015/05/12 23:25:12, Oystein wrote: > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > File content/browser/tracing/tracing_controller_impl.cc (right): > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > content/browser/tracing/tracing_controller_impl.cc:102: > GetContentClient()->GetTraceEventFilterPredicate()); > On 2015/05/12 23:16:32, jam wrote: > > why do you need to change content? seems like chrome can call TraceLog > > directly? > > Tracing is primarily a content/ concept; there's no initialization of tracing > done in chrome/, so I'd have to add some new hook points there. The predicate > also needs to be accessible from content/public/ for the sake of > components/tracing/ anyway, so I'm not sure if setting this from chrome/ gains > us anything? tracing is a concept that's codebase-wide, that's why you have all layers using it. content is initializing it because one place has to which knows about multi-process code, so that's why it's there (otherwise tracing wouldn't work for any content based browser). content isn't concerned about PII data since it doesn't send anything related to browser usage up; that's the responsibility of the embedder. I don't know what you mean by the predicate, it looks like there are some missing files here in this patch. (this is why it's always good to have green try runs before sending for review)
On 2015/05/12 23:32:32, jam wrote: > On 2015/05/12 23:25:12, Oystein wrote: > > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > > File content/browser/tracing/tracing_controller_impl.cc (right): > > > > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > > content/browser/tracing/tracing_controller_impl.cc:102: > > GetContentClient()->GetTraceEventFilterPredicate()); > > On 2015/05/12 23:16:32, jam wrote: > > > why do you need to change content? seems like chrome can call TraceLog > > > directly? > > > > Tracing is primarily a content/ concept; there's no initialization of tracing > > done in chrome/, so I'd have to add some new hook points there. The predicate > > also needs to be accessible from content/public/ for the sake of > > components/tracing/ anyway, so I'm not sure if setting this from chrome/ gains > > us anything? > > tracing is a concept that's codebase-wide, that's why you have all layers using > it. content is initializing it because one place has to which knows about > multi-process code, so that's why it's there (otherwise tracing wouldn't work > for any content based browser). > Sure, I meant more that there's almost no actual tracing code in chrome/, other than an uploader class. > content isn't concerned about PII data since it doesn't send anything related to > browser usage up; that's the responsibility of the embedder. I don't know what > you mean by the predicate, it looks like there are some missing files here in > this patch. (this is why it's always good to have green try runs before sending > for review) I was asked to split up the patch; the base/trace_event part is in the CL linked in the description (https://codereview.chromium.org/1067233002/). Essentially: Where we enable tracing in the different processes (content/browser/tracing/trace_controller_impl.cc for the browser and components/tracing/child_trace_message_filter.cc for the child processes), we need to provide a filtering-delegate/predicate to TraceLog.
On 2015/05/12 23:58:48, Oystein wrote: > On 2015/05/12 23:32:32, jam wrote: > > On 2015/05/12 23:25:12, Oystein wrote: > > > > > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > > > File content/browser/tracing/tracing_controller_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > > > content/browser/tracing/tracing_controller_impl.cc:102: > > > GetContentClient()->GetTraceEventFilterPredicate()); > > > On 2015/05/12 23:16:32, jam wrote: > > > > why do you need to change content? seems like chrome can call TraceLog > > > > directly? > > > > > > Tracing is primarily a content/ concept; there's no initialization of > tracing > > > done in chrome/, so I'd have to add some new hook points there. The > predicate > > > also needs to be accessible from content/public/ for the sake of > > > components/tracing/ anyway, so I'm not sure if setting this from chrome/ > gains > > > us anything? > > > > tracing is a concept that's codebase-wide, that's why you have all layers > using > > it. content is initializing it because one place has to which knows about > > multi-process code, so that's why it's there (otherwise tracing wouldn't work > > for any content based browser). > > > > Sure, I meant more that there's almost no actual tracing code in chrome/, other > than an uploader class. could we maybe apply this filtering at the upload time? i.e. whatever code in chrome asks tracing for all the logs then has a way to run a filter on the result? > > > content isn't concerned about PII data since it doesn't send anything related > to > > browser usage up; that's the responsibility of the embedder. I don't know what > > you mean by the predicate, it looks like there are some missing files here in > > this patch. (this is why it's always good to have green try runs before > sending > > for review) > > I was asked to split up the patch; the base/trace_event part is in the CL linked > in the description (https://codereview.chromium.org/1067233002/). > > Essentially: Where we enable tracing in the different processes > (content/browser/tracing/trace_controller_impl.cc for the browser and > components/tracing/child_trace_message_filter.cc for the child processes), we > need to provide a filtering-delegate/predicate to TraceLog.
That was suggested before as well and it's definitely possible, I'm just really skeptical of adding more parsing/processing of the (potentially) giant JSON buffers in the browser process :/. Uncompressed trace data can be 100 megs. The background for these CLs is gathering end-user traces, and stability/performance is one of the main concerns I have. On Wed, May 13, 2015 at 10:02 AM <jabdelmalek@google.com> wrote: > On 2015/05/12 23:58:48, Oystein wrote: > > On 2015/05/12 23:32:32, jam wrote: > > > On 2015/05/12 23:25:12, Oystein wrote: > > > > > > > > > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > > > > File content/browser/tracing/tracing_controller_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1115343002/diff/100001/content/browser/tracin... > > > > content/browser/tracing/tracing_controller_impl.cc:102: > > > > GetContentClient()->GetTraceEventFilterPredicate()); > > > > On 2015/05/12 23:16:32, jam wrote: > > > > > why do you need to change content? seems like chrome can call > > TraceLog > > > > > directly? > > > > > > > > Tracing is primarily a content/ concept; there's no initialization of > > tracing > > > > done in chrome/, so I'd have to add some new hook points there. The > > predicate > > > > also needs to be accessible from content/public/ for the sake of > > > > components/tracing/ anyway, so I'm not sure if setting this from > > chrome/ > > gains > > > > us anything? > > > > > > tracing is a concept that's codebase-wide, that's why you have all > > layers > > using > > > it. content is initializing it because one place has to which knows > > about > > > multi-process code, so that's why it's there (otherwise tracing > wouldn't > work > > > for any content based browser). > > > > > > Sure, I meant more that there's almost no actual tracing code in chrome/, > other > > than an uploader class. > > could we maybe apply this filtering at the upload time? i.e. whatever code > in > chrome asks tracing for all the logs then has a way to run a filter on the > result? > > > > > content isn't concerned about PII data since it doesn't send anything > related > > to > > > browser usage up; that's the responsibility of the embedder. I don't > > know > what > > > you mean by the predicate, it looks like there are some missing files > > here > in > > > this patch. (this is why it's always good to have green try runs before > > sending > > > for review) > > > I was asked to split up the patch; the base/trace_event part is in the CL > linked > > in the description (https://codereview.chromium.org/1067233002/). > > > Essentially: Where we enable tracing in the different processes > > (content/browser/tracing/trace_controller_impl.cc for the browser and > > components/tracing/child_trace_message_filter.cc for the child > > processes), we > > need to provide a filtering-delegate/predicate to TraceLog. > > > > https://codereview.chromium.org/1115343002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/13 00:18:07, chromium-reviews wrote: > That was suggested before as well and it's definitely possible, I'm just > really skeptical of adding more parsing/processing of the (potentially) > giant JSON buffers in the browser process :/. Uncompressed trace data can > be 100 megs. The background for these CLs is gathering end-user traces, and > stability/performance is one of the main concerns I have. Can you provide more background as I'm unfamiliar with how this will work? I looked at the linked bugs but they were bare. Is the assumption that most of the tracing data will be thrown out in memory, and we'll only keep a small subset which is this non-PII part? How does the Chrome layer get this data to send; is it continuously asking for filtered data, or is it a one-shot where it might ask for the filtered data since startup?
On 2015/05/13 02:05:25, jam wrote: > On 2015/05/13 00:18:07, chromium-reviews wrote: > > That was suggested before as well and it's definitely possible, I'm just > > really skeptical of adding more parsing/processing of the (potentially) > > giant JSON buffers in the browser process :/. Uncompressed trace data can > > be 100 megs. The background for these CLs is gathering end-user traces, and > > stability/performance is one of the main concerns I have. > > Can you provide more background as I'm unfamiliar with how this will work? I > looked at the linked bugs but they were bare. Sure thing, https://go/slow-reports-benchmark is a good place to start (and other docs at https://go/slow-reports) (internal only). There's an external doc with some clientside architecture at https://docs.google.com/a/chromium.org/document/d/1qZmXmodxOKmsTRO27z2WlH2h9K... > Is the assumption that most of the tracing data will be thrown out in memory, > and we'll only keep a small subset which is this non-PII part? How does the > Chrome layer get this data to send; is it continuously asking for filtered data, > or is it a one-shot where it might ask for the filtered data since startup? The PII stripping will mainly be a one-shot affair; the tl;dr idea is that we continually run tracing in a ring-buffer, and when a specified "slow" condition occurs (like a specific UMA histogram bucket being incremented, for example), we grab the traces and upload. In practice most of the tracing data should be kept, as we'll limit the active categories to a specific subset which we've also gone through and vetted for PII. The whitelist is essentially a safety valve against developers accidentally adding args/events which leak PII, by adding an extra step of tracing review.
On 2015/05/13 02:25:22, Oystein wrote: > On 2015/05/13 02:05:25, jam wrote: > > On 2015/05/13 00:18:07, chromium-reviews wrote: > > > That was suggested before as well and it's definitely possible, I'm just > > > really skeptical of adding more parsing/processing of the (potentially) > > > giant JSON buffers in the browser process :/. Uncompressed trace data can > > > be 100 megs. The background for these CLs is gathering end-user traces, and > > > stability/performance is one of the main concerns I have. > > > > Can you provide more background as I'm unfamiliar with how this will work? I > > looked at the linked bugs but they were bare. > > Sure thing, https://go/slow-reports-benchmark is a good place to start (and > other docs at https://go/slow-reports) (internal only). > > There's an external doc with some clientside architecture at > https://docs.google.com/a/chromium.org/document/d/1qZmXmodxOKmsTRO27z2WlH2h9K... > > > Is the assumption that most of the tracing data will be thrown out in memory, > > and we'll only keep a small subset which is this non-PII part? How does the > > Chrome layer get this data to send; is it continuously asking for filtered > data, > > or is it a one-shot where it might ask for the filtered data since startup? > > The PII stripping will mainly be a one-shot affair; the tl;dr idea is that we > continually run tracing in a ring-buffer, and when a specified "slow" condition > occurs (like a specific UMA histogram bucket being incremented, for example), we > grab the traces and upload. > > In practice most of the tracing data should be kept, as we'll limit the active > categories to a specific subset which we've also gone through and vetted for > PII. The whitelist is essentially a safety valve against developers accidentally > adding args/events which leak PII, by adding an extra step of tracing review. If I'm understanding this last paragraph correctly, doesn't this mean that the filtering should remove a minimal subset of the data?
On 2015/05/13 16:50:25, jam wrote: > On 2015/05/13 02:25:22, Oystein wrote: > > On 2015/05/13 02:05:25, jam wrote: > > > On 2015/05/13 00:18:07, chromium-reviews wrote: > > > > That was suggested before as well and it's definitely possible, I'm just > > > > really skeptical of adding more parsing/processing of the (potentially) > > > > giant JSON buffers in the browser process :/. Uncompressed trace data can > > > > be 100 megs. The background for these CLs is gathering end-user traces, > and > > > > stability/performance is one of the main concerns I have. > > > > > > Can you provide more background as I'm unfamiliar with how this will work? I > > > looked at the linked bugs but they were bare. > > > > Sure thing, https://go/slow-reports-benchmark is a good place to start (and > > other docs at https://go/slow-reports) (internal only). > > > > There's an external doc with some clientside architecture at > > > https://docs.google.com/a/chromium.org/document/d/1qZmXmodxOKmsTRO27z2WlH2h9K... > > > > > Is the assumption that most of the tracing data will be thrown out in > memory, > > > and we'll only keep a small subset which is this non-PII part? How does the > > > Chrome layer get this data to send; is it continuously asking for filtered > > data, > > > or is it a one-shot where it might ask for the filtered data since startup? > > > > The PII stripping will mainly be a one-shot affair; the tl;dr idea is that we > > continually run tracing in a ring-buffer, and when a specified "slow" > condition > > occurs (like a specific UMA histogram bucket being incremented, for example), > we > > grab the traces and upload. > > > > In practice most of the tracing data should be kept, as we'll limit the active > > categories to a specific subset which we've also gone through and vetted for > > PII. The whitelist is essentially a safety valve against developers > accidentally > > adding args/events which leak PII, by adding an extra step of tracing review. > > If I'm understanding this last paragraph correctly, doesn't this mean that the > filtering should remove a minimal subset of the data? Ideally, yes.
jam/davidben: How do we progress with this?
On 2015/05/13 21:21:33, Oystein wrote: > On 2015/05/13 16:50:25, jam wrote: > > On 2015/05/13 02:25:22, Oystein wrote: > > > On 2015/05/13 02:05:25, jam wrote: > > > > On 2015/05/13 00:18:07, chromium-reviews wrote: > > > > > That was suggested before as well and it's definitely possible, I'm just > > > > > really skeptical of adding more parsing/processing of the (potentially) > > > > > giant JSON buffers in the browser process :/. Uncompressed trace data > can > > > > > be 100 megs. The background for these CLs is gathering end-user traces, > > and > > > > > stability/performance is one of the main concerns I have. > > > > > > > > Can you provide more background as I'm unfamiliar with how this will work? > I > > > > looked at the linked bugs but they were bare. > > > > > > Sure thing, https://go/slow-reports-benchmark is a good place to start (and > > > other docs at https://go/slow-reports) (internal only). > > > > > > There's an external doc with some clientside architecture at > > > > > > https://docs.google.com/a/chromium.org/document/d/1qZmXmodxOKmsTRO27z2WlH2h9K... > > > > > > > Is the assumption that most of the tracing data will be thrown out in > > memory, > > > > and we'll only keep a small subset which is this non-PII part? How does > the > > > > Chrome layer get this data to send; is it continuously asking for filtered > > > data, > > > > or is it a one-shot where it might ask for the filtered data since > startup? > > > > > > The PII stripping will mainly be a one-shot affair; the tl;dr idea is that > we > > > continually run tracing in a ring-buffer, and when a specified "slow" > > condition > > > occurs (like a specific UMA histogram bucket being incremented, for > example), > > we > > > grab the traces and upload. > > > > > > In practice most of the tracing data should be kept, as we'll limit the > active > > > categories to a specific subset which we've also gone through and vetted for > > > PII. The whitelist is essentially a safety valve against developers > > accidentally > > > adding args/events which leak PII, by adding an extra step of tracing > review. > > > > If I'm understanding this last paragraph correctly, doesn't this mean that the > > filtering should remove a minimal subset of the data? > > Ideally, yes. Ok this seems different than what comment 22 says? I read that as saying that filtering will remove a lot of data, so we should do it in different processes instead of browser. now this says that only a small amount of data will be filtered out, which implies that chrome can do this in the browser process before sending data over the network. On 2015/05/15 21:04:24, Oystein wrote: > jam/davidben: How do we progress with this? apologies, I just saw this. Perhaps we can chat in person? (btw if i take more than a few hours to respond, I missed your message, so please IM me)
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
New version up after chatting offline with jam. We now always set the filtering predicate directly from chrome/ on startup, and the flag in TraceOptions controls whether it's used or not. This means far less plumbing, and no changes in components/tracing or content/public. This also means https://codereview.chromium.org/1067233002/ had to be merged into this CL again.
lgtm, thanks for your patience in figuring out a minimal way of doing this https://codereview.chromium.org/1115343002/diff/160001/chrome/common/trace_ev... File chrome/common/trace_event_args_whitelist.h (right): https://codereview.chromium.org/1115343002/diff/160001/chrome/common/trace_ev... chrome/common/trace_event_args_whitelist.h:8: namespace chrome { nit: using "chrome" namespace is deprecated. as chrome is a leaf node, its code doesn't need to be in a namespace (see cr-dev threads on this) https://codereview.chromium.org/1115343002/diff/160001/chrome/common/trace_ev... chrome/common/trace_event_args_whitelist.h:10: bool IsTraceEventArgsWhitelisted(const char* category_group_name, nit: add brief comment about what this method is used for
Great, thanks for the help jam@! https://codereview.chromium.org/1115343002/diff/160001/chrome/common/trace_ev... File chrome/common/trace_event_args_whitelist.h (right): https://codereview.chromium.org/1115343002/diff/160001/chrome/common/trace_ev... chrome/common/trace_event_args_whitelist.h:8: namespace chrome { On 2015/05/20 15:30:18, jam wrote: > nit: using "chrome" namespace is deprecated. as chrome is a leaf node, its code > doesn't need to be in a namespace (see cr-dev threads on this) Done. https://codereview.chromium.org/1115343002/diff/160001/chrome/common/trace_ev... chrome/common/trace_event_args_whitelist.h:10: bool IsTraceEventArgsWhitelisted(const char* category_group_name, On 2015/05/20 15:30:18, jam wrote: > nit: add brief comment about what this method is used for Done.
The CQ bit was checked by oysteine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1115343002/#ps180001 (title: "Review fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1115343002/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/678b1e5a2bce8aff7b1099c001687061fc031e84 Cr-Commit-Position: refs/heads/master@{#330822}
Message was sent while issue was closed.
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1115343002/diff/180001/chrome/common/trace_ev... File chrome/common/trace_event_args_whitelist.cc (right): https://codereview.chromium.org/1115343002/diff/180001/chrome/common/trace_ev... chrome/common/trace_event_args_whitelist.cc:12: const char* const kEventArgsWhitelist[][2] = {{"toplevel", "*"}, Do these get matched as exact matches or just contains matches? We have events like "toplevel.flow" will those pass the filter or be rejected?
Message was sent while issue was closed.
On 2015/06/01 14:56:09, dsinclair wrote: > https://codereview.chromium.org/1115343002/diff/180001/chrome/common/trace_ev... > File chrome/common/trace_event_args_whitelist.cc (right): > > https://codereview.chromium.org/1115343002/diff/180001/chrome/common/trace_ev... > chrome/common/trace_event_args_whitelist.cc:12: const char* const > kEventArgsWhitelist[][2] = {{"toplevel", "*"}, > Do these get matched as exact matches or just contains matches? We have events > like "toplevel.flow" will those pass the filter or be rejected? It's using MatchPattern(), so unless there's ? or * wildcards, it'll be exact match only.
Message was sent while issue was closed.
On 2015/06/01 17:37:17, Oystein wrote: > On 2015/06/01 14:56:09, dsinclair wrote: > > > https://codereview.chromium.org/1115343002/diff/180001/chrome/common/trace_ev... > > File chrome/common/trace_event_args_whitelist.cc (right): > > > > > https://codereview.chromium.org/1115343002/diff/180001/chrome/common/trace_ev... > > chrome/common/trace_event_args_whitelist.cc:12: const char* const > > kEventArgsWhitelist[][2] = {{"toplevel", "*"}, > > Do these get matched as exact matches or just contains matches? We have events > > like "toplevel.flow" will those pass the filter or be rejected? > > It's using MatchPattern(), so unless there's ? or * wildcards, it'll be exact > match only. (The whitelist hasn't been populated in any meaningful way yet, by the way). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
