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.

Sign up for GitHub

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

Jump to bottom

LOG-13046-agent-add-support-for-windows-event-logs #422

Merged
merged 19 commits into from
Feb 9, 2023
Merged

LOG-13046-agent-add-support-for-windows-event-logs #422

merged 19 commits into from
Feb 9, 2023

Conversation

dkhokhlov
Copy link
Contributor

@dkhokhlov dkhokhlov commented Sep 29, 2022 •
edited
Loading

  • create new Tailer source
  • spawn winevt-tailer as child process (https://github.com/logdna/winevt-tailer).
  • receive JSON log lines from the tailer stdout
  • send log lines to Mezmo using standard agent LineBuilder stream infra
  • parameterize the tailer source in agent configuration file using new params: log.tailer_cmd, log.tailer_args
    Testing:
  • check that entries from Windows Event logs (System, Application) are correctly sent, parsed and presented in Mezmo Web UI front-end
    Notes:
  • early tailer errors and stack traces go into agent log. any non-recoverable tailer process error is fatal - causes agent to exit (and windows service to restart).
  • packaging includes released winevt-tailer exe into agent MSI alone with the tailer configuration appended to agent config file (yaml).
  • install: previous existing conf is now gets saved with timestamp suffix in name, agent uses bundled config to be fully functional.
  • install: if ingestion key provided during MSI or Choco install then agent config get updated with it
  • documentation is WIP

LOG-13046
LOG-14553
LOG-14644

@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch from dece967 to 13cffe4 Compare September 30, 2022 16:46
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch from 13cffe4 to ea44fa6 Compare October 19, 2022 16:55
@dkhokhlov dkhokhlov mentioned this pull request Oct 20, 2022
Open
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch 4 times, most recently from 3c23c46 to d7fded6 Compare October 21, 2022 17:36
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch 9 times, most recently from aeaa174 to 82bd144 Compare November 9, 2022 22:41
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch 14 times, most recently from 812c224 to f4a1301 Compare December 14, 2022 06:21
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch from 349e761 to 5af9d6a Compare February 1, 2023 17:56
@dkhokhlov dkhokhlov mentioned this pull request Feb 4, 2023
Closed
@dkhokhlov
api crate
9450ab8
@dkhokhlov
win32job poc
c20b4f5 
# Conflicts:
#	Cargo.lock
#	common/fs/src/cache/dir_path.rs
@dkhokhlov
decode_parser
53e8eb3 
# Conflicts:
#	common/fs/src/cache/mod.rs
@dkhokhlov
test: stream_gets_some_logs
f62d065
@dkhokhlov
fix: lines terminated by '\r'
700e712
@dkhokhlov
fix: compiler error
84471c0
@dkhokhlov
api tailer config
ae858c5
@dkhokhlov
support for Tailer config mixin
57ba44a 
# Conflicts:
#	bin/src/main.rs
#	common/fs/src/cache/dir_path.rs
@dkhokhlov
test stream_gets_some_logs for linux
cf8e9dc
@dkhokhlov
fix build error
114bfd9
@dkhokhlov
fix: build warnings
4c0a723 
# Conflicts:
#	common/notify_stream/src/lib.rs
@dkhokhlov
included api tailer into unix target
4a2081c 
# Conflicts:
#	bin/src/main.rs
@dkhokhlov
linting
b9aea41
@dkhokhlov
packaging 1st drop
a2b98fc
@dkhokhlov
rename existing config if present
877fab9
@dkhokhlov
chore: upgrade to winevt-tailer v0.3.4
fde6108
@dkhokhlov
additional error handling in api tailer source process. early startup…
b25ea41 
… errors go into agent log. any non-recoverable tailer process error is fatal - causes agent to exist.
@dkhokhlov
reabase on master
b5fc31f
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch from 7327405 to b5fc31f Compare February 7, 2023 23:52
@dkhokhlov dkhokhlov force-pushed the LOG-13046-agent-add-support-for-windows-event-logs branch from 1261083 to 66e274f Compare February 8, 2023 06:48
@dkhokhlov dkhokhlov requested a review from gjanco February 8, 2023 06:57
james-t-bloom
james-t-bloom reviewed Feb 8, 2023
View reviewed changes
Copy link
Contributor

@james-t-bloom james-t-bloom left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

  • most of the issues I had were around understanding what was going on but those have been addressed -

my other ask I guess is this PR encompasses three seperate stories. I know they are related and two of them are really small but it did add a little confusion and overhead tracking down the three seperate stories. These could be split up.

api/Cargo.toml Show resolved Hide resolved
api/src/tailer/error.rs Show resolved Hide resolved
api/src/tailer/mod.rs Show resolved Hide resolved
bin/src/_main.rs
@@ -326,6 +329,39 @@ pub async fn _main(
false => None,
};

let tailer_source = match (config.log.tailer_cmd, config.log.tailer_args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I got kind of turned around with how this is set up - and I feel it requires too much domain knowledge. This could just be a misunderstanding on my part.

Instead of this being wrapped around two configs that are set in a novel way that is new to the agent (by a brand new outside library) but if we wrapped it around if these tailers are defined for a given platform? This is all basically dead code for linux/macos but there is no indication as such. If possible I'm normally against over commenting but a comment around here explaining this I think would be welcome.

Copy link
Contributor Author

@dkhokhlov dkhokhlov Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the source will be None if these params are not configured. meaning - the source is optional. by default it is not enabled on non-windows platforms (as only windows installation package comes with bundled agent config file that has these options enabled).
It is standard approach in agent, see line 512.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gunna approve I guess my one ask would be a comment saying where these are set, but i'm not gunna hold up the PR for that.

common/config/src/raw.rs Show resolved Hide resolved
api/src/tailer/mod.rs Show resolved Hide resolved
james-t-bloom
james-t-bloom approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@james-t-bloom james-t-bloom left a comment

Choose a reason for hiding this comment

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

lgtm one nit

@dkhokhlov dkhokhlov merged commit da31c1a into master Feb 9, 2023
@dkhokhlov dkhokhlov deleted the LOG-13046-agent-add-support-for-windows-event-logs branch February 9, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gjanco gjanco gjanco approved these changes

@james-t-bloom james-t-bloom james-t-bloom approved these changes

@stsantilena stsantilena Awaiting requested review from stsantilena

@c-nixon c-nixon Awaiting requested review from c-nixon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dkhokhlov @gjanco @james-t-bloom