Skip to content

ESS-2234: Add wrapper for new endpoint for fetching aggregated timeseries#120

Merged
branislav-jenco-4ss merged 18 commits intomasterfrom
ESS-2234-support-new-aggregate-samples-endpoint-in-the-drio-python-library
Jan 12, 2024
Merged

ESS-2234: Add wrapper for new endpoint for fetching aggregated timeseries#120
branislav-jenco-4ss merged 18 commits intomasterfrom
ESS-2234-support-new-aggregate-samples-endpoint-in-the-drio-python-library

Conversation

@branislav-jenco-4ss
Copy link
Copy Markdown
Contributor

@branislav-jenco-4ss branislav-jenco-4ss commented Jan 5, 2024

This PR is related to user story [ESS-2234]

Description

This PR adds a new method get_samples_aggregate the client which is a wrapper to our new endpoint that supports simplified fetching of aggregated timeseries. It takes four required arguments: the start and end date, as anything which can be parsed by pandas.to_datetime, an aggregation period and an aggregation function, as well as optionally the max page size used when paging the response. The same limitations as in the API apply. As of now, there is no caching implemented for this method.

It's sort of a first draft, and in the implementation I made a couple choices that we might discuss:

  • no caching is being done
  • output of the method is a Pandas series, just like the old get method
  • the start and end parameters are allowed to be anything that pandas.to_datetime() can parse, just like the old get method
  • max_page_size is a parameter but maybe it's not necessary
  • we do retries on every page, but I haven't implemented a unit test for it

@branislav-jenco-4ss branislav-jenco-4ss added the enhancement New feature or request label Jan 5, 2024
Copy link
Copy Markdown
Contributor

@heidi-holm-4ss heidi-holm-4ss left a comment

Choose a reason for hiding this comment

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

Do we want to include a raise_empty flag similar to the old method? We should also update the documentation with a practical example.

Comment thread datareservoirio/client.py
Comment thread datareservoirio/client.py Outdated
Comment thread datareservoirio/client.py
Comment thread datareservoirio/client.py
Comment thread datareservoirio/client.py
Comment thread datareservoirio/client.py
Comment thread datareservoirio/client.py Outdated
Comment thread datareservoirio/client.py Outdated
Comment thread datareservoirio/client.py Outdated
@branislav-jenco-4ss
Copy link
Copy Markdown
Contributor Author

branislav-jenco-4ss commented Jan 11, 2024

note to self:

  • documentation

Comment thread datareservoirio/client.py
stop=stop_after_attempt(
4
), # Attempt!, not retry attempt. Attempt 2, is 1 retry
retry=retry_if_exception_type(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the future, we could, if it becomes an issue, retry using the indications from the server on how long to wait. I am not saying we should do it now, but it is an hook we could look at if this becomes an issue
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Let's keep it in mind.

Comment thread integration_tests/test_aggregate_samples.py
@branislav-jenco-4ss branislav-jenco-4ss merged commit cf2ee4a into master Jan 12, 2024
@branislav-jenco-4ss branislav-jenco-4ss deleted the ESS-2234-support-new-aggregate-samples-endpoint-in-the-drio-python-library branch January 12, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

4 participants