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

add python changefinder collector #10672

Merged
merged 21 commits into from
Apr 28, 2021

Conversation

andrewm4894
Copy link
Contributor

Summary
  • adds python 'changefinder' based collector for online changepoint detection.
Component Name

collectors/python.d/changefinder

Test Plan

Tested locally on development nodes running for 24-48 hours to find useful examples of changefinder finding actually useful changes and validate runtime performance characteristics.

Additional Information

Much more detail in the README of the collector.

@andrewm4894
Copy link
Contributor Author

adding image here to link to in readme

image

@github-actions github-actions bot added area/build Build system (autotools and cmake). area/collectors Everything related to data collection area/docs area/external/python area/web labels Feb 22, 2021
@lgtm-com
Copy link

lgtm-com bot commented Feb 22, 2021

This pull request introduces 1 alert when merging 0625002 into e1f5a8d - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@netdata-community-bot
Copy link

This pull request has been mentioned on Netdata Community. There might be relevant details there:

https://community.netdata.cloud/t/changefinder-collector-feedback/972/1

@ilyam8
Copy link
Member

ilyam8 commented Mar 5, 2021

@andrewm4894

I have a feeling that every collector you add does api/v1/allmetrics?format=json query, which is quite heavy.

Is there a chance that it could be one omega ML thing with different components (changefinder, etc.)? So we do only one query 🤔 Are there any other problems with that approach?

@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2021

This pull request introduces 1 alert when merging 44d31e7 into 790af96 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2021

This pull request introduces 1 alert when merging d8cd5c7 into 0a47bbd - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@andrewm4894
Copy link
Contributor Author

I have a feeling that every collector you add does api/v1/allmetrics?format=json query, which is quite heavy.

My understanding from talking to @mfundul a while back was the /allmetrics does not quite go all the way to the database since its sort of cached somewhere and so a lot cheaper than doing a /data type call for each chart.

But yes - getting the data into python is probably the biggest challenge with all of this. Interesting enough i think having an efficient and portable/flexible sort of storage that would give more options is part of the reasoning behind influx iox would be cool if there was a way for me to just go and grab the data from arrow and not have to go near the rest api and query engine. Or some sort of python package that could interface more efficiently with the raw data.

I was also thinking of some sort of ml redis queue or something where 'hot' data (in terms of likely to be needed by some ml stuff) could be sent and gotten at quickly if/as needed. But again i'm kind of at the edge of my comfort zone in if such ideas make sense or not.

For now trying to squeeze as much as possible out of the REST API as its all i can do.

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2021

This pull request introduces 1 alert when merging 3c59422 into a96fec9 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Mar 23, 2021

This pull request introduces 1 alert when merging 8b1df29 into 7827c6f - view on LGTM.com

new alerts:

  • 1 for Syntax error

- adds python 'changefinder' based collector for online changepoint detection.
- add a step to remove any chart dimensions not found in `data`. Idea being to maintain as accurate a list of chart dimensions and stop it from growing massive with lots of stale dimensions.
@andrewm4894 andrewm4894 force-pushed the changefinder-python-collector branch from 7237a9b to 084047e Compare April 22, 2021 14:15
@andrewm4894
Copy link
Contributor Author

@ilyam8 @thiagoftsm hey - i was hoping to try get this reviewed this week or next if possible.

ilyam8
ilyam8 previously approved these changes Apr 28, 2021
Co-authored-by: Vladimir Kobal <vlad@prokk.net>
Co-authored-by: Vladimir Kobal <vlad@prokk.net>
vlvkobal
vlvkobal previously approved these changes Apr 28, 2021
@andrewm4894 andrewm4894 requested a review from ilyam8 April 28, 2021 11:40
@ilyam8
Copy link
Member

ilyam8 commented Apr 28, 2021

Approving under the assumption that

Tested locally on development nodes running

was done with latest changes

ilyam8
ilyam8 previously approved these changes Apr 28, 2021
@andrewm4894
Copy link
Contributor Author

Approving under the assumption that

Tested locally on development nodes running

was done with latest changes

going to spin up a vm with latest changes and run that for a few hours to be sure.

…t set.

similar to how we do in the alarms collector.
@andrewm4894 andrewm4894 dismissed stale reviews from ilyam8 and vlvkobal via af0ff52 April 28, 2021 12:45
@andrewm4894
Copy link
Contributor Author

i was missing a small check in validate_charts() that meant the initial check on the collector was failing. fixed now so is similar to what we do in the alarms collector.

…or this collector

so as to collect it in one place.
@andrewm4894
Copy link
Contributor Author

just retesting - will re-request review once ready.

@andrewm4894
Copy link
Contributor Author

andrewm4894 commented Apr 28, 2021

working now, my apologies.

@andrewm4894 andrewm4894 merged commit 6ec39d2 into netdata:master Apr 28, 2021
@andrewm4894 andrewm4894 deleted the changefinder-python-collector branch April 28, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build system (autotools and cmake). area/collectors Everything related to data collection area/docs area/web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants