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] Add support for bulk_max_size and preset #42312

Merged
merged 15 commits into from
Jan 27, 2025
Merged

[filebeat] Add support for bulk_max_size and preset #42312

merged 15 commits into from
Jan 27, 2025

Conversation

khushijain21
Copy link
Contributor

@khushijain21 khushijain21 commented Jan 15, 2025 •
edited
Loading

Proposed commit message

This PR is a follow up to #41849. It improves test scenarios. It also adds support for bulk_max_size and preset.

We will have to wait for next release of otel-collector to use compression_level parameter. But the default compression level used by elasticsearchexporter is set to 1. So we can go ahead and support preset paramater now

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.

Related Issue
Part of https://github.com/elastic/opentelemetry-dev/issues/614
open-telemetry/opentelemetry-collector-contrib#37260

@khushijain21 khushijain21 requested a review from a team as a code owner January 15, 2025 09:03
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 15, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 15, 2025

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @khushijain21? 🙏.
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 Jan 15, 2025

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 Jan 15, 2025
@khushijain21 khushijain21 marked this pull request as draft January 15, 2025 09:04
@khushijain21 khushijain21 changed the title [filebeat] Add support for bulk_max_size [filebeat] Add support for bulk_max_size and preset Jan 15, 2025
@khushijain21 khushijain21 marked this pull request as ready for review January 15, 2025 12:39
@khushijain21 khushijain21 requested a review from rdner January 15, 2025 12:39
@khushijain21 khushijain21 marked this pull request as draft January 15, 2025 12:49
@khushijain21 khushijain21 marked this pull request as ready for review January 15, 2025 16:48
@khushijain21 khushijain21 added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jan 15, 2025
@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 Jan 15, 2025
@khushijain21
Copy link
Contributor Author

/test

@cmacknz
Copy link
Member

cmacknz commented Jan 16, 2025 •
edited
Loading

It looks like we are mapping existing parameters in Beats to similarly named parameters in the OTel collector. This only makes sense if we know they behave equivalent from a performance perspective.

The bulk_max_size translating to max_size_items looks like it is probably OK, did we test that it behaves exactly the same? https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/elasticsearchexporter/README.md#batching

The presets I am much less sure about and I don't think we should touch them without benchmarking them. The parameter values looking the same doesn't matter, the performance characteristics being the same or better is what matters. How are we measuring that?

khushijain21 and others added 2 commits January 20, 2025 19:24
@khushijain21
Copy link
Contributor Author

khushijain21 commented Jan 20, 2025 •
edited
Loading

The bulk_max_size translating to max_size_items looks like it is probably OK, did we test that it behaves exactly the same?

Yes. It was tested with setting bulk_max_size to 1 and sending 1000 log events. The behavior in both modes was similar, i.e all documents were indexed

The presets I am much less sure about and I don't think we should touch them without benchmarking them. The parameter values looking the same doesn't matter, the performance characteristics being the same or better is what matters. How are we measuring that?

All parameters that preset overrides are supported including bulk_max_size as part of this PR. Compression level is always set to 1 on es-exporter so we should be good to go. We can compare the performance impact of preset in a follow up PR using the benchmark work being done here https://ingest-metricstore.kb.us-central1.gcp.cloud.es.io:9243/app/dashboards#/view/88664583-01a2-48e5-80e6-fc93db54fc86?_g=h@9d8efc3

@cmacknz
Copy link
Member

cmacknz commented Jan 20, 2025

Thanks, as long as we know that we aren't done with these conversions until we can see that the performance is similar enough.

belimawr
belimawr approved these changes Jan 21, 2025
View reviewed changes
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.

LGTM as long as the performance is equivalent or better as Craig mentioned.

leehinman
leehinman reviewed Jan 21, 2025
View reviewed changes
libbeat/otelbeat/beatconverter/beatconverter_test.go
Comment on lines +147 to +156
filebeatreceiver:
filebeat:
inputs:
- type: filestream
enabled: true
id: filestream-input-id
paths:
- /tmp/flog.log
output:
elasticsearch:
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 this makes sense. If you have any kind of beat receiver defined, then it has to use the otelconsumer output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the converter makes sure of setting otelconsumer as output. The config provided to the converter is not changed much from what we have in filebeat.yml.

You can also see this is true in the expected output here
https://github.com/elastic/beats/pull/42312/files/cd3c8a80de12c46705f24df2547f264fd3820424#diff-2160b9cb05793fa34025b850f0d97869eaf01f76997fffc55dde0b62d075e6c2R199-R201

@khushijain21 khushijain21 requested a review from a team as a code owner January 24, 2025 13:54
@khushijain21 khushijain21 merged commit cb34518 into elastic:main Jan 27, 2025
141 checks passed
mergify bot pushed a commit that referenced this pull request Jan 27, 2025
@khushijain21 @mergify
[filebeat] Add support for bulk_max_size and preset (#42312)
01f889b 
* TODO comments implemented

- Add support for bulk_max_size and presets in config translation layer

---------

Co-authored-by: Tiago Queiroz <me@tiago.life>
(cherry picked from commit cb34518)
@mergify mergify bot mentioned this pull request Jan 27, 2025
Merged
6 tasks
khushijain21 added a commit that referenced this pull request Jan 27, 2025
@mergify @khushijain21
[filebeat] Add support for bulk_max_size and preset (#42312) (#42429
3cc6507 
)

* TODO comments implemented

- Add support for bulk_max_size and presets in config translation layer

---------

Co-authored-by: Tiago Queiroz <me@tiago.life>
(cherry picked from commit cb34518)

Co-authored-by: Khushi Jain <khushi.jain@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlind23 jlind23 jlind23 approved these changes

@rdner rdner rdner approved these changes

@belimawr belimawr belimawr approved these changes

@leehinman leehinman Awaiting requested review from leehinman

Assignees

@khushijain21 khushijain21

Labels
backport-8.x Automated backport to the 8.x branch with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@khushijain21 @elasticmachine @cmacknz @belimawr @rdner @jlind23 @leehinman