Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix diff_last aggregate calculation #1634

Closed
wants to merge 2 commits into from

Conversation

crnjan
Copy link
Contributor

@crnjan crnjan commented Jan 11, 2023

In order to calculate diff_last include boundary=true parameter when requesting datapoints to read datapoint before requested interval (before datestart), if available. Use that one to calculate diff for first value.

fixes #912 - partly, only applies for diff_last

Signed-off-by: Boris Krivonog boris.krivonog@inova.si

@crnjan crnjan requested a review from a team as a code owner January 11, 2023 09:26
@crnjan crnjan changed the title Fix diff_last calculation Fix diff_last aggregate calculation Jan 11, 2023
@relativeci
Copy link

relativeci bot commented Jan 11, 2023

Job #709: Bundle Size — 15.97MiB (~+0.01%).

cf993c8(current) vs 62f0bb4 main#708(baseline)

Metrics (no changes)
                 Current
Job #709
     Baseline
Job #708
Initial JS 1.73MiB 1.73MiB
Initial CSS 608.52KiB 608.52KiB
Cache Invalidation 90.48% 90.48%
Chunks 218 218
Assets 688 688
Modules 2011 2011
Duplicate Modules 108 108
Duplicate Code 1.8% 1.8%
Packages 133 133
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #709
     Baseline
Job #708
CSS 856.56KiB 856.56KiB
Fonts 1.08MiB 1.08MiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.04MiB (~-0.01%) 9.04MiB
Media 295.6KiB 295.6KiB
Other 4.59MiB (~+0.01%) 4.59MiB

View job #709 reportView main branch activity

@ghys
Copy link
Member

ghys commented Jan 16, 2023

When testing this PR with an existing chart I get (the server is 3.4.0 release build):

image

And the response is:
"{\"error\":{\"message\":\"Invalid start/end time in fetch request: 1675205999 \\u003e 1673910752\",\"http-code\":500,\"exception\":{\"class\":\"java.lang.IllegalArgumentException\",\"message\":\"Invalid start/end time in fetch request: 1675205999 \\u003e 1673910752\",\"localized-message\":\"Invalid start/end time in fetch request: 1675205999 \\u003e 1673910752\"}}}"
In the server logs:

[ERROR] [.internal.JSONResponseExceptionMapper] - Unexpected exception occurred while processing REST request.
java.lang.IllegalArgumentException: Invalid start/end time in fetch request: 1675205999 > 1673910931
        at org.rrd4j.core.FetchRequest.<init>(FetchRequest.java:38) ~[?:?]
        at org.rrd4j.core.RrdDb.createFetchRequest(RrdDb.java:943) ~[?:?]
        at org.openhab.persistence.rrd4j.internal.RRD4jPersistenceService.query(RRD4jPersistenceService.java:318) ~[?:?]
        at org.openhab.core.io.rest.core.internal.persistence.PersistenceResource.createDTO(PersistenceResource.java:352) ~[?:?]

It seems the rrd4j persistence service doesn't like the boundary parameter... which is a bummer since it's the one most would have.

Against main without the boundary parameter:

image

@cweitkamp
Copy link
Contributor

@cweitkamp
Copy link
Contributor

Signed-off-by: Boris Krivonog boris.krivonog@inova.si

Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
…when calculating diff

Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>
@cweitkamp
Copy link
Contributor

I created openhab/openhab-addons#14238

@cweitkamp
Copy link
Contributor

@ghys May I ask you an additional question? What does happen if you call the PersistenceExtensions.minimumBetween() or PersistenceExtensions.maximumBetween() methods from rules with parameters begin > end? Do you see the IAE too?

@ghys
Copy link
Member

ghys commented Jan 19, 2023

@cweitkamp it seems so:

logInfo('interval', FGWP102WallPlug_ElectricmeterkWh.minimumBetween(now, now.minusDays(30)).getState().toString())
00:30:38.894 [ERROR] [.internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test_rrd4j_interval' failed: Invalid start/end time in fetch request: 1674171038 > 1671579038
logInfo('interval', FGWP102WallPlug_ElectricmeterkWh.minimumBetween(now.minusDays(30), now).getState().toString())
00:31:40.941 [INFO ] [rg.openhab.core.model.script.interval] - 525.25 kWh

@ghys
Copy link
Member

ghys commented Jan 20, 2023

Closing as superseded by #1649. The discussion may continue here or be moved to #1649.

@ghys ghys closed this Jan 20, 2023
@cweitkamp
Copy link
Contributor

@ghys Thanks for testing. I will take care of that and add checks in OHC PersistenceExtension.

The fix for RRD4J has been merged. It takes care of both issues (start > end) and additional boundary value after end of requested time range.

@ghys
Copy link
Member

ghys commented Jan 21, 2023

Excellent, then let's merge #1649.

@m-jarzebowski
Copy link

Shouldn't the fix be backported to 3.4.x?

@crnjan crnjan deleted the feature/fix_diff_last_calc branch February 13, 2023 13:57
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.

Aggregate using diff_last will leave a gap at first entry
4 participants