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

feature: tag events that come from a filestream with take_over: true #39828

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Jun 7, 2024

Proposed commit message

Tag events that come from a filestream with take_over: true.
Add appropriate test cases.

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

Related issues

Use cases

From #34894,

We could filter on this tag in Kibana and see the events which came from a filestream in the "take over" mode. So, we know that these events are from the same files the log input(s) was/were previously ingesting.

Kibana Screenshot with filter

Screenshot 2024-06-07 at 9 48 40 PM

@VihasMakwana VihasMakwana requested a review from a team as a code owner June 7, 2024 14:02
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 7, 2024
Copy link
Contributor

mergify bot commented Jun 7, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @VihasMakwana? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@VihasMakwana VihasMakwana added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jun 7, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 7, 2024
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

Great job @VihasMakwana!

The change log entry is also missing.

filebeat/input/filestream/input_test.go Outdated Show resolved Hide resolved
@rdner
Copy link
Member

rdner commented Jun 7, 2024

@VihasMakwana great work! Could you please attach a screenshot to the PR description that shows Kibana with events filtered by the new tag (for historical purposes).

Copy link
Contributor

mergify bot commented Jun 7, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feature-add-takeover-tags upstream/feature-add-takeover-tags
git merge upstream/main
git push upstream feature-add-takeover-tags

@VihasMakwana
Copy link
Contributor Author

@rdner Here's a screenshot of filter:

Screenshot 2024-06-07 at 9 48 40 PM

@cmacknz
Copy link
Member

cmacknz commented Jun 7, 2024

This is more of a comment for the original issue than the PR itself, but do we really need this on every event? There is a storage cost associated adding instrumentation fields like this, and this will be going on potentially one of the highest volume event sources we have (logs).

I also highly suspect this field is only useful around the time take over is first enabled, and after the transition period it likely has low value.

@VihasMakwana
Copy link
Contributor Author

This is more of a comment for the original issue than the PR itself, but do we really need this on every event? There is a storage cost associated adding instrumentation fields like this, and this will be going on potentially one of the highest volume event sources we have (logs).

I also highly suspect this field is only useful around the time take over is first enabled, and after the transition period it likely has low value.

In that case, it might make sense to add a flag e.g. "tag_events: true" for take over.
After than, the user can switch that flag off and we won't enrich the events.
We can use "false" as a default value as this comes with a storage cost. What do you think?

@rdner
Copy link
Member

rdner commented Jun 10, 2024

@cmacknz this will mark events only when the take_over is true (according to the implementation in this PR), therefore the behavior is "opt-in".

Also, the take_over mode is used only for the migration process and once the inputs got migrated it's not necessary anymore and can be disabled again. In this case, no additional disk space is required. This rag is very valuable for debugging and the feature request is actually coming from the support, so we can easier troubleshoot the migration.

@pierrehilbert
Copy link
Collaborator

Maybe we can add a step 4 to make it obvious that the user needs to remove the flag?
It already indicated here https://www.elastic.co/guide/en/beats/filebeat/current/_step_2_enable_the_take_over_mode.html but I feel this is better to have it two times than having it missed.
WDYT @rdner?
cc @kilfoyle

@rdner
Copy link
Member

rdner commented Jun 10, 2024

@pierrehilbert yeah, I think we should update the docs and tell the users that once they ran a filestream input in take_over: true mode and saw incoming events with the tag introduced in this PR, they can remove take_over: true and restart the input again.

@VihasMakwana is that something you would be able to add to the docs as a follow up PR?

@VihasMakwana
Copy link
Contributor Author

@pierrehilbert yeah, I think we should update the docs and tell the users that once they ran a filestream input in take_over: true mode and saw incoming events with the tag introduced in this PR, they can remove take_over: true and restart the input again.

@VihasMakwana is that something you would be able to add to the docs as a follow up PR?

Sure, I would open up another PR for this soon.

@VihasMakwana
Copy link
Contributor Author

Follow up docs PR: #39863

@VihasMakwana VihasMakwana merged commit 328670b into elastic:main Jun 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tag events that come from a filestream with take_over: true
6 participants