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

in gelf: new plugin #7194

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

in gelf: new plugin #7194

wants to merge 5 commits into from

Conversation

Matanelc
Copy link

@Matanelc Matanelc commented Apr 14, 2023 •
edited
Loading

Rebased on -> #4156

New input plugin for ingest GELF messages from TCP and UDP. In UDP support for gzip and zlib compressed messages, and chunked messages.

Enter [N/A] in the box, if an item is not applicable to your change.

Testing Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change

  • [N/A] Debug log output from testing the change

  • Attached Valgrind output that shows no leaks or memory corruption was found

Documentation

  • Documentation required for this feature

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@Matanelc Matanelc temporarily deployed to pr April 14, 2023 14:21 — with GitHub Actions Inactive
@Matanelc Matanelc temporarily deployed to pr April 14, 2023 14:21 — with GitHub Actions Inactive
@Matanelc Matanelc temporarily deployed to pr April 14, 2023 14:22 — with GitHub Actions Inactive
@Matanelc Matanelc temporarily deployed to pr April 14, 2023 14:46 — with GitHub Actions Inactive
matanelcohen and others added 3 commits April 16, 2023 10:43
@matanelcohen
support IN_GELF, rebased and fixed compile error
582d591 
Signed-off-by: Matanel Cohen <[email protected]>
@pwhelan @matanelcohen
in_calyptia_fleet: initial implementation of calyptia fleet managemen…
67b4dfc 
…t. (fluent#7179)

* in_calyptia_fleet: initial implementation of Calyptia fleet management.

Signed-off-by: Phillip Whelan <[email protected]>
Signed-off-by: Matanel Cohen <[email protected]>
@matanelcohen
support IN_GELF, rebased and fixed compile error
d8a5e20 
Signed-off-by: Matanel Cohen <[email protected]>
@Matanelc Matanelc temporarily deployed to pr April 16, 2023 09:31 — with GitHub Actions Inactive
@Matanelc Matanelc temporarily deployed to pr April 16, 2023 09:31 — with GitHub Actions Inactive
@Matanelc Matanelc temporarily deployed to pr April 16, 2023 09:31 — with GitHub Actions Inactive
@Matanelc Matanelc temporarily deployed to pr April 16, 2023 09:55 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens changed the title Matanelc/in gelf in gelf: new plugin May 5, 2023
@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label May 5, 2023
@patrick-stephens
Copy link
Contributor

Can you link the docs PR?

patrick-stephens
patrick-stephens reviewed May 5, 2023
View reviewed changes
CMakeLists.txt
@@ -201,6 +201,9 @@ option(FLB_IN_WINDOWS_EXPORTER_METRICS "Enable windows exporter metrics input pl
option(FLB_IN_OPENTELEMETRY "Enable OpenTelemetry input plugin" Yes)
option(FLB_IN_ELASTICSEARCH "Enable Elasticsearch (Bulk API) input plugin" Yes)
option(FLB_IN_CALYPTIA_FLEET "Enable Calyptia Fleet input plugin" Yes)
option(FLB_IN_GELF "Enable GELF input plugin" Yes)
option(FLB_IN_CALYPTIA_FLEET "Enable Calyptia Fleet input plugin" Yes)
option(FLB_IN_GELF "Enable GELF input plugin" Yes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a rebase has gone wrong here.

@patrick-stephens
Copy link
Contributor

Can we add any integration tests? https://github.com/fluent/fluent-bit-ci

@leonardo-albertovich
Copy link
Collaborator

I'm assigning this to myself so I can properly review it on monday.

@leonardo-albertovich leonardo-albertovich self-assigned this May 5, 2023
@leonardo-albertovich
Copy link
Collaborator

It seems to me that this PR is packed with goodies but I think it needs to be split in three :

  1. The changes in flb_gzip.c : for this one in particular I think the description should explain the changes it makes to flb_gzip_uncompress because @cosmo0920 is in the process of making some changes to it and I don't want this to cause unnecessary / avoidable hardships.
  2. The gelf unpacker code should be submitted as a stand alone "component" just like flb_pack_gelf and ideally if possible a parser that uses this new unpacker should be added. That way this new feature can be easily reused .
  3. If by this point we determine that in_tcp and in_udp can't be improved to use the newly created gelf parser (or if we determine that the addition would improve ease of use greatly) a third PR with the output plugin should follow.

Obviously this is the "best case scenario" and I understand that not everyone might share these opinions but I do think we should at least try to find a middle ground to make the most out of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrick-stephens patrick-stephens patrick-stephens left review comments

@fujimotos fujimotos Awaiting requested review from fujimotos fujimotos is a code owner

@niedbalski niedbalski Awaiting requested review from niedbalski niedbalski is a code owner

@edsiper edsiper Awaiting requested review from edsiper edsiper is a code owner

@leonardo-albertovich leonardo-albertovich Awaiting requested review from leonardo-albertovich leonardo-albertovich is a code owner

@koleini koleini Awaiting requested review from koleini koleini is a code owner

@celalettin1286 celalettin1286 Awaiting requested review from celalettin1286 celalettin1286 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@leonardo-albertovich leonardo-albertovich

Labels
docs-required ok-package-test Run PR packaging tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Matanelc @patrick-stephens @leonardo-albertovich @matanelcohen @pwhelan