Skip to content

[timeseries] Add initial support for elasticsearch #99#164

Open
nepython wants to merge 1 commit intomasterfrom
issues/99-add-initial-support-second-timeseries_db
Open

[timeseries] Add initial support for elasticsearch #99#164
nepython wants to merge 1 commit intomasterfrom
issues/99-add-initial-support-second-timeseries_db

Conversation

@nepython
Copy link
Copy Markdown
Contributor

@nepython nepython commented Jul 15, 2020

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)

Closes #99, #68

Need Help
There is one test (test_get_device_metrics_csv) failing on travis (it's probably caused by cardinality aggregation counting None from date_histogram as a value). I am unable to find a solution to this. In real case this should be rare as can be confirmed by the passing of test_wifi_hostapd.

PS: Query posted on https://discuss.elastic.co/t/cardinality-aggregation-always-returning-one-extra-count/243462

@nepython nepython marked this pull request as draft July 15, 2020 19:24
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch 3 times, most recently from be49865 to 184f7a6 Compare July 17, 2020 19:04
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch from 184f7a6 to d383f17 Compare July 21, 2020 16:45
@nepython nepython linked an issue Jul 21, 2020 that may be closed by this pull request
4 tasks
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch 2 times, most recently from 6205616 to de11205 Compare July 24, 2020 20:22
Copy link
Copy Markdown
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.

Good progress @nepython, see my comments below.

Comment thread README.rst
"GROUP BY time(1d)"
)
),
'elasticsearch': _make_query({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why don't you change the code so that calling this functon from the configuration is not necessary?
You can loop over the data structure and call it when it's initialized, this way we make things easy for users and we avoid them come to complain to us in the support channels 😂

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.

Yes that can be done and I have done the same for built-in charts. _make_query is just a utility function which will update the aggregation for a default_chart_query defined in openwisp_monitoring.db.backends.elasticsearch.queries. So queries returned via _make_query will always retain the structure of default_chart_query.

I wanted to leave the option of directly using a dsl query with timeseries_db.query, exactly like how we can query InfluxDB directly using the same function.

A full dsl query will look like this,

{'query': {'nested': {'path': 'tags',
   'query': {'bool': {'must': [{'match': {'tags.object_id': {'query': '9a39a5ae-146b-4a50-b113-f9381b8c1721'}}},
      {'match': {'tags.content_type': {'query': 'config.device'}}}]}}}},
 '_source': False,
 'size': 0,
 'aggs': {'GroupByTime': {'nested': {'path': 'points',
    'aggs': {'set_range': {'filter': {'range': {'points.time': {'from': 'now-1d/d',
         'to': 'now/d'}}},
      'aggs': {'time': {'date_histogram': {'field': 'points.time',
         'fixed_interval': '10m',
         'format': 'date_time_no_millis',
         'order': {'_key': 'desc'},
         'time_zone': 'Asia/Kolkata'},
        'aggs': {'nest': {'nested': {'path': 'points.fields',
           'aggs': {'CPU_load': {'avg': {'field': 'points.fields.cpu_usage'}}}}}}}}}}}}}}

So, if we make _make_query as a compulsion, we might be cutting down a user's freedom to query via DSL. Personally, I would like to give user this freedom (this would enable him to just put a query like above in chart configuration and it will work) 😄.

Comment thread openwisp_monitoring/db/backends/elasticsearch/index.py Outdated
Comment thread openwisp_monitoring/db/backends/elasticsearch/index.py Outdated
Comment thread openwisp_monitoring/db/backends/elasticsearch/index.py Outdated
Comment thread requirements.txt Outdated
Comment thread openwisp_monitoring/db/backends/influxdb/tests.py
Comment thread openwisp_monitoring/db/backends/elasticsearch/tests.py Outdated
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch 3 times, most recently from c789856 to e07bc63 Compare July 27, 2020 18:29
@nepython nepython linked an issue Jul 27, 2020 that may be closed by this pull request
3 tasks
Comment thread openwisp_monitoring/db/backends/elasticsearch/index.py Outdated
Comment thread openwisp_monitoring/db/backends/elasticsearch/index.py Outdated
Comment thread openwisp_monitoring/db/backends/elasticsearch/queries.py Outdated
Comment thread tests/openwisp2/settings.py Outdated
Comment thread tests/openwisp2/settings.py Outdated
Comment thread docker-compose.yml
INFLUXDB_USER: openwisp
INFLUXDB_USER_PASSWORD: openwisp
# clustered version of elasticsearch is used as that might be used in production
es01:
Copy link
Copy Markdown
Contributor

@PabloCastellano PabloCastellano Jul 28, 2020

Choose a reason for hiding this comment

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

We don't need to run Elasticsearch in a High Available environment. Testing HA capabilities is ElasticSearch's job 😃 . We can simply make sure that setting up a multi-nodes cluster works but IMHO it is enough for us to run tests in only one instance.

WDYT?

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.

I agree though there are some problems that I was facing with elasticsearch docker due to which too I am using two nodes 😅. Can you please check out if it's possible to run elasticsearch on a single port (I am not sure about this as I could not :/ ) and then I can adapt. Thanks!

@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch 5 times, most recently from 7bad1b0 to e98535a Compare July 29, 2020 17:32
@nepython nepython added enhancement New feature or request timeseries Issues / PRs / tasks related to timeseries database labels Jul 29, 2020
@nepython nepython marked this pull request as ready for review July 29, 2020 17:57
@nepython
Copy link
Copy Markdown
Contributor Author

Currently, things to do:~

  • Fix cardinality aggregation returning an extra value for wifi_clients causing test_get_device_metrics_csv to fail.
  • Elasticsearch tests are currently consuming ~120s to run. This can be reduced to 75s by retaining indices. There are other optimizations too which shall be implemented together in [timeseries] Optimize elasticsearch #168.

Copy link
Copy Markdown
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.

Currently, things to do:~

  • Fix cardinality aggregation returning an extra value for wifi_clients causing test_get_device_metrics_csv to fail.

Please resolve this asap.

  • Elasticsearch tests are currently consuming ~120s to run. This can be reduced to 75s by retaining indices. There are other optimizations too which shall be implemented together in #168.

We may do this in a second step. We make it work first, then we optimize.

Comment thread tests/openwisp2/settings.py Outdated
Comment thread tests/openwisp2/settings.py Outdated
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch from e98535a to 9c53057 Compare July 30, 2020 13:54
@nepython
Copy link
Copy Markdown
Contributor Author

Currently, things to do:~

  • Fix cardinality aggregation returning an extra value for wifi_clients causing test_get_device_metrics_csv to fail.

Please resolve this asap.

I need help, mentioned in opening comment 😬. I am still trying to resolve but couldn't find any fix, will question this on elasticsearch discussion forums if I am unable to in the next 1-2 days.

  • Elasticsearch tests are currently consuming ~120s to run. This can be reduced to 75s by retaining indices. There are other optimizations too which shall be implemented together in [timeseries] Optimize elasticsearch #168.

We may do this in a second step. We make it work first, then we optimize.

Ok

@nemesifier
Copy link
Copy Markdown
Member

Currently, things to do:~

  • Fix cardinality aggregation returning an extra value for wifi_clients causing test_get_device_metrics_csv to fail.

Please resolve this asap.

I need help, mentioned in opening comment . I am still trying to resolve but couldn't find any fix, will question this on elasticsearch discussion forums if I am unable to in the next 1-2 days.

I see that you mentioned it, can you please write a detailed explanation of what is going on and provide links to the external info you mention?

@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch 3 times, most recently from 9a80da6 to fa5759a Compare August 6, 2020 18:41
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch from fa5759a to 319ec3d Compare August 7, 2020 08:36
Base automatically changed from dev to master August 7, 2020 23:31
@nemesifier nemesifier changed the base branch from master to dev August 10, 2020 21:13
@nepython nepython force-pushed the issues/99-add-initial-support-second-timeseries_db branch 4 times, most recently from 93039cf to 91a648f Compare August 21, 2020 06:33
Copy link
Copy Markdown
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.

@nepython how's it going with fixing the build here?
Please focus on completing this ⚠️

@nepython
Copy link
Copy Markdown
Contributor Author

@nepython how's it going with fixing the build here?
Please focus on completing this ⚠️

@nemesisdesign, definitely I am working to fix the build, working mostly on https://github.com/nepython/openwisp-monitoring/tree/issues/99-add-initial-support-second-timeseries_db to try out minor changes but so far no good. I asked a query on S/O in the morning (due to no response on elasticsearch discussion forum 😶), it can be found at https://stackoverflow.com/questions/63590693/elasticsearch-cardinality-aggregation-returning-one-extra-count.

As far as I can visualize, the clients Chart sometimes returns an extra count and sometimes it doesn't. In the current travis build one test is failing due to this. Locally, that one test is always passing now (but it was failing earlier 5-6 days ago just don't know why it is passing now :/).
Screenshot from 2020-08-26 09-39-43

What I am trying to figure out right now is why sometimes the clients charts returns an extra count and correct count the other times. Is this even an issue on our end or on elasticsearch's (unsure)? I posted a detailed status on the PR and a request too to provide me with some JSON data or access to any instance in which the user is ready to experiment with elasticsearch (as you had mentioned in our last call) few days back on the IM channel https://gitter.im/openwisp/openwisp-monitoring?at=5f3f709f49148b41c9619185 and reminded the same personally but didn't receive any response 😓.

Right now if we are in a hurry to merge this PR, we can do two things:~

  1. Disable clients chart for elasticsearch and state it in README as Work in Progress.
  2. We can include the code related to clients chart in dev with a short note related to this problem of clients chart in a section say Known Issues and open up an issue for the same, while skipping that one test for now and adding a comment above it that this needs to be fixed.

This is a big add-on and something I have worked really hard upon so would really like to see this getting merged though even after a month I am not able to solve this one single issue (which is blocking everything else) :(. I am determined to fix this issue even after GSoC and see this module getting released and be helpful to others 😃.

Base automatically changed from dev to master August 31, 2020 01:21
@devkapilbansal devkapilbansal force-pushed the issues/99-add-initial-support-second-timeseries_db branch 2 times, most recently from 1a5f91b to 624987b Compare April 8, 2021 06:37
@devkapilbansal devkapilbansal force-pushed the issues/99-add-initial-support-second-timeseries_db branch from 624987b to 044a29f Compare April 8, 2021 06:47
@cbeaujoin-stellar
Copy link
Copy Markdown

cbeaujoin-stellar commented Mar 13, 2026

Proposal: Modernizing Elasticsearch Integration for OpenWISP Monitoring - GSoC 2026

To properly reopen the subject of Elasticsearch integration, we should align the implementation with modern Elastic Stack standards and ensure seamless deployment across the OpenWISP ecosystem.

The following requirements are proposed for the updated implementation:

  1. Versioning & Library Updates

Elasticsearch 9.x: The integration must target Elasticsearch version 9.x to ensure long-term compatibility.

Client Library: The elasticsearch Python client must be updated, and the codebase refactored to handle the API changes inherent in the 9.x series.

  1. Data Architecture: ECS & Data Streams

ECS Compliance: Data must follow the Elastic Common Schema (ECS) format. This is critical for leveraging Kibana’s native capabilities and ensuring interoperability with the wider Elastic ecosystem.

Bidirectional Mapping: To maintain compatibility with OpenWISP's internal logic and other time-series backends (like InfluxDB), a mapping layer must be implemented to translate between OpenWISP metrics and ECS fields bidirectionally.

Data Streams: We must move away from traditional indices in favor of Elasticsearch Data Streams. This modern approach better handles time-series data and simplifies rollover management.

  1. Deployment & Automation

OpenWISP Docker: Support for Elasticsearch must be integrated into the openwisp-docker project for both development and production-ready stacks.

Ansible Integration: * Elasticsearch deployment should be included as an optional component in ansible-openwisp2.
To follow best practices, we should leverage proven third-party collections (e.g., NETWAYS/ansible-collection-elasticstack) rather than maintaining custom deployment logic.

  1. Lifecycle & Configuration Management

Templates & ILM: Default Index Templates and Index Lifecycle Management (ILM) policies must be provided out-of-the-box.

Configurability: Users must be able to override these templates and policies via Django settings or Ansible variables to suit their specific retention and hardware requirements.

  1. Visualization (Bonus)

Kibana Dashboards: The contribution should include pre-configured Kibana dashboards and visualizations that leverage the ECS data to provide immediate insights into network health and performance.

@855princekumar
Copy link
Copy Markdown

@cbeaujoin-stellar Thanks for pointing me here earlier.

I’ve started going through this thread and the Elasticsearch PR (#164) to understand the current state better.

I also read through the proposed changes around ECS, data streams, and updated client versions. For now, I’m planning to begin by trying to get the existing implementation working again with the current codebase (rebasing, fixing tests, updating dependencies) so I have a clearer base to build on. Once I have that in a better state, I’ll try to move step by step into the improvements mentioned here.

If there’s any specific part in the current PR that would be a good starting point, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request timeseries Issues / PRs / tasks related to timeseries database

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[timeseries] Add initial support for second timeseries db [enhancement] Prepare travis-ci build

5 participants