Skip to content

fix(dup): last_connection / last_disconnection timestamp unit#1134

Merged
Annopaolo merged 1 commit intoastarte-platform:release-1.2from
noaccOS:fix/connection-unit
Mar 13, 2025
Merged

fix(dup): last_connection / last_disconnection timestamp unit#1134
Annopaolo merged 1 commit intoastarte-platform:release-1.2from
noaccOS:fix/connection-unit

Conversation

@noaccOS
Copy link
Copy Markdown
Collaborator

@noaccOS noaccOS commented Mar 11, 2025

a regression was introduced in 2c66918, where timestamps were in millisecond precision, but DateTime.from_unix! was given microsecond precision. This caused datetimes to be incorrect.

this was the result after setting up astarte in 5 minutes and running astarte-stream-qt5-test

❯ docker compose exec -it scylla cqlsh -e 'select last_connection from test.devices;'

 last_connection
---------------------------------
 1970-01-21 03:48:22.216000+0000

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?
  • Yes
  • No

Additional documentation e.g. usage docs, diagrams, etc.:


@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.24%. Comparing base (1ed24d1) to head (042574f).
Report is 2 commits behind head on release-1.2.

Additional details and impacted files
@@               Coverage Diff               @@
##           release-1.2    #1134      +/-   ##
===============================================
+ Coverage        63.21%   63.24%   +0.03%     
===============================================
  Files               41       41              
  Lines             1968     1970       +2     
===============================================
+ Hits              1244     1246       +2     
  Misses             724      724              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@noaccOS noaccOS marked this pull request as ready for review March 11, 2025 14:25
@noaccOS noaccOS requested review from Annopaolo, davidebriani, eddbbt and lusergit and removed request for Annopaolo and davidebriani March 11, 2025 14:27
Copy link
Copy Markdown
Collaborator

@lusergit lusergit left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Comment on lines +971 to +974
defp pad_usec(datetime) do
# Ecto requires :utc_datetime_usec to be in microsecond precision
microsecond =
case datetime.microsecond do
{_, 0} -> {0, 6}
{n, _} -> {n, 6}
end

%{datetime | microsecond: microsecond}
end
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just Ecto.Type.cast!(:utc_datetime_usec, datetime) should work.
I think the only issue is where the code is preparing database changes with Ecto.Changeset.change which doesn't automatically cast params according to their Ecto types.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If you don't like the case, I was experimenting and it looks like DateTime.add(datetime, 0, :microsecond) also sets the precision to 6

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've changed it to DateTime.add(datetime, 0, :microsecond)

@noaccOS noaccOS force-pushed the fix/connection-unit branch from 5403d94 to 8178378 Compare March 12, 2025 08:54
@noaccOS noaccOS force-pushed the fix/connection-unit branch from 8178378 to b82c32d Compare March 12, 2025 10:46
timestamps were in millisecond precision, but `DateTime.from_unix!` was
given microsecond precision. This caused datetimes to be incorrect.

this was the result after setting up astarte in 5 minutes and running
astarte-stream-qt5-test

```
❯ docker compose exec -it scylla cqlsh -e 'select last_connection from test.devices;'

 last_connection
---------------------------------
 1970-01-21 03:48:22.216000+0000
```

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>
@noaccOS noaccOS force-pushed the fix/connection-unit branch from b82c32d to 042574f Compare March 13, 2025 13:26
@Annopaolo Annopaolo merged commit e82576e into astarte-platform:release-1.2 Mar 13, 2025
5 checks passed
@noaccOS noaccOS deleted the fix/connection-unit branch March 13, 2025 14:54
AliouneGaye21 pushed a commit to AliouneGaye21/astarte that referenced this pull request Sep 22, 2025
As per astarte PR astarte-platform#1134

Signed-off-by: Eddy Babetto <eddy.babetto@secomind.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants