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

NOISSUE - Update timescale reader #2085

Merged
merged 24 commits into from
Mar 5, 2024
Merged

Conversation

Musilah
Copy link
Contributor

@Musilah Musilah commented Feb 20, 2024

What type of PR is this?

This a feature because it updates the Timescale reader.

What does this do?

This PR updates the Timescale Reader to be able to use aggregation queries for timeseries data to be used in dashboards charts.

Which issue(s) does this PR fix/relate to?

NOISSUE on Magistrala

Have you included tests for your changes?

No

Did you document any new/modified feature?

No

Notes

@Musilah Musilah marked this pull request as ready for review February 23, 2024 09:15
docker/docker-compose.yml Outdated Show resolved Hide resolved
pkg/sdk/go/message.go Outdated Show resolved Hide resolved
@ianmuchyri
Copy link
Contributor

{
    "offset": 0,
    "limit": 10,
    "format": "messages",
    "aggregation": "SUM",
    "interval": "1 minute",
    "total": 100,
    "messages": [
        {
            "time": 1708687680,
            "value": 18474
        },
        {
            "time": 1708687620,
            "value": 27894
        },
        {
            "time": 1708687560,
            "value": 3737
        }
    ]
}

The total count is not correct

@ianmuchyri
Copy link
Contributor

ianmuchyri commented Feb 23, 2024

Also for me to query for messages with the current implementation, I have to include aggregation and interval or it will not work. However, they are supposed to be conditional filters and the querying should work even without having to add them as filter parameters. Check on how to solve this.

image

pkg/sdk/go/message.go Outdated Show resolved Hide resolved
pkg/sdk/go/sdk.go Show resolved Hide resolved
pkg/sdk/go/sdk.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
docker/.env Outdated Show resolved Hide resolved
pkg/sdk/go/sdk.go Show resolved Hide resolved
pkg/sdk/go/sdk.go Outdated Show resolved Hide resolved
readers/messages.go Outdated Show resolved Hide resolved
readers/api/transport.go Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Show resolved Hide resolved
@Musilah Musilah force-pushed the readersupdate branch 4 times, most recently from 634c58e to bc94de8 Compare February 27, 2024 10:31
pkg/sdk/go/sdk.go Show resolved Hide resolved
readers/api/transport.go Show resolved Hide resolved
readers/timescale/messages_test.go Outdated Show resolved Hide resolved
readers/timescale/messages_test.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
ianmuchyri
ianmuchyri previously approved these changes Feb 28, 2024
docker/.env Outdated Show resolved Hide resolved
docker/.env Outdated Show resolved Hide resolved
readers/api/transport.go Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
pkg/sdk/go/message.go Outdated Show resolved Hide resolved
readers/api/transport.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
Copy link
Contributor

@felixgateru felixgateru left a comment

Choose a reason for hiding this comment

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

The epoch time attached to an aggregation query such as MAX and MIN for a value is not the time in which the MAX or MIN value occured.
Screenshot from 2024-02-29 11-31-49
Which is different from when the value occured:
image
It makes sense for aggregation to do this but there is a concern that the reason you are doing aggregation is to not only get the value but also the exact time within your desired interval in which the max occured.

@Musilah Musilah force-pushed the readersupdate branch 2 times, most recently from 787822b to 50c288d Compare February 29, 2024 14:58
internal/apiutil/errors.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/api/requests.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
readers/timescale/messages.go Outdated Show resolved Hide resolved
internal/apiutil/errors.go Outdated Show resolved Hide resolved
Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

Also update api documentation to include the 2 filters

@Musilah Musilah force-pushed the readersupdate branch 2 times, most recently from 86fc9b9 to d984262 Compare March 1, 2024 12:03
Musilah added 20 commits March 5, 2024 07:39
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
Signed-off-by: Musilah <[email protected]>
@Musilah Musilah force-pushed the readersupdate branch 2 times, most recently from 735dbd9 to 1fad094 Compare March 5, 2024 04:47
Signed-off-by: Musilah <[email protected]>
- Add examples for aggregation, intervale, to and from parameters for the api docs
- refactor transport validation when we have aggregation i.e KISS
- simplify and rename SQL query

Signed-off-by: Rodney Osodo <[email protected]>
Signed-off-by: Rodney Osodo <[email protected]>
@dborovcanin dborovcanin merged commit 42d433a into absmach:main Mar 5, 2024
5 checks passed
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