Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

All monitors own their loggers and set monitorID field #2334

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

rmfitzpatrick
Copy link
Contributor

@rmfitzpatrick rmfitzpatrick commented Jul 19, 2022

These changes move from the global logrus standard logger to per-monitor instance loggers that also set their "monitorID" field to be able to better differentiate between monitor instances. This is required for future Splunk collector distribution functionality and to fix a limitation in the Smart Agent receiver where all instances of a particular monitor type are reported to use the same collector component logger despite collecting data from different entities (makes debugging difficult).

I believe that all subprocess monitors already set a monitorID field so this continues the pattern w/ native ones.

edit: there are also a number of import ordering changes that were automatically applied from goland.**

edit: Appears to be some deadlock/race condition scenarios in unrelated test that I've tried to address.

@rmfitzpatrick rmfitzpatrick requested review from keitwb, asuresh4 and a team as code owners July 19, 2022 20:36
@rmfitzpatrick rmfitzpatrick marked this pull request as draft July 19, 2022 20:36
@rmfitzpatrick
Copy link
Contributor Author

I think the integration test failure is unrelated and should be resolved by #2335

@rmfitzpatrick rmfitzpatrick marked this pull request as ready for review July 20, 2022 17:11
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM. I have reviewed the changes, they look ok to me.

Copy link
Contributor

@jvoravong jvoravong left a comment

Choose a reason for hiding this comment

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

Just one question/nit. LGTM. This will be useful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants