Allow tracing categories to default to disabled.

This CL allows the prefixing of category names with disabled- to have them
not traced by default. Only if an explicit disabled-foo is provided in the
category filter do we enable the category.

Providing a disabled-foo in the category filter does not act like an include
category in that all other includes have to be specified. The disables are
treated as their own group.

BUG=232109

Review URL: https://chromiumcodereview.appspot.com/14748003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@198787 0039d316-1c4b-4281-b951-d872f2087c98

base/debug/trace_event.h

@@ -200,6 +200,10 @@
 #define TRACE_STR_COPY(str) \
     trace_event_internal::TraceStringWithCopy(str)
 
+// This will mark the trace event as disabled by default. The user will need
+// to explicitly enable the event.
+#define TRACE_DISABLED_BY_DEFAULT(name) "disabled-by-default-" name
+
 // By default, uint64 ID argument values are not mangled with the Process ID in
 // TRACE_EVENT_ASYNC macros. Use this macro to force Process ID mangling.
 #define TRACE_ID_MANGLE(id) \

base/debug/trace_event_impl.cc

@@ -903,15 +903,7 @@ void TraceLog::SetEnabled(const CategoryFilter& category_filter,
                   << "set of options.";
     }
 
-    // Tracing is already enabled, so just merge in enabled categories.
-    // We only expand the set of enabled categories upon nested SetEnable().
-    if (category_filter_.HasIncludedPatterns() &&
-        category_filter.HasIncludedPatterns()) {
-      category_filter_.Merge(category_filter);
-    } else {
-      // If either old or new included categories are empty, allow all events.
-      category_filter_.Clear();
-    }
+    category_filter_.Merge(category_filter);
     EnableIncludedCategoryGroups();
     return;
   }
@@ -1307,8 +1299,9 @@ bool CategoryFilter::IsEmptyOrContainsLeadingOrTrailingWhitespace(
           str.at(str.length() - 1) == ' ';
 }
 
-static bool DoesCategoryGroupContainCategory(const char* category_group,
-                                             const char* category) {
+bool CategoryFilter::DoesCategoryGroupContainCategory(
+    const char* category_group,
+    const char* category) const {
   DCHECK(category);
   CStringTokenizer category_group_tokens(category_group,
                           category_group + strlen(category_group), ",");
@@ -1336,6 +1329,7 @@ CategoryFilter::CategoryFilter(const std::string& filter_string) {
 
 CategoryFilter::CategoryFilter(const CategoryFilter& cf)
     : included_(cf.included_),
+      disabled_(cf.disabled_),
       excluded_(cf.excluded_) {
 }
 
@@ -1347,6 +1341,7 @@ CategoryFilter& CategoryFilter::operator=(const CategoryFilter& rhs) {
     return *this;
 
   included_ = rhs.included_;
+  disabled_ = rhs.disabled_;
   excluded_ = rhs.excluded_;
   return *this;
 }
@@ -1365,29 +1360,23 @@ void CategoryFilter::Initialize(const std::string& filter_string) {
       // Remove '-' from category string.
       category = category.substr(1);
       excluded_.push_back(category);
+    } else if (category.compare(0, strlen(TRACE_DISABLED_BY_DEFAULT("")),
+                                TRACE_DISABLED_BY_DEFAULT("")) == 0) {
+      disabled_.push_back(category);
     } else {
       included_.push_back(category);
     }
   }
 }
 
-void CategoryFilter::WriteString(std::string* out,
+void CategoryFilter::WriteString(const StringList& values,
+                                 std::string* out,
                                  bool included) const {
-  std::vector<std::string>::const_iterator ci;
-  std::vector<std::string>::const_iterator end;
-  if (included) {
-    ci = included_.begin();
-    end = included_.end();
-  } else {
-    ci = excluded_.begin();
-    end = excluded_.end();
-  }
-
-  // Prepend commas for all excluded categories IF we have included categories.
-  bool prepend_comma_for_first_excluded = !included && !included_.empty();
+  bool prepend_comma = !out->empty();
   int token_cnt = 0;
-  for (; ci != end; ++ci) {
-    if (token_cnt > 0 || prepend_comma_for_first_excluded)
+  for (StringList::const_iterator ci = values.begin();
+       ci != values.end(); ++ci) {
+    if (token_cnt > 0 || prepend_comma)
       StringAppendF(out, ",");
     StringAppendF(out, "%s%s", (included ? "" : "-"), ci->c_str());
     ++token_cnt;
@@ -1396,9 +1385,9 @@ void CategoryFilter::WriteString(std::string* out,
 
 std::string CategoryFilter::ToString() const {
   std::string filter_string;
-  WriteString(&filter_string, true);
-  WriteString(&filter_string, false);
-
+  WriteString(included_, &filter_string, true);
+  WriteString(disabled_, &filter_string, true);
+  WriteString(excluded_, &filter_string, false);
   return filter_string;
 }
 
@@ -1406,13 +1395,24 @@ bool CategoryFilter::IsCategoryGroupEnabled(
     const char* category_group_name) const {
   // TraceLog should call this method only as  part of enabling/disabling
   // categories.
-  std::vector<std::string>::const_iterator ci = included_.begin();
-  for (; ci != included_.end(); ++ci) {
+  StringList::const_iterator ci;
+
+  // Check the disabled- filters and the disabled-* wildcard first so that a
+  // "*" filter does not include the disabled.
+  for (ci = disabled_.begin(); ci != disabled_.end(); ++ci) {
+    if (DoesCategoryGroupContainCategory(category_group_name, ci->c_str()))
+      return true;
+  }
+  if (DoesCategoryGroupContainCategory(category_group_name,
+                                       TRACE_DISABLED_BY_DEFAULT("*")))
+    return false;
+
+  for (ci = included_.begin(); ci != included_.end(); ++ci) {
     if (DoesCategoryGroupContainCategory(category_group_name, ci->c_str()))
       return true;
   }
-  ci = excluded_.begin();
-  for (; ci != excluded_.end(); ++ci) {
+
+  for (ci = excluded_.begin(); ci != excluded_.end(); ++ci) {
     if (DoesCategoryGroupContainCategory(category_group_name, ci->c_str()))
       return false;
   }
@@ -1421,21 +1421,33 @@ bool CategoryFilter::IsCategoryGroupEnabled(
   return included_.empty();
 }
 
+bool CategoryFilter::HasIncludedPatterns() const {
+  return !included_.empty();
+}
+
 void CategoryFilter::Merge(const CategoryFilter& nested_filter) {
-  included_.insert(included_.end(),
-                   nested_filter.included_.begin(),
-                   nested_filter.included_.end());
+  // Keep included patterns only if both filters have an included entry.
+  // Otherwise, one of the filter was specifying "*" and we want to honour the
+  // broadest filter.
+  if (HasIncludedPatterns() && nested_filter.HasIncludedPatterns()) {
+    included_.insert(included_.end(),
+                     nested_filter.included_.begin(),
+                     nested_filter.included_.end());
+  } else {
+    included_.clear();
+  }
+
+  disabled_.insert(disabled_.end(),
+                   nested_filter.disabled_.begin(),
+                   nested_filter.disabled_.end());
   excluded_.insert(excluded_.end(),
                    nested_filter.excluded_.begin(),
                    nested_filter.excluded_.end());
 }
 
-bool CategoryFilter::HasIncludedPatterns() const {
-  return !included_.empty();
-}
-
 void CategoryFilter::Clear() {
   included_.clear();
+  disabled_.clear();
   excluded_.clear();
 }
 

base/debug/trace_event_impl.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/gtest_prod_util.h"
 #include "base/hash_tables.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_vector.h"
@@ -233,9 +234,6 @@ class BASE_EXPORT CategoryFilter {
   // Merges nested_filter with the current CategoryFilter
   void Merge(const CategoryFilter& nested_filter);
 
-  // Determines whether or not we have explicitly allowed category patterns.
-  bool HasIncludedPatterns() const;
-
   // Clears both included/excluded pattern lists. This would be equivalent to
   // creating a CategoryFilter with an empty string, through the constructor.
   // i.e: CategoryFilter("").
@@ -244,15 +242,26 @@ class BASE_EXPORT CategoryFilter {
   // are not excluding anything.
   void Clear();
 
+ private:
+  FRIEND_TEST_ALL_PREFIXES(TraceEventTestFixture, CategoryFilter);
+
   static bool IsEmptyOrContainsLeadingOrTrailingWhitespace(
       const std::string& str);
 
- private:
+  typedef std::vector<std::string> StringList;
+
   void Initialize(const std::string& filter_string);
-  void WriteString(std::string* out, bool included) const;
+  void WriteString(const StringList& values,
+                   std::string* out,
+                   bool included) const;
+  bool HasIncludedPatterns() const;
+
+  bool DoesCategoryGroupContainCategory(const char* category_group,
+                                        const char* category) const;
 
-  std::vector<std::string> included_;
-  std::vector<std::string> excluded_;
+  StringList included_;
+  StringList disabled_;
+  StringList excluded_;
 };
 
 class TraceSamplingThread;

base/debug/trace_event_unittest.cc

@@ -75,9 +75,13 @@ class TraceEventTestFixture : public testing::Test {
   }
 
   void BeginTrace() {
+    BeginSpecificTrace("*");
+  }
+
+  void BeginSpecificTrace(const std::string& filter) {
     event_watch_notification_ = 0;
     notifications_received_ = 0;
-    TraceLog::GetInstance()->SetEnabled(CategoryFilter("*"),
+    TraceLog::GetInstance()->SetEnabled(CategoryFilter(filter),
                                         TraceLog::RECORD_UNTIL_FULL);
   }
 
@@ -915,6 +919,8 @@ TEST_F(TraceEventTestFixture, Categories) {
   // Category groups containing more than one category.
   TRACE_EVENT_INSTANT0("c5,c6", "name", TRACE_EVENT_SCOPE_THREAD);
   TRACE_EVENT_INSTANT0("c7,c8", "name", TRACE_EVENT_SCOPE_THREAD);
+  TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("c9"), "name",
+                       TRACE_EVENT_SCOPE_THREAD);
 
   EndTraceAndFlush();
   std::vector<std::string> cat_groups;
@@ -931,6 +937,9 @@ TEST_F(TraceEventTestFixture, Categories) {
                         cat_groups.end(), "c5,c6") != cat_groups.end());
   EXPECT_TRUE(std::find(cat_groups.begin(),
                         cat_groups.end(), "c7,c8") != cat_groups.end());
+  EXPECT_TRUE(std::find(cat_groups.begin(),
+                        cat_groups.end(),
+                        "disabled-by-default-c9") != cat_groups.end());
   // Make sure metadata isn't returned.
   EXPECT_TRUE(std::find(cat_groups.begin(),
                         cat_groups.end(), "__metadata") == cat_groups.end());
@@ -1384,6 +1393,38 @@ TEST_F(TraceEventTestFixture, ThreadNameChanges) {
   EXPECT_EQ(expected_name, tmp);
 }
 
+// Test that the disabled trace categories are included/excluded from the
+// trace output correctly.
+TEST_F(TraceEventTestFixture, DisabledCategories) {
+  ManualTestSetUp();
+
+  BeginTrace();
+  TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("cc"), "first",
+                       TRACE_EVENT_SCOPE_THREAD);
+  TRACE_EVENT_INSTANT0("included", "first", TRACE_EVENT_SCOPE_THREAD);
+  EndTraceAndFlush();
+  {
+    const DictionaryValue* item = NULL;
+    ListValue& trace_parsed = trace_parsed_;
+    EXPECT_NOT_FIND_("disabled-by-default-cc");
+    EXPECT_FIND_("included");
+  }
+  Clear();
+
+  BeginSpecificTrace("disabled-by-default-cc");
+  TRACE_EVENT_INSTANT0(TRACE_DISABLED_BY_DEFAULT("cc"), "second",
+                       TRACE_EVENT_SCOPE_THREAD);
+  TRACE_EVENT_INSTANT0("other_included", "second", TRACE_EVENT_SCOPE_THREAD);
+  EndTraceAndFlush();
+
+  {
+    const DictionaryValue* item = NULL;
+    ListValue& trace_parsed = trace_parsed_;
+    EXPECT_FIND_("disabled-by-default-cc");
+    EXPECT_FIND_("other_included");
+  }
+}
+
 // Test trace calls made after tracing singleton shut down.
 //
 // The singleton is destroyed by our base::AtExitManager, but there can be
@@ -1594,9 +1635,12 @@ TEST_F(TraceEventTestFixture, TraceCategoriesAfterNestedEnable) {
   trace_log->SetEnabled(CategoryFilter("foo2"), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("foo2"));
   EXPECT_FALSE(*trace_log->GetCategoryGroupEnabled("baz"));
+  // The "" becomes the default catergory set when applied.
   trace_log->SetEnabled(CategoryFilter(""), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("foo"));
   EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("baz"));
+  EXPECT_STREQ("-*Debug,-*Test",
+               trace_log->GetCurrentCategoryFilter().ToString().c_str());
   trace_log->SetDisabled();
   trace_log->SetDisabled();
   trace_log->SetDisabled();
@@ -1610,7 +1654,23 @@ TEST_F(TraceEventTestFixture, TraceCategoriesAfterNestedEnable) {
   trace_log->SetEnabled(CategoryFilter("moo"), TraceLog::RECORD_UNTIL_FULL);
   EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("baz"));
   EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("moo"));
-  EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("foo"));
+  EXPECT_FALSE(*trace_log->GetCategoryGroupEnabled("foo"));
+  EXPECT_STREQ("-foo,-bar",
+               trace_log->GetCurrentCategoryFilter().ToString().c_str());
+  trace_log->SetDisabled();
+  trace_log->SetDisabled();
+
+  // Make sure disabled categories aren't cleared if we set in the second.
+  trace_log->SetEnabled(CategoryFilter("disabled-by-default-cc,foo"),
+                        TraceLog::RECORD_UNTIL_FULL);
+  EXPECT_FALSE(*trace_log->GetCategoryGroupEnabled("bar"));
+  trace_log->SetEnabled(CategoryFilter("disabled-by-default-gpu"),
+                        TraceLog::RECORD_UNTIL_FULL);
+  EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("disabled-by-default-cc"));
+  EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("disabled-by-default-gpu"));
+  EXPECT_TRUE(*trace_log->GetCategoryGroupEnabled("bar"));
+  EXPECT_STREQ("disabled-by-default-cc,disabled-by-default-gpu",
+               trace_log->GetCurrentCategoryFilter().ToString().c_str());
   trace_log->SetDisabled();
   trace_log->SetDisabled();
 }
@@ -1790,6 +1850,8 @@ TEST_F(TraceEventTestFixture, CategoryFilter) {
   std::string category_filter_str = default_cf.ToString();
   EXPECT_STREQ("-*Debug,-*Test", category_filter_str.c_str());
   EXPECT_TRUE(default_cf.IsCategoryGroupEnabled("not-excluded-category"));
+  EXPECT_FALSE(
+      default_cf.IsCategoryGroupEnabled("disabled-by-default-category"));
   EXPECT_FALSE(default_cf.IsCategoryGroupEnabled("Category1,CategoryDebug"));
   EXPECT_FALSE(default_cf.IsCategoryGroupEnabled("CategoryDebug,Category1"));
   EXPECT_FALSE(default_cf.IsCategoryGroupEnabled("CategoryTest,Category2"));
@@ -1819,14 +1881,13 @@ TEST_F(TraceEventTestFixture, CategoryFilter) {
 
   cf.Merge(default_cf);
   category_filter_str = cf.ToString();
-  EXPECT_STREQ("included,inc_pattern*,-excluded,-exc_pattern*,-*Debug,-*Test",
+  EXPECT_STREQ("-excluded,-exc_pattern*,-*Debug,-*Test",
                 category_filter_str.c_str());
   cf.Clear();
-  EXPECT_FALSE(cf.HasIncludedPatterns());
 
   CategoryFilter reconstructed_cf(category_filter_str);
   category_filter_str = reconstructed_cf.ToString();
-  EXPECT_STREQ("included,inc_pattern*,-excluded,-exc_pattern*,-*Debug,-*Test",
+  EXPECT_STREQ("-excluded,-exc_pattern*,-*Debug,-*Test",
                category_filter_str.c_str());
 
   // One included category.
@@ -1839,6 +1900,25 @@ TEST_F(TraceEventTestFixture, CategoryFilter) {
   category_filter_str = one_exc_cf.ToString();
   EXPECT_STREQ("-only_exc_cat", category_filter_str.c_str());
 
+  // Enabling a disabled- category does not require all categories to be traced
+  // to be included.
+  CategoryFilter disabled_cat("disabled-by-default-cc,-excluded");
+  EXPECT_STREQ("disabled-by-default-cc,-excluded",
+               disabled_cat.ToString().c_str());
+  EXPECT_TRUE(disabled_cat.IsCategoryGroupEnabled("disabled-by-default-cc"));
+  EXPECT_TRUE(disabled_cat.IsCategoryGroupEnabled("some_other_group"));
+  EXPECT_FALSE(disabled_cat.IsCategoryGroupEnabled("excluded"));
+
+  // Enabled a disabled- category and also including makes all categories to
+  // be traced require including.
+  CategoryFilter disabled_inc_cat("disabled-by-default-cc,included");
+  EXPECT_STREQ("included,disabled-by-default-cc",
+               disabled_inc_cat.ToString().c_str());
+  EXPECT_TRUE(
+      disabled_inc_cat.IsCategoryGroupEnabled("disabled-by-default-cc"));
+  EXPECT_TRUE(disabled_inc_cat.IsCategoryGroupEnabled("included"));
+  EXPECT_FALSE(disabled_inc_cat.IsCategoryGroupEnabled("other_included"));
+
   // Test that IsEmptyOrContainsLeadingOrTrailingWhitespace actually catches
   // categories that are explicitly forbiden.
   // This method is called in a DCHECK to assert that we don't have these types