Skip to content

Commit 6f0ca70

Browse files
committed
Clamp event timestamps to i64::MAX
BigQuery's integer type is a signed 64-bit value, while we collect unsigned ones. Timestamps are in milliseconds, so 9223372036854775807 (i64::MAX) ms is about 4.5 billion years, or roughly 1/15 of the age of earth. I think we should be fine with that for a little while longer. So any such value _should_ not happen. But it does in 0.002% of pings and thus data ends up in `additional_properties` instead of where it belongs. By clamping we might reduce that %age further. An event with that timestamp is most likely still not useful, but maybe the others are? One more problem is: Should we report this as an issue? If so how? Right now we don't have the APIs to tie that back to a specific event other then the one we own. But then our error types don't express the right things.
1 parent 770859d commit 6f0ca70

File tree

1 file changed

+76
-0
lines changed
  • glean-core/src/event_database

1 file changed

+76
-0
lines changed

glean-core/src/event_database/mod.rs

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,26 @@ impl EventDatabase {
560560
// Let's fix cur_ec up and hope this isn't a sign something big is broken.
561561
cur_ec = execution_counter;
562562
}
563+
564+
// event timestamp is a `u64`, but BigQuery uses `i64` (signed!) everywhere. Let's clamp the value to make
565+
// sure we stay within bounds.
566+
if event.event.timestamp > i64::MAX as u64 {
567+
record_error(
568+
glean,
569+
// We don't have the info about the actual event here, so we tackle it onto our own event.
570+
// Also none of the error types are expressing what actually happened.
571+
&glean_restarted_meta(store_name).into(),
572+
ErrorType::InvalidState,
573+
format!(
574+
"Calculated event timestamp was too high. Got: {}, max: {}",
575+
event.event.timestamp,
576+
i64::MAX,
577+
),
578+
None,
579+
);
580+
event.event.timestamp = event.event.timestamp.clamp(0, i64::MAX as u64);
581+
}
582+
563583
if highest_ts > event.event.timestamp {
564584
// Even though we sorted everything, something in the
565585
// execution_counter or glean.startup.date math went awry.
@@ -1357,4 +1377,60 @@ mod test {
13571377
)
13581378
.is_err());
13591379
}
1380+
1381+
#[test]
1382+
fn normalize_store_clamps_timestamp() {
1383+
// TODO
1384+
let (glean, _dir) = new_glean(None);
1385+
1386+
let store_name = "store-name";
1387+
let event = RecordedEvent {
1388+
category: "category".into(),
1389+
name: "name".into(),
1390+
..Default::default()
1391+
};
1392+
1393+
let timestamps = [
1394+
0,
1395+
(i64::MAX / 2) as u64,
1396+
i64::MAX as _,
1397+
(i64::MAX as u64) + 1,
1398+
];
1399+
let mut store = timestamps
1400+
.into_iter()
1401+
.map(|timestamp| StoredEvent {
1402+
event: RecordedEvent {
1403+
timestamp,
1404+
..event.clone()
1405+
},
1406+
execution_counter: None,
1407+
})
1408+
.collect();
1409+
1410+
let glean_start_time = glean.start_time();
1411+
glean
1412+
.event_storage()
1413+
.normalize_store(&glean, store_name, &mut store, glean_start_time);
1414+
assert_eq!(4, store.len());
1415+
1416+
assert_eq!(0, store[0].event.timestamp);
1417+
assert_eq!((i64::MAX / 2) as u64, store[1].event.timestamp);
1418+
assert_eq!((i64::MAX as u64), store[2].event.timestamp);
1419+
assert_eq!((i64::MAX as u64), store[3].event.timestamp);
1420+
1421+
let errors = test_get_num_recorded_errors(
1422+
&glean,
1423+
&CommonMetricData {
1424+
name: "restarted".into(),
1425+
category: "glean".into(),
1426+
send_in_pings: vec![store_name.into()],
1427+
lifetime: Lifetime::Ping,
1428+
..Default::default()
1429+
}
1430+
.into(),
1431+
ErrorType::InvalidState,
1432+
);
1433+
1434+
assert_eq!(Ok(1), errors);
1435+
}
13601436
}

0 commit comments

Comments
 (0)