Commit 0daa79d2 authored by Mike Wittman's avatar Mike Wittman Committed by Commit Bot

[Sampling profiler] Rationalize release/channel config

Updates the ThreadProfiler configuration to consider a build to be a
Chrome release if it's official and Chrome branded. The existing code
only considered the Chrome branded state which is overly lenient. For
what its worth breakpad uses the same conditions for whether it should
be enabled.

This changes behavior of edge cases as follows:

Unofficial Chrome branded builds are now considered to be
development/CQ builds and as such the profiler is always enabled
for them.

Official Chrome branded builds with unknown channel are no longer
treated as development/CQ builds, so the profiler is disabled for
them rather than always enabled.

Bug: 1129939
Change-Id: I52b2120ef98734278ffd211809463c2ded3d3bd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2432386
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: default avatarEtienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812417}
parent 50b04c1c
......@@ -33,6 +33,17 @@ bool IsBrowserTestModeEnabled() {
switches::kStartStackProfilerBrowserTest;
}
// Returns the channel if this is a Chrome release, otherwise returns nullopt. A
// build is considered to be a Chrome release if it's official and has Chrome
// branding.
base::Optional<version_info::Channel> GetReleaseChannel() {
#if defined(OFFICIAL_BUILD) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
return chrome::GetChannel();
#else
return base::nullopt;
#endif
}
} // namespace
ThreadProfilerConfiguration::ThreadProfilerConfiguration()
......@@ -84,8 +95,7 @@ bool ThreadProfilerConfiguration::GetSyntheticFieldTrial(
std::string* group_name) const {
DCHECK_EQ(metrics::CallStackProfileParams::BROWSER_PROCESS, current_process_);
if (!platform_configuration_->IsSupported(BUILDFLAG(GOOGLE_CHROME_BRANDING),
chrome::GetChannel())) {
if (!platform_configuration_->IsSupported(GetReleaseChannel())) {
return false;
}
......@@ -175,16 +185,14 @@ ThreadProfilerConfiguration::GenerateConfiguration(
if (process != metrics::CallStackProfileParams::BROWSER_PROCESS)
return PROFILE_FROM_COMMAND_LINE;
const version_info::Channel channel = chrome::GetChannel();
if (!platform_configuration.IsSupported(BUILDFLAG(GOOGLE_CHROME_BRANDING),
channel)) {
const base::Optional<version_info::Channel> release_channel =
GetReleaseChannel();
if (!platform_configuration.IsSupported(release_channel))
return PROFILE_DISABLED;
}
using RuntimeModuleState =
ThreadProfilerPlatformConfiguration::RuntimeModuleState;
switch (platform_configuration.GetRuntimeModuleState(
BUILDFLAG(GOOGLE_CHROME_BRANDING), channel)) {
switch (platform_configuration.GetRuntimeModuleState(release_channel)) {
case RuntimeModuleState::kModuleAbsentButAvailable:
platform_configuration.RequestRuntimeModuleInstall();
FALLTHROUGH;
......@@ -197,12 +205,8 @@ ThreadProfilerConfiguration::GenerateConfiguration(
}
ThreadProfilerPlatformConfiguration::RelativePopulations
relative_populations = platform_configuration.GetEnableRates(
BUILDFLAG(GOOGLE_CHROME_BRANDING), channel);
if (relative_populations.enabled == 0 &&
relative_populations.experiment == 0) {
return PROFILE_DISABLED;
}
relative_populations =
platform_configuration.GetEnableRates(release_channel);
CHECK_EQ(0, relative_populations.experiment % 2);
return ChooseConfiguration({
......
......@@ -5,6 +5,7 @@
#include "chrome/common/profiler/thread_profiler_platform_configuration.h"
#include "base/command_line.h"
#include "base/notreached.h"
#include "base/profiler/stack_sampling_profiler.h"
#include "build/build_config.h"
#include "chrome/common/profiler/process_type.h"
......@@ -24,12 +25,10 @@ class DefaultPlatformConfiguration
// ThreadProfilerPlatformConfiguration:
RuntimeModuleState GetRuntimeModuleState(
bool is_chrome_branded,
version_info::Channel channel) const override;
base::Optional<version_info::Channel> release_channel) const override;
RelativePopulations GetEnableRates(
bool is_chrome_branded,
version_info::Channel channel) const override;
base::Optional<version_info::Channel> release_channel) const override;
double GetChildProcessEnableFraction(
metrics::CallStackProfileParams::Process process) const override;
......@@ -39,8 +38,8 @@ class DefaultPlatformConfiguration
metrics::CallStackProfileParams::Thread thread) const override;
protected:
bool IsSupportedForChannel(bool is_chrome_branded,
version_info::Channel channel) const override;
bool IsSupportedForChannel(
base::Optional<version_info::Channel> release_channel) const override;
bool browser_test_mode_enabled() const { return browser_test_mode_enabled_; }
......@@ -54,29 +53,24 @@ DefaultPlatformConfiguration::DefaultPlatformConfiguration(
ThreadProfilerPlatformConfiguration::RuntimeModuleState
DefaultPlatformConfiguration::GetRuntimeModuleState(
bool is_chrome_branded,
version_info::Channel channel) const {
base::Optional<version_info::Channel> release_channel) const {
return RuntimeModuleState::kModuleNotRequired;
}
ThreadProfilerPlatformConfiguration::RelativePopulations
DefaultPlatformConfiguration::GetEnableRates(
bool is_chrome_branded,
version_info::Channel channel) const {
// TODO(https://crbug.com/1129939): Make this logic consistent with
// IsSupportedForChannel() for identifying local/CQ builds.
switch (channel) {
// Enable the profiler unconditionally for development/waterfall builds.
case version_info::Channel::UNKNOWN:
return RelativePopulations{100, 0};
case version_info::Channel::CANARY:
case version_info::Channel::DEV:
return RelativePopulations{80, 20};
base::Optional<version_info::Channel> release_channel) const {
CHECK(IsSupportedForChannel(release_channel));
default:
return RelativePopulations{0, 0};
if (!release_channel) {
// This is a local/CQ build.
return RelativePopulations{100, 0};
}
CHECK(*release_channel == version_info::Channel::CANARY ||
*release_channel == version_info::Channel::DEV);
return RelativePopulations{80, 20};
}
double DefaultPlatformConfiguration::GetChildProcessEnableFraction(
......@@ -109,16 +103,15 @@ bool DefaultPlatformConfiguration::IsEnabledForThread(
}
bool DefaultPlatformConfiguration::IsSupportedForChannel(
bool is_chrome_branded,
version_info::Channel channel) const {
base::Optional<version_info::Channel> release_channel) const {
// The profiler is always supported for local builds and the CQ.
if (!is_chrome_branded)
if (!release_channel)
return true;
// Canary and dev are the only channels currently supported in release
// builds.
return channel == version_info::Channel::CANARY ||
channel == version_info::Channel::DEV;
return *release_channel == version_info::Channel::CANARY ||
*release_channel == version_info::Channel::DEV;
}
#if defined(OS_ANDROID)
......@@ -132,8 +125,7 @@ class AndroidPlatformConfiguration : public DefaultPlatformConfiguration {
// DefaultPlatformConfiguration:
RuntimeModuleState GetRuntimeModuleState(
bool is_chrome_branded,
version_info::Channel channel) const override;
base::Optional<version_info::Channel> release_channel) const override;
void RequestRuntimeModuleInstall() const override;
......@@ -145,8 +137,8 @@ class AndroidPlatformConfiguration : public DefaultPlatformConfiguration {
metrics::CallStackProfileParams::Thread thread) const override;
protected:
bool IsSupportedForChannel(bool is_chrome_branded,
version_info::Channel channel) const override;
bool IsSupportedForChannel(
base::Optional<version_info::Channel> release_channel) const override;
};
AndroidPlatformConfiguration::AndroidPlatformConfiguration(
......@@ -155,22 +147,21 @@ AndroidPlatformConfiguration::AndroidPlatformConfiguration(
ThreadProfilerPlatformConfiguration::RuntimeModuleState
AndroidPlatformConfiguration::GetRuntimeModuleState(
bool is_chrome_branded,
version_info::Channel channel) const {
base::Optional<version_info::Channel> release_channel) const {
// The module will be present in releases due to having been installed via
// RequestRuntimeModuleInstall(), and in local/CQ builds of bundle targets
// where the module was installed with the bundle.
if (stack_unwinder::Module::IsInstalled())
return RuntimeModuleState::kModulePresent;
if (is_chrome_branded) {
if (release_channel) {
// We only want to incur the cost of universally downloading the module in
// early channels, where profiling will occur over substantially all of
// the population. When supporting later channels in the future we will
// enable profiling for only a fraction of users and only download for
// those users.
if (channel == version_info::Channel::CANARY ||
channel == version_info::Channel::DEV) {
if (*release_channel == version_info::Channel::CANARY ||
*release_channel == version_info::Channel::DEV) {
return RuntimeModuleState::kModuleAbsentButAvailable;
}
......@@ -214,8 +205,7 @@ bool AndroidPlatformConfiguration::IsEnabledForThread(
}
bool AndroidPlatformConfiguration::IsSupportedForChannel(
bool is_chrome_branded,
version_info::Channel channel) const {
base::Optional<version_info::Channel> release_channel) const {
// On Android profiling is only enabled in its own dedicated browser tests
// in local builds and the CQ.
// TODO(https://crbug.com/1004855): Enable across all browser tests.
......@@ -237,8 +227,7 @@ ThreadProfilerPlatformConfiguration::Create(bool browser_test_mode_enabled) {
}
bool ThreadProfilerPlatformConfiguration::IsSupported(
bool is_chrome_branded,
version_info::Channel channel) const {
base::Optional<version_info::Channel> release_channel) const {
return base::StackSamplingProfiler::IsSupportedForCurrentPlatform() &&
IsSupportedForChannel(is_chrome_branded, channel);
IsSupportedForChannel(release_channel);
}
......@@ -7,6 +7,7 @@
#include <memory>
#include "base/optional.h"
#include "components/metrics/call_stack_profile_params.h"
#include "components/version_info/version_info.h"
......@@ -14,14 +15,19 @@
//
// The interface functions this class make a distinction between 'supported' and
// 'enabled' state. Supported means the profiler can be run in *some*
// circumstances for *some* fraction of the population on the
// platform/branding/channel combination. This state is intended to enable
// experiment reporting. This avoids spamming UMA with experiment state on
// platforms/channels where the profiler is not being run.
// circumstances for *some* fraction of the population on the platform/{released
// Chrome channel, development/CQ build} combination. This state is intended to
// enable experiment reporting. This avoids spamming UMA with experiment state
// on platforms/channels where the profiler is not being run.
//
// Enabled means we chose to the run the profiler on at least some threads on a
// platform/branding/channel combination that is configured for profiling. The
// overall enable/disable state should be reported to UMA in this case.
// platform/{released Chrome channel, development/CQ build} combination that is
// configured for profiling. The overall enable/disable state should be reported
// to UMA in this case.
//
// The base::Optional<version_info::Channel> release_channel passed to functions
// in this interface should be the channel for released Chrome and nullopt for
// development/CQ builds.
class ThreadProfilerPlatformConfiguration {
public:
// State of the runtime module used by the profiler on the platform (if any).
......@@ -41,8 +47,7 @@ class ThreadProfilerPlatformConfiguration {
// |enabled| + |experiment| is expected to equal 100. Profiling is to be
// enabled with probability |enabled|/100. The fraction |experiment|/100 is to
// be split in to two equal-sized experiment groups with probability
// |experiment|/(2 * 100), one of which will be enabled and one disabled. As a
// special case {0, 0} means always disable.
// |experiment|/(2 * 100), one of which will be enabled and one disabled.
struct RelativePopulations {
int enabled;
int experiment;
......@@ -55,26 +60,24 @@ class ThreadProfilerPlatformConfiguration {
bool browser_test_mode_enabled);
// True if the platform supports the StackSamplingProfiler and the profiler is
// to be run for the channel/chrome branding.
bool IsSupported(bool is_chrome_branded, version_info::Channel channel) const;
// to be run for the released Chrome channel or development/CQ build.
bool IsSupported(base::Optional<version_info::Channel> release_channel) const;
// Returns the current state of the runtime support module for the
// channel/chrome branding on the platform. Runtime module state is valid only
// if IsSupported().
// Returns the current state of the runtime support module for the released
// Chrome channel or development/CQ build on the platform. Runtime module
// state is valid only if IsSupported().
virtual RuntimeModuleState GetRuntimeModuleState(
bool is_chrome_branded,
version_info::Channel channel) const = 0;
base::Optional<version_info::Channel> release_channel) const = 0;
// Request install of the runtime support module. May be invoked only if
// GetRuntimeModuleState() returns kModuleAbsentButAvailable.
virtual void RequestRuntimeModuleInstall() const {}
// Returns the relative population disposition for the channel/chrome branding
// on the platform. See the documentation on RelativePopulations. Enable rates
// are valid only if IsSupported().
// Returns the relative population disposition for the released Chrome channel
// or development/CQ build on the platform. See the documentation on
// RelativePopulations. Enable rates are valid only if IsSupported().
virtual RelativePopulations GetEnableRates(
bool is_chrome_branded,
version_info::Channel channel) const = 0;
base::Optional<version_info::Channel> release_channel) const = 0;
// Returns the fraction of the time that profiling should be randomly enabled
// for the child |process|. The return value is in the range [0.0, 1.0].
......@@ -87,11 +90,12 @@ class ThreadProfilerPlatformConfiguration {
metrics::CallStackProfileParams::Thread thread) const = 0;
protected:
// True if the profiler is to be run for the channel/chrome branding on the
// platform. Does not need to check whether the StackSamplingProfiler is
// supported on the platform since that's done in IsSupported().
virtual bool IsSupportedForChannel(bool is_chrome_branded,
version_info::Channel channel) const = 0;
// True if the profiler is to be run for the released Chrome channel or
// development/CQ build on the platform. Does not need to check whether the
// StackSamplingProfiler is supported on the platform since that's done in
// IsSupported().
virtual bool IsSupportedForChannel(
base::Optional<version_info::Channel> release_channel) const = 0;
};
#endif // CHROME_COMMON_PROFILER_THREAD_PROFILER_PLATFORM_CONFIGURATION_H_
......@@ -7,6 +7,7 @@
#include <utility>
#include "base/profiler/profiler_buildflags.h"
#include "base/test/gtest_util.h"
#include "build/build_config.h"
#include "components/version_info/version_info.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -62,47 +63,29 @@ bool operator==(
TEST_F(ThreadProfilerPlatformConfigurationTest, IsSupported) {
#if !THREAD_PROFILER_SUPPORTED_ON_PLATFORM
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::CANARY));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::DEV));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::BETA));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::STABLE));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::UNKNOWN));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::CANARY));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::DEV));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::BETA));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::STABLE));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/false,
version_info::Channel::UNKNOWN));
EXPECT_FALSE(config()->IsSupported(base::nullopt));
#elif defined(OS_ANDROID)
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::CANARY));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::DEV));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::BETA));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::STABLE));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::UNKNOWN));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::CANARY));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::DEV));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::BETA));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::STABLE));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/false,
version_info::Channel::UNKNOWN));
EXPECT_FALSE(config()->IsSupported(base::nullopt));
#else
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
EXPECT_TRUE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::CANARY));
EXPECT_TRUE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::DEV));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::BETA));
EXPECT_FALSE(config()->IsSupported(/*is_chrome_branded=*/true,
version_info::Channel::STABLE));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::UNKNOWN));
EXPECT_TRUE(config()->IsSupported(version_info::Channel::CANARY));
EXPECT_TRUE(config()->IsSupported(version_info::Channel::DEV));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::BETA));
EXPECT_FALSE(config()->IsSupported(version_info::Channel::STABLE));
EXPECT_TRUE(config()->IsSupported(/*is_chrome_branded=*/false,
version_info::Channel::UNKNOWN));
EXPECT_TRUE(config()->IsSupported(base::nullopt));
#endif
}
......@@ -112,70 +95,53 @@ MAYBE_PLATFORM_CONFIG_TEST_F(ThreadProfilerPlatformConfigurationTest,
ThreadProfilerPlatformConfiguration::RuntimeModuleState;
#if defined(OS_ANDROID)
EXPECT_EQ(RuntimeModuleState::kModuleNotAvailable,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
config()->GetRuntimeModuleState(version_info::Channel::UNKNOWN));
EXPECT_EQ(RuntimeModuleState::kModuleAbsentButAvailable,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::CANARY));
config()->GetRuntimeModuleState(version_info::Channel::CANARY));
EXPECT_EQ(RuntimeModuleState::kModuleAbsentButAvailable,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::DEV));
config()->GetRuntimeModuleState(version_info::Channel::DEV));
EXPECT_EQ(RuntimeModuleState::kModuleNotAvailable,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::BETA));
config()->GetRuntimeModuleState(version_info::Channel::BETA));
EXPECT_EQ(RuntimeModuleState::kModuleNotAvailable,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::STABLE));
config()->GetRuntimeModuleState(version_info::Channel::STABLE));
EXPECT_EQ(RuntimeModuleState::kModuleNotAvailable,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
config()->GetRuntimeModuleState(version_info::Channel::UNKNOWN));
#else
EXPECT_EQ(RuntimeModuleState::kModuleNotRequired,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
config()->GetRuntimeModuleState(version_info::Channel::UNKNOWN));
EXPECT_EQ(RuntimeModuleState::kModuleNotRequired,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::CANARY));
config()->GetRuntimeModuleState(version_info::Channel::CANARY));
EXPECT_EQ(RuntimeModuleState::kModuleNotRequired,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::DEV));
config()->GetRuntimeModuleState(version_info::Channel::DEV));
EXPECT_EQ(RuntimeModuleState::kModuleNotRequired,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::BETA));
config()->GetRuntimeModuleState(version_info::Channel::BETA));
EXPECT_EQ(RuntimeModuleState::kModuleNotRequired,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::STABLE));
config()->GetRuntimeModuleState(version_info::Channel::STABLE));
EXPECT_EQ(RuntimeModuleState::kModuleNotRequired,
config()->GetRuntimeModuleState(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
config()->GetRuntimeModuleState(version_info::Channel::UNKNOWN));
#endif
}
MAYBE_PLATFORM_CONFIG_TEST_F(ThreadProfilerPlatformConfigurationTest,
GetEnableRates) {
// Note: death tests aren't supported on Android. Otherwise this test would
// check that all inputs result in CHECKs.
#if !defined(OS_ANDROID)
using RelativePopulations =
ThreadProfilerPlatformConfiguration::RelativePopulations;
EXPECT_EQ((RelativePopulations{100, 0}),
config()->GetEnableRates(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
EXPECT_CHECK_DEATH(config()->GetEnableRates(version_info::Channel::UNKNOWN));
EXPECT_EQ((RelativePopulations{80, 20}),
config()->GetEnableRates(/*is_chrome_branded=*/true,
version_info::Channel::CANARY));
config()->GetEnableRates(version_info::Channel::CANARY));
EXPECT_EQ((RelativePopulations{80, 20}),
config()->GetEnableRates(/*is_chrome_branded=*/true,
version_info::Channel::DEV));
EXPECT_EQ((RelativePopulations{0, 0}),
config()->GetEnableRates(/*is_chrome_branded=*/true,
version_info::Channel::BETA));
EXPECT_EQ((RelativePopulations{0, 0}),
config()->GetEnableRates(/*is_chrome_branded=*/true,
version_info::Channel::STABLE));
config()->GetEnableRates(version_info::Channel::DEV));
EXPECT_CHECK_DEATH(config()->GetEnableRates(version_info::Channel::BETA));
EXPECT_CHECK_DEATH(config()->GetEnableRates(version_info::Channel::STABLE));
EXPECT_EQ((RelativePopulations{100, 0}),
config()->GetEnableRates(/*is_chrome_branded=*/true,
version_info::Channel::UNKNOWN));
config()->GetEnableRates(base::nullopt));
#endif
}
MAYBE_PLATFORM_CONFIG_TEST_F(ThreadProfilerPlatformConfigurationTest,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment