Bot option: Error Forward Message #1133

SYNchroACK · 2017-12-11T08:03:30Z

error_forward_message is an option for Expert bots. Bots like gethostbyname sometimes raise some errors which are only due some service problems like DNS nameserver for a specific domain is replying SERVFAIL. Therefore, with this new option, intelmq administrator can configure an expert bot to forward the original message to the next bot automatically after X retries configured as usual.

ghost · 2017-12-11T16:44:02Z

I changed the target branch to develop because this is a new feature. Have a look at https:/certtools/intelmq/blob/master/docs/Developers-Guide.md#branching-model maintenance only get's bugfixes and master is always the latest release (nothing between stable versions)
You may want to rebase to develop now.

Please add this to the changelog (in the 1.1 core section).

ghost · 2017-12-11T16:45:22Z

@aaronkaplan I added you because I'd like if the name of the parameter is intuitive. I am not sure if it is.

The relevant change is in 89574ff

SYNchroACK · 2017-12-12T00:48:52Z

You may want to rebase to develop now.

Thank you @wagner-certat for the clarification. I tried to rebase but with no luck. Could you provide the commands to perform that operation successfully? Thank you.

ghost · 2017-12-12T08:44:19Z

As the branch you branched off was very old too (at the stage of the 1.0.0 RC), cherry-picking is faster:

First update your local branch:

git remote update

If you have a develop branch locally, assuming that upstream is the remote pointing to certtools (depends on your local configuration):

git checkout develop
git pull upstream develop

If you don't have develop locally yet:

git checkout -b develop upstream/develop

Copy the old branch, delete the existing and make it a copy of develop:

git branch backup error_forward_message
git branch -D error_forward_message
git checkout -b error_forward_message develop

And now pick the commit to the new branch:

git cherry-pick 89574ff

You will get a conflict in intelmq/etc/defaults.conf. Fix that and do

git add intelmq/etc/defaults.conf
git cherry-pick --continue

Now upload that branch:

git push -f origin error_forward_message

If everything went fine, you can delete the backup branch.

SYNchroACK · 2017-12-12T10:53:16Z

If everything went fine, you can delete the backup branch.

@wagner-certat Thank you! It works.

codecov-io · 2017-12-12T10:57:35Z

Codecov Report

Merging #1133 into develop will increase coverage by 0.01%.
The diff coverage is 80%.

@@             Coverage Diff             @@
##           develop    #1133      +/-   ##
===========================================
+ Coverage    75.81%   75.82%   +0.01%     
===========================================
  Files          231      231              
  Lines        10892    10909      +17     
  Branches      1447     1448       +1     
===========================================
+ Hits          8258     8272      +14     
- Misses        2306     2309       +3     
  Partials       328      328

Impacted Files	Coverage Δ
intelmq/tests/lib/test_bot.py	`100% <100%> (ø)`	⬆️
intelmq/lib/test.py	`93.64% <100%> (ø)`	⬆️
intelmq/lib/bot.py	`64.07% <73.33%> (+0.43%)`	⬆️

ghost · 2017-12-12T12:10:23Z

@wagner-certat Thank you! It works.

Great, looks good.

ghost

You must not forward bogus messages for parser and output bots. So it only applies to experts.

@aaronkaplan has some comments too

ghost · 2017-12-13T15:15:43Z

intelmq/lib/bot.py

@@ -213,6 +213,10 @@ def start(self, starting: bool=True, error_on_pipeline: bool=True,

 if error_on_message:

+ if self.parameters.error_forward_message:


For upgrades this parameter won't exist, you need to have a fallback (use getattr)

SYNchroACK · 2017-12-14T10:23:16Z

@wagner-certat the current travis error is regarding the self.__group which are not present in tests.py. Do you agree with the parameter self.__group? Makes sense to have it loaded? If yes, can you help me fix the tests.py, I'm not sure the best way to adapt the tests.

Thank you in advance.

ghost · 2017-12-18T13:56:27Z

@wagner-certat the current travis error is regarding the self.__group which are not present in tests.py. Do you agree with the parameter self.__group? Makes sense to have it loaded?

Currently it is a "parameter", i.e. user-defined. That does not make sense for the POV from intelmq itself, because it is defined by the bot type.

If yes, can you help me fix the tests.py, I'm not sure the best way to adapt the tests.

mocked_config needs to take an additional parameter for the group. The value is in self.bot_type.

…ror_forward_message

SYNchroACK · 2018-01-16T18:39:37Z

@sebix check if everything is ok please. Thank you! :)

ghost

Code is ok for me.

Still waiting for @aaronkaplan's comments

aaronkaplan

I am not sure if I think this is a good idea to implement it so generically. Why?
Because if you just want to skip let's say failed DNS queries, then you actually implemented it in a very generic way here.

But if it's implemented so generically, it will be abused.
And if someone configures this and is wondering why the data is not there for "downstream" bots, then ... it's hard to debug.

So... I am definitely for "if it breaks, break early and fix it specifically at that bot".
This suggestion makes intelMQ potentially harder to debug.

But... we are discussing in chat. Maybe it's possible to accommodate easily and intuitively

aaronkaplan · 2018-01-22T15:15:58Z

docs/User-Guide.md

@@ -109,6 +109,9 @@ You can set these parameters per bot as well. The settings will take effect afte
 * **`error_dump_message`** - specifies if the bot will write queued up messages to its dump file (use intelmqdump to re-insert the message).
 * **`true/false`** - write or not write message to the dump file

+* **`error_forward_message`** - specifies if the bot will forward messages to output queue which couldn't be processed by the bot. This option MUST be only used by Expert Bots.


non intuitive name.

how about:
event_always_passthrough .... pass through the event in any situation, even when errors exist in the expert.

aaronkaplan · 2018-01-22T15:17:20Z

intelmq/etc/defaults.conf

@@ -6,6 +6,7 @@
 "destination_pipeline_password": null,
 "destination_pipeline_port": 6379,
 "error_dump_message": true,
+ "error_forward_message": false,


see above.
We definitely need a comment here! This can have pretty bad effects if turned on.

aaronkaplan · 2018-01-22T15:17:59Z

intelmq/lib/bot.py

@@ -211,6 +217,10 @@ def start(self, starting: bool=True, error_on_pipeline: bool=True,

 if error_on_message:

+ if self.parameters.error_forward_message and self.group == "Expert":
+ self.logger.info("Forwarding message to output queue.")


self.logger.info ("Forcing forwarding of message even in case of errors")

aaronkaplan

Another comment: I believe it is much much better to add a 2> /tmp/errors and > /tmp/ok style output switch similar to nifi:

https://nifi.apache.org/docs/nifi-docs/components/org.apache.nifi/nifi-enrich-nar/1.5.0/org.apache.nifi.processors.GeoEnrichIP/index.html

So, basically , one queue for "ok" events, one queue for "not ok events" where the expert could not do anything. Now if the user clearly says that both output queues should be the same, then that's ok.
Then you have your behaviour

ghost · 2018-01-22T15:55:12Z

This could be a use case for #1088
The name for this special destination queue could be e.g. _on_error

SYNchroACK added component: configuration component: core nice to have labels Dec 11, 2017

ghost requested review from aaronkaplan and a user December 11, 2017 16:39

ghost self-assigned this Dec 11, 2017

ghost added this to the 1.1.0 milestone Dec 11, 2017

ghost changed the base branch from master to develop December 11, 2017 16:39

ENH: new bot option - error_forward_message

4cf7401

SYNchroACK force-pushed the error_forward_message branch from 89574ff to 4cf7401 Compare December 12, 2017 10:52

ghost suggested changes Dec 13, 2017

View reviewed changes

ENH: error_forward_message option fix and group parameter added

e4d778b

ghost modified the milestones: 1.1.0, 1.0.3 Jan 16, 2018

Sebastian Wagner and others added 5 commits January 16, 2018 16:08

ENH: lib/bot: expose top level parameters

9a7eb14

Merge branch 'develop' into SYNchroACK-error_forward_message

2adff1c

ENH: change self.__group to self.group

de5cfa0

MAINT: conflit solved

7cd60bb

Merge branch 'wagner-certat-SYNchroACK-error_forward_message' into er…

619415a

…ror_forward_message

ghost approved these changes Jan 17, 2018

View reviewed changes

ghost assigned aaronkaplan and unassigned ghost Jan 17, 2018

SYNchroACK added 2 commits January 18, 2018 16:18

Merge branch 'develop' into error_forward_message

c6f8552

Merge branch 'develop' into error_forward_message

4746ebc

aaronkaplan reviewed Jan 22, 2018

View reviewed changes

ghost mentioned this pull request Mar 6, 2018

Names queues #1088 #1190

Merged

ghost closed this in d572671 Mar 9, 2018

SYNchroACK deleted the error_forward_message branch August 30, 2018 22:48

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bot option: Error Forward Message #1133

Bot option: Error Forward Message #1133

SYNchroACK commented Dec 11, 2017

ghost commented Dec 11, 2017

ghost commented Dec 11, 2017

SYNchroACK commented Dec 12, 2017

ghost commented Dec 12, 2017

SYNchroACK commented Dec 12, 2017

codecov-io commented Dec 12, 2017 •

edited

Loading

ghost commented Dec 12, 2017

ghost left a comment •

edited by ghost

Loading

ghost Dec 13, 2017

SYNchroACK commented Dec 14, 2017 •

edited

Loading

ghost commented Dec 18, 2017

SYNchroACK commented Jan 16, 2018

ghost left a comment

aaronkaplan left a comment

aaronkaplan Jan 22, 2018

aaronkaplan Jan 22, 2018

aaronkaplan Jan 22, 2018

aaronkaplan left a comment

ghost commented Jan 22, 2018

		@@ -213,6 +213,10 @@ def start(self, starting: bool=True, error_on_pipeline: bool=True,

		if error_on_message:

		if self.parameters.error_forward_message:

Bot option: Error Forward Message #1133

Bot option: Error Forward Message #1133

Conversation

SYNchroACK commented Dec 11, 2017

ghost commented Dec 11, 2017

ghost commented Dec 11, 2017

SYNchroACK commented Dec 12, 2017

ghost commented Dec 12, 2017

SYNchroACK commented Dec 12, 2017

codecov-io commented Dec 12, 2017 • edited Loading

Codecov Report

ghost commented Dec 12, 2017

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost Dec 13, 2017

Choose a reason for hiding this comment

SYNchroACK commented Dec 14, 2017 • edited Loading

ghost commented Dec 18, 2017

SYNchroACK commented Jan 16, 2018

ghost left a comment

Choose a reason for hiding this comment

aaronkaplan left a comment

Choose a reason for hiding this comment

aaronkaplan Jan 22, 2018

Choose a reason for hiding this comment

aaronkaplan Jan 22, 2018

Choose a reason for hiding this comment

aaronkaplan Jan 22, 2018

Choose a reason for hiding this comment

aaronkaplan left a comment

Choose a reason for hiding this comment

ghost commented Jan 22, 2018

codecov-io commented Dec 12, 2017 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

SYNchroACK commented Dec 14, 2017 •

edited

Loading