Skip to content

Add presubmit test to verify v2 metrics are working when enabled #1980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cjreynol wants to merge 4 commits into
base: main
Choose a base branch
from
+130 −11
Open

Add presubmit test to verify v2 metrics are working when enabled #1980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
32d205c
Only write metrics event files when enabled
cjreynol Jan 8, 2026
7724afd
Add group metrics directory to `cvd fleet` output
cjreynol Jan 8, 2026
b868adf
Install `cuttlefish-metrics` to test container
cjreynol Jan 8, 2026
183b800
Add presubmit test to verify metrics transmission
cjreynol Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions base/cvd/cuttlefish/host/commands/cvd/instances/local_instance_group.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ Result<Json::Value> LocalInstanceGroup::FetchStatus(
}
Json::Value group_json;
group_json["group_name"] = GroupName();
group_json["metrics_dir"] = MetricsDir();
group_json["start_time"] = Format(StartTime());
group_json["instances"] = instances_json;
return group_json;
Expand Down
6 changes: 0 additions & 6 deletions base/cvd/cuttlefish/host/libs/metrics/enabled.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@

namespace cuttlefish {

inline constexpr std::string_view kReadmeText =
"The existence of records in this directory does"
" not mean metrics are being transmitted. The data is always gathered and "
"written out for debugging purposes. To enable metrics transmission "
"the `cuttlefish-metrics` package must be installed.";

inline constexpr char kTransmitterPath[] =
"/usr/lib/cuttlefish-metrics/bin/metrics_transmitter";

Expand Down
8 changes: 4 additions & 4 deletions base/cvd/cuttlefish/host/libs/metrics/metrics_orchestration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ MetricsPaths GetMetricsPaths(const LocalInstanceGroup& instance_group) {

Result<void> SetUpMetrics(const std::string& metrics_directory) {
CF_EXPECT(EnsureDirectoryExists(metrics_directory));
CF_EXPECT(WriteNewFile(metrics_directory + "/README", kReadmeText));
CF_EXPECT(GenerateSessionIdFile(metrics_directory));
return {};
}
Expand All @@ -117,11 +116,12 @@ Result<MetricsData> GatherMetrics(const MetricsPaths& metrics_paths,
Result<void> OutputMetrics(EventType event_type,
const MetricsPaths& metrics_paths,
const MetricsData& metrics_data) {
const CuttlefishLogEvent cf_log_event = BuildCuttlefishLogEvent(metrics_data);
CF_EXPECT(WriteMetricsEvent(event_type, metrics_paths.metrics_directory,
cf_log_event));
if (AreMetricsEnabled()) {
const CuttlefishLogEvent cf_log_event =
BuildCuttlefishLogEvent(metrics_data);
CF_EXPECT(TransmitMetrics(kTransmitterPath, cf_log_event));
CF_EXPECT(WriteMetricsEvent(event_type, metrics_paths.metrics_directory,
cf_log_event));
}
return {};
}
Expand Down
8 changes: 7 additions & 1 deletion e2etests/cvd/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("boot_tests.bzl", "cvd_command_boot_test", "cvd_load_boot_test", "launch_cvd_boot_test")
load("boot_tests.bzl", "cvd_command_boot_test", "cvd_load_boot_test", "launch_cvd_boot_test", "metrics_test")

# cvd fetch + launch_cvd tests
launch_cvd_boot_test(
Expand Down Expand Up @@ -116,4 +116,10 @@ cvd_command_boot_test(
tags = ["requires_gpu"],
)

metrics_test(
name = "verify_metrics_transmission",
branch = "aosp-android-latest-release",
target = "aosp_cf_x86_64_only_phone-userdebug",
)

# TODO(b/329100411) test loading older branches
16 changes: 16 additions & 0 deletions e2etests/cvd/boot_tests.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,19 @@ def cvd_command_boot_test(name, branch, target, cvd_command = [], credential_sou
"no-sandbox",
] + tags,
)

def metrics_test(name, branch, target, credential_source = ""):
args = ["-b", branch, "-t", target]
if credential_source:
args += ["-c", credential_source]
native.sh_test(
name = name,
size = "medium",
srcs = ["metrics_test.sh"],
args = args,
tags = [
"exclusive",
"external",
"no-sandbox",
],
)
101 changes: 101 additions & 0 deletions e2etests/cvd/metrics_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
#!/usr/bin/env bash

set -e -x

BRANCH=""
TARGET=""
CREDENTIAL_SOURCE="${CREDENTIAL_SOURCE:-}"

while getopts "b:c:t:" opt; do
case "${opt}" in
b)
BRANCH="${OPTARG}"
;;
c)
CREDENTIAL_SOURCE="${OPTARG}"
;;
t)
TARGET="${OPTARG}"
;;
*)
echo "Unknown flag: -${opt}" >&2
echo "Usage: $0 -b BRANCH -t TARGET"
exit 1
esac
done
shift $((OPTIND-1))

if [[ "${BRANCH}" == "" ]]; then
echo "Missing required -b argument"
fi

if [[ "${TARGET}" == "" ]]; then
echo "Missing required -t argument"
fi

workdir="$(mktemp -d -t cvd_command_test.XXXXXX)"

function collect_logs_and_cleanup() {
# Don't immediately exit on failure anymore
set +e
if [[ -n "${TEST_UNDECLARED_OUTPUTS_DIR}" ]] && [[ -d "${TEST_UNDECLARED_OUTPUTS_DIR}" ]]; then
cp "${workdir}"/*.log "${TEST_UNDECLARED_OUTPUTS_DIR}"
cp "${workdir}"/cuttlefish_runtime/*.log "${TEST_UNDECLARED_OUTPUTS_DIR}"
cp "${workdir}"/cuttlefish_runtime/logcat "${TEST_UNDECLARED_OUTPUTS_DIR}"
cp "${workdir}"/cuttlefish_runtime/cuttlefish_config.json "${TEST_UNDECLARED_OUTPUTS_DIR}"
fi
rm -rf "${workdir}"
# Be nice, don't leave devices behind.
cvd reset -y
}

# Regardless of whether and when a failure occurs logs must collected
trap collect_logs_and_cleanup EXIT

# Make sure to run in a clean environment, without any devices running
cvd reset -y

credential_arg=""
if [[ -n "$CREDENTIAL_SOURCE" ]]; then
credential_arg="--credential_source=${CREDENTIAL_SOURCE}"
fi

cvd fetch \
--default_build="${BRANCH}/${TARGET}" \
--target_directory="${workdir}" \
${credential_arg}

(
cd "${workdir}"

# generate instantiation, start, and boot complete events
cvd create --host_path="${workdir}" --product_path="${workdir}"

# verify transmission by detecting existence of the metrics directory and the debug event files
metrics_dir=`cvd fleet | jq --raw-output '.groups.[0].metrics_dir'`
if ! [[ -d "${metrics_dir}" ]]; then
echo "metrics directory not found"
exit
fi

# file prefixes sourced from `cuttlefish/host/libs/metrics/event_type.cc::EventTypeString` function
if ! [[ -f "`ls ${metrics_dir}/device_instantiation*.txtpb`" ]]; then
echo "metrics not transmitted for the expected instantiation event"
exit
fi
if ! [[ -f "`ls ${metrics_dir}/device_boot_start*.txtpb`" ]]; then
echo "metrics not transmitted for the expected start event"
exit
fi
if ! [[ -f "`ls ${metrics_dir}/device_boot_complete*.txtpb`" ]]; then
echo "metrics not transmitted for the expected boot complete event"
exit
fi
Copy link
Member

@jmacnak jmacnak Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it needs to block this PR but I find this one file per event type quite strange.

It also seems like this file generation as-is right now completely overwrites any existing file (

android-cuttlefish/base/cvd/cuttlefish/host/libs/metrics/metrics_writer.cc

Line 51 in 35f43d8

CF_EXPECT(WriteNewFile(event_filepath, text_proto_out));
)

I think it is feasible to have repeating events. Could the existing boot completed event trigger twice currently if someone did a adb reboot ?

I guess I would imagine us having something like a

message CuttlefishLogEvents {
   repeated CuttlefishLogEvent event = 1;
}

and then there would be a single "metrics.txtpb" file which can be written to in an appending fashion.

WDYT?

Copy link
Collaborator Author

@cjreynol cjreynol Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use the <event type>*.txtpb to simplify the checks here because we are controlling the cvd invocations and know there will only be one event per type, but the full filepath for each event is generated with the format:

<event type>_<datetimestamp>_<random 10 digit suffix>.txtpb

to avoid the possibility of overwriting.

reference:

android-cuttlefish/base/cvd/cuttlefish/host/libs/metrics/metrics_writer.cc

Line 47 in 35f43d8

"{}/{}_{}_{}.txtpb", metrics_directory, EventTypeString(event_type),

Each event is also per group, capturing the details for each instance within the group (since we cannot start/stop individual instances).

Which all means that it is fine to have an invocation sequence like:

`cvd create`  # starts by default
`cvd stop`
`cvd start`
`cvd stop`

and the resulting <group_root_directory>/metrics/ directory could have event files roughly like:

'device_instantiation_2026-01-09 16:05:00.12345_1234561111.txtpb'
'device_boot_start_2026-01-09 16:05:10.12345_1234562222.txtpb'
'device_boot_complete_2026-01-09 16:06:15.12345_1234563333.txtpb'
'device_boot_stop_2026-01-09 16:12:00.12345_123456444.txtpb'
'device_boot_start_2026-01-09 16:23:00.12345_1234565555.txtpb'
'device_boot_complete_2026-01-09 16:24:15.12345_123456666.txtpb'
'device_boot_stop_2026-01-09 16:38:00.12345_123457777.txtpb'

The timestamps are the practical differentiator, though the random number suffix is there in the case where two event types somehow occurred on the same group at the same time. I believe cvd prevents that from being possible in practice, but just in case.

I do not think an adb reboot will trigger new events, since the V2 metrics are all cvd generated (to ideally work identically across any build/host tools). The V1 metrics might capture that, but I do not know either way.

All of that said, I am not opposed to a single, expanding file. The main drivers for going this direction were:

  • cvd is doing the writing (and thus transmitting) at the time of each event rather than gathering up events and doing it all at once (to avoid missing events due to unexpected failures). Coming up with a unique filename and writing a separate file matched that one-event-at-a-time structure.
  • I wanted to avoid consideration for concurrent writing to a single file if/when the amount of metrics events expands.


# generate stop event and verify transmission
cvd stop
if ! [[ -f "`ls ${metrics_dir}/device_stop*.txtpb`" ]]; then
echo "metrics not transmitted for the expected stop event"
exit
fi
)
1 change: 1 addition & 0 deletions tools/testutils/cw/Containerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ FROM with_bazel AS with_cuttlefish_packages

RUN --mount=source=.,target=/mnt,type=bind apt-get install -y \
/mnt/cuttlefish-base_*_*64.deb \
/mnt/cuttlefish-metrics_*_*64.deb \
/mnt/cuttlefish-user_*_*64.deb \
/mnt/cuttlefish-orchestration_*_*64.deb

Expand Down
Loading