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

[change] Reduced network calls in device metric flow #501 #516

Merged
merged 8 commits into from
Jun 3, 2023

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented May 19, 2023

Closes #501

Checks:

  • I have manually tested the proposed changes
  • I have written new test cases to avoid regressions (if necessary)
  • I have updated the documentation (e.g. README.rst)

@pandafy
Copy link
Member Author

pandafy commented May 23, 2023

The CI is failing due to a mismatch in formatting of timestamp. In the current implementation (master branch), the datetime object gets serialized when passed to celery. Due to this, the time argument of post_metric_write signal is a string. When using UDP for writing timeseries database, the datetime object is not serialized (because the task is executed synchonously) and the formatting of timestamp differs.

Moreover, if we pass a timestamp string to Metric.write method, the post_metric_write signal will use the exact string for time argument which leads to inconsistent results.

I think we should format all incoming timestamps to the ISO format in Metric.write which also includes the timezone information.

@pandafy pandafy force-pushed the issues/501-reduce-network-calls branch 2 times, most recently from 7d13bc1 to da66d98 Compare May 29, 2023 11:03
@pandafy pandafy marked this pull request as ready for review May 29, 2023 11:47
@pandafy pandafy force-pushed the issues/501-reduce-network-calls branch from da66d98 to 2a8f683 Compare May 30, 2023 05:53
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

This looks on the right track to me and I do not see any evident flaw in the code.

I do see 2 areas which I think we can improve: fetching metrics and charts objects now is done each time, both when reading and writing. Charts and metric objects are not written often, I believe they're written only once when they are created. From time to time another metric or chart may be written when new data comes in, therefore I think caching the DB calls to fetch these objects will help considerably to speed up execution time, both for writing and reading metrics, and is worth the bit of extra effort, can you proceed with this?

openwisp_monitoring/device/api/views.py Show resolved Hide resolved
openwisp_monitoring/device/api/views.py Outdated Show resolved Hide resolved
openwisp_monitoring/monitoring/base/models.py Outdated Show resolved Hide resolved
@pandafy pandafy force-pushed the issues/501-reduce-network-calls branch from 2a8f683 to 2f6807f Compare June 1, 2023 12:13
@pandafy pandafy force-pushed the issues/501-reduce-network-calls branch from 2f6807f to 25e896d Compare June 1, 2023 12:20
@nemesifier nemesifier merged commit ef349ba into master Jun 3, 2023
17 checks passed
@nemesifier nemesifier deleted the issues/501-reduce-network-calls branch June 3, 2023 17:47
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.

[change] Reduce network calls in device metric flow
2 participants