Repository navigation
Conversation
`Time.timestampUS` converted `Date().timeIntervalSince1970` to `UInt64` before scaling it to microseconds, which dropped the fractional second. Every `SessionStartEvent` therefore reported an `event_timestamp_us` floored to a whole second, while the Android SDK reports millisecond precision. The `UInt64` conversion would also trap if the device clock were ever set before 1970. Scale the `TimeInterval` before converting it to `Int64`, and let `Time` take an injectable date source (defaulting to `Date.init`, like `SessionInitiator`) so the conversion can be unit tested. Fixes #16796
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves an issue where Firebase Sessions truncated session start event timestamps to whole seconds. It updates the Time class to retain fractional seconds when calculating the microsecond timestamp and introduces a customizable currentTimeProvider to facilitate testing. Additionally, unit tests have been added to verify fractional seconds and negative timestamps. The feedback suggests using .rounded() before casting the floating-point product to Int64 to prevent truncation errors due to floating-point precision limits.
| var timestampUS: Int64 { | ||
| return Int64(UInt64(Date().timeIntervalSince1970) * USEC_PER_SEC) | ||
| return Int64(currentTime().timeIntervalSince1970 * Double(USEC_PER_SEC)) | ||
| } |
There was a problem hiding this comment.
Converting a Double directly to Int64 performs truncation (rounding towards zero). Due to floating-point precision limits, the product of timeIntervalSince1970 and USEC_PER_SEC might be slightly less than the expected integer value (e.g., 1700000000000002.999999), which would truncate down to 1700000000000002 instead of rounding to 1700000000000003. Using .rounded() before casting to Int64 ensures the nearest microsecond is chosen and avoids systematic negative bias.
| var timestampUS: Int64 { | |
| return Int64(UInt64(Date().timeIntervalSince1970) * USEC_PER_SEC) | |
| return Int64(currentTime().timeIntervalSince1970 * Double(USEC_PER_SEC)) | |
| } | |
| var timestampUS: Int64 { | |
| return Int64((currentTime().timeIntervalSince1970 * Double(USEC_PER_SEC)).rounded()) | |
| } |
There was a problem hiding this comment.
[paulb777-bot] Thanks. Truncation is intentional. A timestamp normally floors to its unit, and this matches how other SDKs convert dates to microseconds, such as timeIntervalSince1970 * USEC_PER_SEC in Performance's FPRNanoPbUtils.m. The difference is at most 1 µs, which doesn't matter for session events.
Problem
Time.timestampUScomputedInt64(UInt64(Date().timeIntervalSince1970) * USEC_PER_SEC). TheUInt64conversion drops the fractional second before the value is scaled. As a result,
the
event_timestamp_ussent with everySessionStartEventwas rounded down toa whole second, even though the field is in microseconds. This has been the case
since the file was added in #10311. Android sends
System.currentTimeMillis() * 1000, so it has millisecond precision. The sameUInt64conversion would also trap if the device clock were ever set before1970.
Change
TimeIntervalbyUSEC_PER_SECbefore converting toInt64.This keeps sub-second precision and removes the
UInt64conversion. Any realDateis far inside theInt64microsecond range (Date.distantFutureisabout 6.4e16 µs).
Timetake an injectable date source (currentTimeProvider, defaultingto
Date.init, as inSessionInitiator) so the conversion can be unit tested.TimeProvider,MockTimeProvider, and theSessionStartEventcall site areunchanged.
TimeTests, plus a Crashlytics CHANGELOG entry, since Sessions has nochangelog of its own.
Session timeout and rollover logic (
SessionInitiator) usesDatedirectly andis not affected.
Testing
timestampUSarithmetic reverted, injection kept):test_timestampUS_keepsFractionalSecondsfails withXCTAssertEqual failed: ("1700000000000000") is not equal to ("1700000000250000")and... ("1700000000750000"). Two times in the same second collapse to thesame value.
test_timestampUS_beforeUnixEpoch_isNegativecrashes withFatal error: Double value cannot be converted to UInt64 because the result would be less than UInt64.min.FirebaseSessionsUnittarget passes on macOS(66 tests, 0 failures), including both new tests and the existing
SessionStartEventTests../scripts/style.shleaves the changed files unchanged.scripts/check_whitespace.shandscripts/check_copyright.shpass.Fixes #16796