Extract timestamp as double for InMemorySessionService events

[tlanclos]

InMemorySessionService sets a Session's last modified time based on the last appended event's timestamp. The timestamp in an event is recorded in millis while the Session's timestamp is an Instant. During the transformation, Events perform this converstion using division. Before this change, the timestamp was truncated to the second, yet the code was trying to extract nanos which were always 0. This fixes that bug with a simple type change. I've also added a test to prevent regressions.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a bug in the InMemorySessionService where session last modified times were not accurately reflecting the full precision of event timestamps. By adjusting the timestamp conversion logic to preserve fractional seconds, the service now maintains more precise session state, which is crucial for accurate event-driven processing.

Highlights

• Timestamp Precision Fix: The InMemorySessionService now correctly handles fractional seconds when updating a session's last modified time. Previously, event timestamps, which are in milliseconds, were truncated to the nearest second during conversion to an Instant, leading to a loss of precision.
• Type Change for Division: The getEventTimestampEpochSeconds helper method in InMemorySessionService was updated from returning a long to a double, and the division operation was changed from integer division (/ 1000L) to floating-point division (/ 1000.0) to preserve sub-second precision.
• Regression Test Added: A new test case, appendEvent_updatesSessionTimestampWithFractionalSeconds, was added to InMemorySessionServiceTest to ensure that session timestamps are updated with correct fractional second precision and to prevent future regressions of this bug.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• core/src/main/java/com/google/adk/sessions/InMemorySessionService.java
  • Changed the return type of getEventTimestampEpochSeconds from long to double.
  • Modified the division in getEventTimestampEpochSeconds from integer (/ 1000L) to floating-point (/ 1000.0) to retain sub-second precision.
• core/src/test/java/com/google/adk/sessions/InMemorySessionServiceTest.java
  • Added an import for java.time.Instant.
  • Introduced a new test method appendEvent_updatesSessionTimestampWithFractionalSeconds to validate that session lastUpdateTime correctly includes fractional seconds.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. 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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request effectively addresses the bug where fractional seconds were truncated when updating a session's last modified time. The change to use a double for timestamp calculation in getEventTimestampEpochSeconds and the corresponding floating-point division correctly preserves the precision. The addition of a dedicated test case, appendEvent_updatesSessionTimestampWithFractionalSeconds, is a valuable improvement, ensuring that this specific scenario is covered and preventing future regressions. The changes are well-implemented and directly resolve the identified issue.

[anFatum]

Hello @tlanclos! Thank you for your contribution!

I think we actually could change the getInstantFromEvent to the

  private Instant getInstantFromEvent(Event event) {
    return Instant.ofEpochMilli(event.timestamp());
  }

since event.timestamp() returns milis.

I'm quite unsure about changing the type from long to double, given that we have a comparison of double and long getEventTimestampEpochSeconds(event) < threshold.getEpochSecond(), where threshold.getEpochSecond() returns long

I've checked and your test pass with the changed getInstantFromEvent method implementation. What do you think?

Additionally, I guess we might get rid of the getEventTimestampEpochSecond and achieve a cleaner result by replacing it with

event -> event.timestamp() < threshold.toEpochMilli()

[tlanclos]

Thanks for the suggestion, @anFatum! I originally took the approach that I did so that I introduced minimal code change; however, I agree that the type handling could be better. Instead of comparing against longs or doubles, I decided to remove getEventTimestampEpochSeconds and instead move the conversion to getInstantFromEvent since that method is used in a few places. I think the next step in cleaning this up would be to make Event.timestamp() an Instant as suggested in the TODOs.

[anFatum]

Thanks for your change @tlanclos! Looks good to me, the only thing I'll ask you to squash your commits so I can merge this PR, thank you!

[tlanclos]

No problem, got everything squashed! It should be ready for merge