-
Notifications
You must be signed in to change notification settings - Fork 245
check: add --format flag, json reporter
#663
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
base: main
Are you sure you want to change the base?
Conversation
c2cb8cc to
524059d
Compare
check/src/main.rs
Outdated
| return; | ||
| } | ||
|
|
||
| let events = row |
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.
Note that this will compress out None values; the daemon/src/analysis.rs AnalysisWriter will not perform that compression. We could push this log into that writer as well if desired.
524059d to
b46ae62
Compare
|
I'm pretty slammed with work right now, will follow up on this within the next week. |
|
so far it looks good to me. I am not worried about the use of |
7f8bdb1 to
b881cc5
Compare
|
This is ready for review; I haven't written rust in a few years and I'm unattached to this implementation so I'm happy to steer this change wherever needed. |
untitaker
left a comment
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.
first wave of review, i think the direction is good but if the format is supposed to match then we should be able to share code.
| writer: BufWriter<File>, | ||
| } | ||
|
|
||
| // The `njson` report has the same output format as the daemon analysis report. |
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.
can it be made so that the structs are then actually shared between check and daemon? okay to move things into lib/
particularly, the definition of OutputRow and all code that converts to that structure
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.
Extracted a DetectionRow in ea6e73e. At present the daemon doesn't perform the same row collapsing so I'll add that logic in a separate PR to separate that behavior change.
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'll need to take another pass at this since the first pass of using TryFrom doesn't seem ideal.
|
|
||
| [dependencies.simple_logger] | ||
| version = "5.0.0" | ||
| features = ["stderr"] |
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.
can you document here why it has to be stderr?
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.
It's no longer strictly necessary to log to stderr; my original implementation printed json to stdout for simplicity but since --path supports both files and directories we have to write .ndjson files in order to have a coherent output that matches the daemon ndjson output.
My instinct is to revert this change and continue logging to stdout so that check ... | less will work as expected, but I don't have strong feelings either way. I'll document why we made this change if you want to continue with the switch to stderr.
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 think logging to stderr is still the right thing to do, just asking about documentation
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.
Amended in b877ce4.
| fn try_from(ar: AnalysisRow) -> Result<DetectionRow, Self::Error> { | ||
| let events: Vec<Event> = ar.events.into_iter().filter_map(|e| e).collect(); | ||
|
|
||
| if events.is_empty() { |
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.
This will break skip reason generation. TryFrom might be a poor fit here because we need to be able to return an output row with n events or a skip reason and TryFrom doesn't make this configurable without passing a tuple, and at that point it's starting to abuse the TryFrom semantics.
| } | ||
| } | ||
|
|
||
| let det = DetectionRow::try_from(row).ok(); |
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.
Needs to handle skip reasons.
|
|
||
| // The `njson` report has the same output format as the daemon analysis report. | ||
| // See also: [Newline Delimited JSON](https://docs.mulesoft.com/dataweave/latest/dataweave-formats-ndjson) | ||
| impl NdjsonReport { |
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.
This could likely reuse the daemon AnalysisWriter struct. Since we're going to be refactoring to reuse components then extracting and reusing that struct will be worth the lift.
Pull Request Checklist
cargo fmt./checkbut I can add them if neededProblem
Rayhunter
checkoutput is emitted as plain text and doesn't lend itself to easy analysis by other tools.Solution
Implement a
--reportflag, preserve the existing behavior as the default (manually invoked with--report log), and add an ndjson formatter.Fixes #570.
Verification
Example output
1749319445-goodsim.ndjson
Considerations
The use of adyn Traitfeels a bit clumsy--reportnow writes a file so there's no issue of redirection writing extra logs.--pathflag can be either a file or directory so this may need to emit multiple json reports in one shot