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

feat: add support for resolution mapping #16

Merged
merged 4 commits into from
Dec 14, 2021

Conversation

NorbertNader
Copy link
Contributor

@NorbertNader NorbertNader commented Dec 3, 2021

Overview

This will allow customers to specify they want to fetch aggrgated data and the resolution mapping for the aggregated data.

The resolutionMapping maps the requested viewport date range to a specific resolution for the aggregation function.
The key is a number represented in milliseconds for the viewport duration. The value is the desired resolution that can be any string or number.

The reason the resolution mapping is flexible when it comes to the resolution is because different data sources might have different api signatures for the resolution. E.g. SiteWise uses '1h|1m|1d' as valid resolutions. Another api might want to use number representing milliseconds or nanoseconds. It is up to the data source to implement how to determine the resolution based on this mapping.

Next steps

Add support for multiple aggregation types in the cache. This will allow subscriptions made to the same data source to not have the cache cleared/overwritten leading to erroneous data e.g. If we have two subscriptions made to the same data source one for AVERAGE and the other for COUNT, we do not want them to affect each other and have them cached separately.

Tests

https://github.com/awslabs/iot-app-kit/runs/4434958580?check_suite_focus=true

Legal

This project is available under the Apache 2.0 License.

@NorbertNader NorbertNader self-assigned this Dec 3, 2021
@NorbertNader NorbertNader added the enhancement New feature or request label Dec 3, 2021
@NorbertNader NorbertNader force-pushed the support-aggregate-type-and-resolution-mapping branch from 5ca03fb to 1ed18a0 Compare December 3, 2021 01:34
@diehbria
Copy link
Contributor

diehbria commented Dec 3, 2021

if a user requests some data that is aggregated by average, and then then updated to aggregate by count, would the data-module provide the previously cached data requested under 'average' to the subscriber looking for count?

@NorbertNader
Copy link
Contributor Author

if a user requests some data that is aggregated by average, and then then updated to aggregate by count, would the data-module provide the previously cached data requested under 'average' to the subscriber looking for count?

Yes, that is exactly what would happen. Good callout, thanks for bringing this up! I think we’ll want to address this. There are two ways we can approach this that I can think of:

  1. When the subscription is updated with a different aggregation type, we clear the cache. This is the fast solution and probably what I would recommend at this time.
  2. A more long term solution would be to support multiple aggregation types in the cache. I think that is something we will want to do but out of the scope of this PR.

@gareth-amazon
Copy link
Contributor

  1. A more long term solution would be to support multiple aggregation types in the cache. I think that is something we will want to do but out of the scope of this PR.

I would just do that in the next PR. What is #1 going to buy us in the short term?

@NorbertNader NorbertNader force-pushed the support-aggregate-type-and-resolution-mapping branch from 1ed18a0 to f3a7f55 Compare December 4, 2021 17:42
@NorbertNader
Copy link
Contributor Author

NorbertNader commented Dec 4, 2021

  1. A more long term solution would be to support multiple aggregation types in the cache. I think that is something we will want to do but out of the scope of this PR.

I would just do that in the next PR. What is #1 going to buy us in the short term?

Solution 1. would allow us to move forward but has other concerns we I discussed offline with @diehbria . If we have two subscriptions one to AVERAGE and then add another one for COUNT, that would clear the cache for the first subscription leading to erroneous data since they would both be getting COUNT while the first subscription is expecting AVERAGE.

That is why instead we are dropping the aggregateType completely for now since that is something we would have to tackle first. I have updated the overview with the next steps highlighting this issue. For now my proposal is to go with a fetchAggregatedData boolean and the resolutionMapping

@NorbertNader NorbertNader force-pushed the support-aggregate-type-and-resolution-mapping branch from f3a7f55 to 4504b5f Compare December 4, 2021 17:58
@NorbertNader NorbertNader changed the title feat: add support for resolution mapping and aggregate type feat: add support for resolution mapping Dec 6, 2021
@NorbertNader NorbertNader force-pushed the support-aggregate-type-and-resolution-mapping branch from 4504b5f to 5f355ac Compare December 6, 2021 19:10
This will allow customers to specify they want to fetch aggrgated data and the resolution mapping for the aggregated data.

The resolutionMapping maps the requested viewport date range to a specific resolution for the aggregation function.
The key is a number represented in milliseconds for the viewport duration. The value is the desired resolution that can be any string or number.

The reason the resolution mapping is flexible when it comes to the resolution is because different data sources might have different api signatures for the resolution. E.g. SiteWise uses '1h|1m|1d' as valid resolutions. Another api might want to use number representing milliseconds or nanoseconds. It is up to the data source to implement how to determine the resolution based on this mapping.

**Next steps**

Add support for multiple aggregation types in the cache. This will allow subscriptions made to the same data source to not have the cache cleared/overwritten leading to erroneous data e.g. If we have two subscriptions made to the same data source one for AVERAGE and the other for COUNT, we do not want them to affect each other and have them cached separately.
@NorbertNader NorbertNader force-pushed the support-aggregate-type-and-resolution-mapping branch from 5f355ac to 4e07fe5 Compare December 9, 2021 07:26
@NorbertNader NorbertNader merged commit 1dad2b6 into main Dec 14, 2021
@NorbertNader NorbertNader deleted the support-aggregate-type-and-resolution-mapping branch January 14, 2022 22:47
sheilaXu added a commit that referenced this pull request Sep 23, 2022
* initial copy

* remove build-storybook from build script

* fix snapshot

* fix lint

* fix build with node v16

* fix test and lint

* fix transform controls getting reset

Co-authored-by: Xinyi Xu <xinyxu@amazon.com>
TheEvilDev pushed a commit that referenced this pull request Nov 2, 2022
* feat: add support for resolution mapping

This will allow customers to specify they want to fetch aggrgated data and the resolution mapping for the aggregated data.

The resolutionMapping maps the requested viewport date range to a specific resolution for the aggregation function.
The key is a number represented in milliseconds for the viewport duration. The value is the desired resolution that can be any string or number.

The reason the resolution mapping is flexible when it comes to the resolution is because different data sources might have different api signatures for the resolution. E.g. SiteWise uses '1h|1m|1d' as valid resolutions. Another api might want to use number representing milliseconds or nanoseconds. It is up to the data source to implement how to determine the resolution based on this mapping.

**Next steps**

Add support for multiple aggregation types in the cache. This will allow subscriptions made to the same data source to not have the cache cleared/overwritten leading to erroneous data e.g. If we have two subscriptions made to the same data source one for AVERAGE and the other for COUNT, we do not want them to affect each other and have them cached separately.

* Update testing-ground.tsx

Co-authored-by: Norbert Nader <nnader@amazon.com>
This was referenced Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants