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.

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

stop monitoring excluded paths #21282

Merged
merged 5 commits into from
Nov 9, 2020
Merged

Conversation

newly12
Copy link
Contributor

@newly12 newly12 commented Sep 24, 2020

What does this PR do?

If the files match the excluded pattern, recursiveWatcher does not emit events for them, as well as errors if any.

Why is it important?

This eliminates unnecessary warning logs for file paths that user intends to exclude.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

run test case TestRecursiveExcludedPaths

Related issues

@newly12 newly12 requested a review from a team as a code owner September 24, 2020 07:40
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 24, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 24, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [marc-gr commented: jenkins run tests]

  • Start Time: 2020-11-09T08:20:43.238+0000

  • Duration: 26 min 52 sec

Test stats 🧪

Test Results
Failed 0
Passed 456
Skipped 60
Total 516

@newly12 newly12 marked this pull request as draft September 24, 2020 12:27
@newly12 newly12 marked this pull request as ready for review September 24, 2020 12:49
@elasticmachine
Copy link
Collaborator

Pinging @elastic/siem (Team:SIEM)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 24, 2020
@newly12
Copy link
Contributor Author

newly12 commented Oct 9, 2020

Hi,

Could someone please have a look at this PR? Feel free to let me know your thought on this. Thanks in advance.

@marc-gr
Copy link
Contributor

marc-gr commented Oct 16, 2020

jenkins run tests

@marc-gr marc-gr self-assigned this Oct 19, 2020
@newly12
Copy link
Contributor Author

newly12 commented Oct 31, 2020

Hi,

the test is completed. is there any concern regarding this PR? Please let me know.

@@ -154,7 +161,7 @@ func (watcher *recursiveWatcher) forwardEvents() error {
switch event.Op {
case fsnotify.Create:
err := watcher.addRecursive(event.Name)
if err != nil {
if err != nil && !watcher.isExcludedPath(event.Name) {
Copy link
Contributor

@adriansr adriansr Nov 2, 2020

Choose a reason for hiding this comment

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

So the changes to this function are ignoring errors related to ignored paths. Does it still make sense to act on events for ignored paths?

For example, adding a recursive watch on an ignored directory. I wonder if it will make more sense to just skip those events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adriansr Thanks for the review, it makes more sense to stop watching them. I've updated this PR regarding your suggestion, please have a look.

@newly12 newly12 force-pushed the ignore_excluded_paths branch from 3e68cf9 to 05ac7c6 Compare November 5, 2020 05:56
@adriansr
Copy link
Contributor

adriansr commented Nov 5, 2020

jenkins, test this

@adriansr
Copy link
Contributor

adriansr commented Nov 5, 2020

jenkins, test this please

@vjsamuel
Copy link
Contributor

vjsamuel commented Nov 6, 2020

@adriansr do we have anything left to address as part of this PR? would love to get this in so that we can reenable log shipping on our end.

@adriansr
Copy link
Contributor

adriansr commented Nov 6, 2020

Jenkins run tests

@elasticmachine
Copy link
Collaborator

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 456
Skipped 60
Total 516

Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

@newly12, all is looking good. Only needs a changelog entry in CHANGELOG.next.asciidoc.

@newly12 newly12 force-pushed the ignore_excluded_paths branch from 05ac7c6 to e0f4aa8 Compare November 8, 2020 11:56
@newly12 newly12 changed the title ignore fsnotify events for excluded files stop monitoring excluded paths Nov 8, 2020
@newly12
Copy link
Contributor Author

newly12 commented Nov 8, 2020

Thanks @adriansr ! I've CHANGELOG.next.asciidoc and title of this PR as stop monitoring excluded paths to reflect what it really does. Please let me know if anything.

@marc-gr
Copy link
Contributor

marc-gr commented Nov 9, 2020

jenkins run tests

@adriansr adriansr merged commit af8eedd into elastic:master Nov 9, 2020
@adriansr adriansr added bug needs_backport PR is waiting to be backported to other branches. Auditbeat Team:Security-External Integrations and removed Team:SIEM labels Nov 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@marc-gr marc-gr added v7.11.0 and removed needs_backport PR is waiting to be backported to other branches. labels Nov 9, 2020
marc-gr pushed a commit to marc-gr/beats that referenced this pull request Dec 1, 2020
If the files match the excluded pattern, recursiveWatcher does not emit events for them, as well as errors if any.

(cherry picked from commit af8eedd)
marc-gr added a commit that referenced this pull request Dec 1, 2020
If the files match the excluded pattern, recursiveWatcher does not emit events for them, as well as errors if any.

(cherry picked from commit af8eedd)

Co-authored-by: Peter Deng <[email protected]>
@newly12 newly12 deleted the ignore_excluded_paths branch December 7, 2020 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should not emit fsnotify watcher error for files match excluded_files pattern
6 participants