refactor(dup): use DateTime internally by noaccOS · Pull Request #1142 · astarte-platform/astarte

noaccOS · 2025-03-12T16:40:49Z

~~depends on #1134 to avoid a rebase~~

depends on astarte-platform/astarte_core#119 and astarte-platform/astarte_data_access#100

see #1129 for context

timestamps are preserved in telemetry.

I have read CONTRIBUTING.md and CODE_OF_CONDUCT.md
I have added to CHANGELOG.md relevant changes and any other user facing change
I have added or ran the appropriate tests

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 · 2025-03-13T16:13:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.96%. Comparing base (754277a) to head (3dce23f).

Additional details and impacted files

@@               Coverage Diff               @@
##           release-1.2    #1142      +/-   ##
===============================================
+ Coverage        63.88%   63.96%   +0.07%     
===============================================
  Files               30       30              
  Lines             1944     1948       +4     
===============================================
+ Hits              1242     1246       +4     
  Misses             702      702

☔ 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.

Annopaolo

I'm not sure there's a need for using the DecimicrosecondDateTime struct instead of a well-defined separation between integer timestamps and DateTimes, but if it works...

Annopaolo · 2025-03-17T14:39:04Z

apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/impl.ex

-  @groups_lifespan_decimicroseconds 60 * 10 * 1000 * 10000
-  @deletion_refresh_lifespan_decimicroseconds 60 * 10 * 1000 * 10000
-  @datastream_maximum_retention_refresh_lifespan_decimicroseconds 60 * 10 * 1000 * 10000
+  @interface_lifespan minute: 10


This seems rather ad hoc, I'd suggest using the standard :timer.minutes(10) and update DecimicrosecondDateTime.shift consequently.

It mimics DateTime.shift. Although the duration type is not yet available, we can mimic its functionality if we're just compiling a constant minute: 10 value

If you don't like, I'd argue for having two variables for each lifespan, as in

@interface_lifespan_amount 10 @interface_lifespan_unit :minute # usage DecimicrosecondDateTime.add(datetime, @interface_lifespan_amount, @interface_lifespan_unit)

DateTime.add expects a unit anyway, IMO it's easier to understand if we give it in minutes ourselves

I was not familiar with DateTime.shift's game, now I see why. Looks ok

noaccOS · 2025-03-17T15:13:52Z

I'm not sure there's a need for using the DecimicrosecondDateTime struct instead of a well-defined separation between integer timestamps and DateTimes, but if it works...

I started out using a tuple, but ended up using a struct because it allows easier matches here

eddbbt · 2025-03-18T14:11:13Z

This has been moved to DataAccess imho it shall not be merged as it is

import decimicrosecond datetime from astarte-platform/astarte#1142 this way it can be used in data_access's datetime for cast and split_submillis Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

davidebriani · 2025-03-18T14:54:27Z

To be honest, I am not sure what is the problem that the PR is trying to solve.
Maybe some more context could be added in the PR's description?

noaccOS · 2025-03-18T15:12:07Z

This has been moved to DataAccess imho it shall not be merged as it is

marking as draft as this was made dependent on prs in data_access and core

use DateTime.add directly instead of converting to and from timestamps, which makes it harder to understand what's happening Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

eddbbt · 2025-03-19T09:45:09Z

apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/impl.ex

  defp execute_time_based_actions(state, timestamp) do
-    if state.connected && state.last_seen_message > 0 do
+    if state.connected && DecimicrosecondDateTime.after?(state.last_seen_message, @epoch) do
      # timestamps are handled as microseconds*10, so we need to divide by 10 when saving as a metric for a coherent data


leftover (comment)

eddbbt · 2025-03-19T09:51:29Z

apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/queries.ex

  alias Astarte.Core.Device, as: CoreDevice
  alias Astarte.Core.InterfaceDescriptor
  alias Astarte.Core.Mapping
+  alias Astarte.DataAccess.DateTime, as: MsDateTime


we can remove the alias now

eddbbt · 2025-03-19T09:54:45Z

apps/astarte_data_updater_plant/mix.exs

+      {:astarte_core, github: "noaccOS/astarte_core", branch: "chore/decimicro"},
+      {:astarte_data_access, github: "noaccOS/astarte_data_access", branch: "chore/decimicro"},


those are not merged yet?

check PR description

implement a new type, DecimicrosecondDateTime to allow using DateTimes with one extra precision digit compared to std DateTimes. Functions from the original DateTime are implemented as they're needed. timestamps are preserved in telemetry. Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

noaccOS force-pushed the refactor/dup-datetime branch from 9e49bbe to a948e58 Compare March 12, 2025 16:41

noaccOS requested review from Annopaolo, davidebriani, eddbbt, lusergit and shinnokdisengir and removed request for Annopaolo and davidebriani March 12, 2025 16:44

noaccOS force-pushed the refactor/dup-datetime branch from a948e58 to df1f304 Compare March 13, 2025 13:26

noaccOS mentioned this pull request Mar 13, 2025

refactor(appengine): use datetimes for data insertion #1129

Merged

5 tasks

noaccOS force-pushed the refactor/dup-datetime branch 2 times, most recently from c1b32f0 to 522c3b5 Compare March 13, 2025 16:09

noaccOS force-pushed the refactor/dup-datetime branch from 522c3b5 to 5446dba Compare March 13, 2025 16:18

Annopaolo requested changes Mar 17, 2025

View reviewed changes

Annopaolo approved these changes Mar 18, 2025

View reviewed changes

noaccOS force-pushed the refactor/dup-datetime branch from 5446dba to ee888e8 Compare March 18, 2025 15:09

noaccOS marked this pull request as draft March 18, 2025 15:09

noaccOS mentioned this pull request Mar 18, 2025

chore: add decimicrosecond datetime astarte-platform/astarte_core#119

Open

noaccOS force-pushed the refactor/dup-datetime branch from ee888e8 to 8bb2a29 Compare March 18, 2025 15:16

noaccOS added 3 commits March 19, 2025 10:29

refactor(dup): make datetime operation explicit

55ceb22

use DateTime.add directly instead of converting to and from timestamps, which makes it harder to understand what's happening Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

refactor(dup): make payloads_decoder return a datetime

e9320f4

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

temp: use custom data access + core

1f02d3d

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

refactor(dup): use millisecond datetime on database side

bb90060

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

noaccOS force-pushed the refactor/dup-datetime branch from 8bb2a29 to ffbda9f Compare March 19, 2025 09:33

eddbbt reviewed Mar 19, 2025

View reviewed changes

noaccOS force-pushed the refactor/dup-datetime branch from ffbda9f to 3dce23f Compare March 19, 2025 10:03

		{:astarte_core, github: "noaccOS/astarte_core", branch: "chore/decimicro"},
		{:astarte_data_access, github: "noaccOS/astarte_data_access", branch: "chore/decimicro"},

Conversation

noaccOS commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

codecov bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Annopaolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noaccOS commented Mar 17, 2025

Uh oh!

eddbbt commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidebriani commented Mar 18, 2025

Uh oh!

noaccOS commented Mar 18, 2025

Uh oh!

eddbbt Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noaccOS commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

eddbbt commented Mar 18, 2025 •

edited

Loading

eddbbt Mar 19, 2025 •

edited

Loading