-
Notifications
You must be signed in to change notification settings - Fork 296
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
ENH: Add ability to copy more fields from the report #2513
ENH: Add ability to copy more fields from the report #2513
Conversation
Sometimes collectors sets more context information, e.g. the source file name. Currently, parsers usually remove those non-standard information. With this change, we let the user decide to copy more information to the event. In addition, the currently copied fields were documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced by the parameter name. Custom fields sound very generic and is ambiguous, also it lacks any mention of report or event.
What about replicate_report_fields(_to_event)
? It's longer, but it speaks by itself.
intelmq/lib/bot.py
Outdated
if self.copy_custom_fields: | ||
for key in self.copy_custom_fields: | ||
if key not in report: | ||
continue | ||
for event in events: | ||
event.add(key, report.get(key), overwrite=False) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this code is in intelmq.lib.bot.ParserBot.process
, it will not be active in all parsers as the above documentation suggests.
Adding it to intelmq.lib.message.Report.__init__
(and passing the parameter in intelmq.lib.bot.Bot.new_event
will cover all cases AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you mean some parsers do not use the process
? Then they do not use the default_fields
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process
can be overwritten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, it could be, but I didn't know it's a broad practice - I've looked at the source code, and you're right, a lot of parsers rely on overriding the whole process
. I'll move the code to the new_event
, and I think we should consider moving support of default_fields
there as well as this is something I'd expect all parsers to support (but it's a separated thing).
@sebix I'm open to name changes, but hmm, I'm not convinced with your suggestion either. Actually, my original idea was What would you say about:
...or any other? I think that the last sounds currently the best |
Some remarks by @th-certbund:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, I read it and it's understandable.
thank you @kamil-certat !! |
Sometimes collectors sets more context information, e.g. the source file name. Currently, parsers usually remove those non-standard information. With this change, we let the user decide to copy more information to the event.
In addition, the currently copied fields were documented.
Real use case: the HTTP collector sets filename when extracting data from an archive. This filename contains an information we want to include in the final event, which we cannot retrieve in any other way.