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

[filebeat][gcs] - Removed bucket_timeout config option and replaced bucket context with parent program context #41970

Merged
merged 4 commits into from
Dec 11, 2024
Merged

[filebeat][gcs] - Removed bucket_timeout config option and replaced bucket context with parent program context #41970

merged 4 commits into from
Dec 11, 2024

Conversation

ShourieG
Copy link
Contributor

@ShourieG ShourieG commented Dec 10, 2024 •
edited
Loading

Type of change

  • Deprecation
  • Docs

Proposed commit message

  • WHAT: Removed bucket_timeout config option and replaced bucket context with parent program context
  • WHY: The bucket_timeout option was used to specify the timeout for the individual bucket operations. This option was
    difficult to understand in its effects resulting in malconfiguration leading to processing timeouts and context cancellation, ultimately causing gaps in the ingested data. To avoid this, the bucket_timeout option has been removed. The input now uses the parent program context to handle the contexts for bucket operations.

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.

Disruptive User Impact

Since it is a config option that has not impact on ingested data, this removal should not have any user impact.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@ShourieG ShourieG requested a review from a team as a code owner December 10, 2024 12:03
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 10, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 10, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ShourieG? 🙏.
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-8./d is the label to automatically backport to the 8./d branch. /d is the digit

@mergify Mergify
Copy link
Contributor

mergify bot commented Dec 10, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Dec 10, 2024
@ShourieG ShourieG added the Team:Security-Service Integrations Security Service Integrations Team label Dec 10, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 10, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@ShourieG ShourieG added Filebeat Filebeat needs_team Indicates that the issue/PR needs a Team:* label labels Dec 10, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 10, 2024
@ShourieG ShourieG added needs_team Indicates that the issue/PR needs a Team:* label input:GCS labels Dec 10, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 10, 2024
@ShourieG ShourieG added docs needs_team Indicates that the issue/PR needs a Team:* label labels Dec 10, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 10, 2024
@ShourieG ShourieG added needs_team Indicates that the issue/PR needs a Team:* label deprecation labels Dec 10, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 10, 2024
@ShourieG ShourieG mentioned this pull request Dec 10, 2024
Open
14 tasks
efd6
efd6 reviewed Dec 11, 2024
View reviewed changes
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Suggested commit message body:

Removed bucket_timeout config option and replaced bucket context with parent
program context

The bucket_timeout option was used to specify the timeout for the individual
bucket operations. This option was difficult to understand in its effects
resulting in malconfiguration leading to processing timeouts and context
cancellation, ultimately causing gaps in the ingested data. To avoid this,
the bucket_timeout option has been removed. The input now uses the parent
program context to handle the contexts for bucket operations.

x-pack/filebeat/docs/inputs/input-gcs.asciidoc Outdated
@@ -23,6 +23,8 @@ The input can be configured to work with and without polling, though if polling
3. If any major error occurs which stops the main thread, the logs will be appropriately generated,
describing said error.

**Config Option Removal Notice** : The `bucket_timeout` config option has been removed from the google cloud storage input. The intention behind this removal is to simplify the configuration and to make it more user friendly. The `bucket_timeout` option was used to specify the timeout for the individual bucket operations. But this proved counter-intuitive because bucket operations could slow down due to a lot of external factors like network latency, file size, back pressure etc. This could lead to the input not being able to process the files in time, which would then lead to a context cancellation error. This error would then cause gaps in the ingested data. To avoid this, the `bucket_timeout` option has been removed. The input now uses the parent program context to handle the contexts for bucket operations. This ensures that the input is able to process the files in a timely manner and avoid most context cancellation errors.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to say so much. All we need to say is that it was confusing and had the potential to let users malconfigure the input, and so it's been removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the documentation.

efd6
efd6 approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

Thanks

@ShourieG ShourieG merged commit 002a220 into elastic:main Dec 11, 2024
20 of 22 checks passed
mergify bot pushed a commit that referenced this pull request Dec 11, 2024
@ShourieG @mergify
[filebeat][gcs] - Removed bucket_timeout config option and replaced b…
7b03526 
…ucket context with parent program context (#41970)

(cherry picked from commit 002a220)
@mergify mergify bot mentioned this pull request Dec 11, 2024
Merged
6 tasks
@ShourieG ShourieG deleted the gcs/remove_bucket_timeout branch December 11, 2024 08:17
ShourieG added a commit that referenced this pull request Dec 11, 2024
@mergify @ShourieG
[filebeat][gcs] - Removed bucket_timeout config option and replaced b…
f468019 
…ucket context with parent program context (#41970) (#41987)

(cherry picked from commit 002a220)

Co-authored-by: ShourieG <shourie.ganguly@elastic.co>
@ShourieG ShourieG added the backport-8.17 Automated backport with mergify label Dec 11, 2024
mergify bot pushed a commit that referenced this pull request Dec 11, 2024
@ShourieG @mergify
[filebeat][gcs] - Removed bucket_timeout config option and replaced b…
e6399a3 
…ucket context with parent program context (#41970)

(cherry picked from commit 002a220)
@mergify mergify bot mentioned this pull request Dec 11, 2024
Merged
6 tasks
ShourieG added a commit that referenced this pull request Dec 12, 2024
@mergify @ShourieG
[filebeat][gcs] - Removed bucket_timeout config option and replaced b…
e4ac9d2 
…ucket context with parent program context (#41970) (#41997)

(cherry picked from commit 002a220)

Co-authored-by: ShourieG <shourie.ganguly@elastic.co>
michalpristas pushed a commit to michalpristas/beats that referenced this pull request Dec 13, 2024
@ShourieG @michalpristas
[filebeat][gcs] - Removed bucket_timeout config option and replaced b…
9e5af0b 
…ucket context with parent program context (elastic#41970)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efd6 efd6 efd6 approved these changes

Assignees

@ShourieG ShourieG

Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.17 Automated backport with mergify deprecation docs Filebeat Filebeat input:GCS Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShourieG @elasticmachine @efd6