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 new bot: time filter #1969

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

mariuskarotkis
Copy link
Contributor

Add new bot: time filter

@ghost ghost added component: bots feature Indicates new feature requests or new features labels Jun 7, 2021
@ghost
Copy link

ghost commented Jun 7, 2021

We already have the "filter" expert which can filter on (non-)equality, non-existence and regular expressions (match and non-match). We also have the sieve expert with filtering capabilities on all of the above plus existence, substrings, numeric comparisons, network ranges, set/list operations and all with typing support.

I'm reflecting what is the best user experience

  1. having a dedicated bot for filtering only on time (con: causes the bot-list to grow with yet-another filter expert), or
  2. expanding the existing filter expert to time-filtering (see also Filter bot  #569), (con: makes the configuration of the bot more complex), and/or
  3. expanding the sieve expert to time filtering

1 and 2 contradict each other, while 3 is orthogonal. I'm currently tending towards 1 (which is, what this PR does), as 2 means a more complex configuration of the bot - but I want to sleep over it first and hear others' opinions. And 3, our swiss-army knife, would be cool as well =)

@ghost ghost self-requested a review June 9, 2021 19:07
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think that a dedicated time filtering bot would be the best solution, but we have to think twice about which features this bot should have, to make it not too complicated to configure.

intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Show resolved Hide resolved
@ghost
Copy link

ghost commented Jun 14, 2021

I not sure yet what the purpose of this bot is due to missing docs, however I want to point out named queued ("paths") in IntelMQ. Using them could extend the functionality of the bot, by keeping the configuration simple. Thay are used in the "normal" filter expert like this: https://intelmq.readthedocs.io/en/latest/user/bots.html#filter (section Possible paths) and https://github.com/certtools/intelmq/blob/develop/intelmq/bots/experts/filter/expert.py#L105-L143

For example, with a single parameter (timespan), multiple use-cases could be solved, all used in parallel by the IntelMQ user

  • path older_than receives all data matching event < now()-timespan
  • path newer_than receives all data matching event > now()-timespan
  • path older_than_future receives all data matching event < now()+timespan
  • path newer_than_future receives all data matching event > now()+timespan

or similar.

@ghost
Copy link

ghost commented Aug 20, 2021

@mariuskarotkis The tests (or other Actions) of all your remaining PRs are failing currently. Could you please have a look at them? Please also apply the lessons learnt from the review of the merged PRs - if applicable to the new ones. If a PR is ready for review from your side, please give us a short ping.

@mariuskarotkis
Copy link
Contributor Author

Please check tests.

@ghost
Copy link

ghost commented Aug 20, 2021

The pycodestyle check fails with

intelmq/lib/utils.py:925:13: E701 multiple statements on one line (colon)

For the nosetests, you introduced a new testing requirement freeze_time. But that does not exist: https://pypi.org/project/freeze_time/ and causes the check to fail. Maybe you meant freezegun? However, we already use time-machine is a similar test case: https://github.com/certtools/intelmq/blob/develop/intelmq/tests/bots/experts/aggregate/test_expert.py We evaluated various libraries for simulating the time and that was the best matching result. It would also support decorators: https://github.com/adamchainz/time-machine#readme I'd prefer if we can stick to one time travelling library to keep the maintenance low - if possible.

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2021

Codecov Report

Merging #1969 (8c72554) into develop (1a7a339) will decrease coverage by 0.57%.
The diff coverage is 86.66%.

❗ Current head 8c72554 differs from pull request most recent head e7427bd. Consider uploading reports for the commit e7427bd to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #1969      +/-   ##
===========================================
- Coverage    76.91%   76.35%   -0.57%     
===========================================
  Files          454      456       +2     
  Lines        24120    24052      -68     
  Branches      3516     3793     +277     
===========================================
- Hits         18553    18366     -187     
- Misses        4793     4938     +145     
+ Partials       774      748      -26     
Impacted Files Coverage Δ
intelmq/bots/experts/time_filter/expert.py 79.41% <79.41%> (ø)
...elmq/tests/bots/experts/time_filter/test_expert.py 92.50% <92.50%> (ø)
intelmq/lib/utils.py 72.36% <100.00%> (-0.78%) ⬇️
intelmq/bin/intelmqdump.py 18.30% <0.00%> (-24.56%) ⬇️
intelmq/bin/intelmqctl.py 9.18% <0.00%> (-4.34%) ⬇️
intelmq/lib/processmanager.py 23.99% <0.00%> (-3.82%) ⬇️
intelmq/bots/experts/reverse_dns/expert.py 83.87% <0.00%> (-0.26%) ⬇️
intelmq/tests/lib/test_utils.py 99.48% <0.00%> (-0.04%) ⬇️
intelmq/tests/bin/test_intelmqdump.py 100.00% <0.00%> (ø)
... and 5 more

@mariuskarotkis mariuskarotkis requested a review from a user August 26, 2021 09:10
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

the docs need much more details on how the bot works and what it does. it's really hard to get an understanding of the bot, even when studying the code in detail

docs/user/bots.rst Outdated Show resolved Hide resolved
docs/user/bots.rst Outdated Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Show resolved Hide resolved
intelmq/bots/experts/time_filter/expert.py Outdated Show resolved Hide resolved
intelmq/lib/utils.py Show resolved Hide resolved
intelmq/lib/utils.py Outdated Show resolved Hide resolved
@mariuskarotkis mariuskarotkis requested a review from a user August 27, 2021 10:29
@ghost
Copy link

ghost commented Aug 27, 2021

When I read the docs of this new bot I don't understand the difference to the existing filter expert using not_before = 2 days (the value of the timespan parameter here). Can you please elaborate on docs more, so that the advantage / difference to the filter expert is recognizable?

@ghost ghost removed their request for review September 7, 2021 06:02
@mariuskarotkis mariuskarotkis requested a review from a user October 26, 2022 12:54
@mariuskarotkis mariuskarotkis marked this pull request as draft November 14, 2022 13:05
@mariuskarotkis mariuskarotkis marked this pull request as ready for review November 14, 2022 13:05
@mariuskarotkis
Copy link
Contributor Author

@sebix Please check reviewer is active or no.

@sebix
Copy link
Member

sebix commented Jan 20, 2023

Can you answer this question in the docs?

When I read the docs of this new bot I don't understand the difference to the existing filter expert using not_before = 2 days (the value of the timespan parameter here). Can you please elaborate on docs more, so that the advantage / difference to the filter expert is recognizable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: bots feature Indicates new feature requests or new features needs: feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants