-
Notifications
You must be signed in to change notification settings - Fork 181
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.
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
base: main
Are you sure you want to change the base?
Conversation
71907e7 to
b91fb1f
Compare
The main reason for the change is to use the presence of the event files as a signal that metrics transmissions are working. That signal will be used in an upcoming presubmit test to verify metrics functionality. This means the files will not be written out for debugging when metrics are disabled. However, that is offset for Googlers who will have the package installed by default and now have metrics enabled even for local development. Since the event files are only written after transmissions, I think there is less likelihood of confusion on whether metrics are being transmitted. This commit also removes the README that tried to explain this to users who stumbled upon the `metrics` directory. Bug: 453757034
To be used in the upcoming metrics transmission presubmit test Bug: 453757034
Enable metrics for our Github presubmit tests by including the package in the container definition used by `presubmit.yaml`. Bug: 453757034
| if ! [[& "`ls ${metrics_dir}/device_boot_complete*.txtpb`" ]]; then | ||
| echo "metrics not transmitted for the expected boot complete event" | ||
| exit | ||
| fi |
There was a problem hiding this comment.
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 (
| 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?
There was a problem hiding this comment.
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:
| "{}/{}_{}_{}.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:
cvdis 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.
This test works by using the existence of the metrics event files, which are now written AFTER transmission, as a signal metrics were transmitted successfully. Bug: 453757034
ce50854 to
183b800
Compare
Bug: 453757034