Fix windows event logs to only starts once #851
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
okankoAMZ
force-pushed
the
okanko-dev
branch
7 times, most recently
from
0a0ee6e to
f019777
Compare
jefchien
reviewed
Sep 15, 2023
okankoAMZ
force-pushed
the
okanko-dev
branch
from
f019777 to
c074c04
Compare
jefchien
reviewed
Sep 15, 2023
| @@ -25,6 +27,8 @@ const ( | |||
| forcePullInterval = 250 * time.Millisecond | |||
| ) | |||
|
|
|||
| var startOnlyOnce sync.Once | |||
There was a problem hiding this comment.
nit: I think we might be able to move this into the Plugin struct since the plugin should only be initialized in the init function and should be the same instance wherever it's used.
bhanuba
previously approved these changes
Sep 15, 2023
okankoAMZ
force-pushed
the
okanko-dev
branch
2 times, most recently
from
ec830c6 to
c09405e
Compare
okankoAMZ
force-pushed
the
okanko-dev
branch
from
c09405e to
c9ffed8
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #851 +/- ##
==========================================
+ Coverage 57.58% 60.42% +2.83%
==========================================
Files 370 356 -14
Lines 17548 18116 +568
==========================================
+ Hits 10105 10946 +841
+ Misses 6848 6595 -253
+ Partials 595 575 -20
☔ View full report in Codecov by Sentry. |
jefchien
approved these changes
Sep 15, 2023
| Name: "System Event//Log::", | ||
| } | ||
| plugin.Events = append(plugin.Events, ec) | ||
| require.Equal(t, 0, len(plugin.newEvents), "Start should be ran only once so there should be only 1 new event") |
There was a problem hiding this comment.
nit: The error log would be misleading.
|
Please update the title to better reflect the change. Maybe something like "Fix windows event logs to only starts once" |
okankoAMZ
changed the title
bhanuba
approved these changes
Sep 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the issue
Currently, when the agent start in windows both log agent and telegraf start the windows log event plugin. As a result, the the plugins generates duplicate logs.
Description of changes
Changed the start function of the plugin to be exclusively ran once to avoid any duplicate plugin calls to be started.
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Created and ran a units test this can be seen in TestWindowsDuplicateStart
Also ran integration test: this is the instance where there is only logs
Requirements
Before commit the code, please do the following steps.
make fmtandmake fmt-shmake lint