Skip to content

[confgenerator] Create MergeInternalLoggingProcessor in FilesMixin to merge processor MultilineRules. #2025

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
Merged
merged 23 commits into from
Aug 15, 2025
Merged

[confgenerator] Create MergeInternalLoggingProcessor in FilesMixin to merge processor MultilineRules. #2025

merged 23 commits into from
Aug 15, 2025

Conversation

@franciscovalentecastro
Copy link
Contributor

@franciscovalentecastro franciscovalentecastro commented Aug 8, 2025 •
edited
Loading

Description

This PR creates the interface InternalLoggingProcessorMerger to enable InternalLoggingReceivers to merge InternalLoggingProcessors features into the receiver when possible and supported.

Specifically the new method MergeInternalLoggingProcessor of LoggingReceiverFilesMixin will merge the processor LoggingProcessorParseMultilineRegex.Rules []MultilineRules into the files receiver.

Details

  • Co-author @quentinmit : All the related MULTILINE_PARSER logic was updated so the "merging" only has to be implemented in one place. This reduces a lot of tech debt.

  • This solution resembles the already implemented MetricsProcessorMerger. This solution differs in the abstraction layer where this "merges" happen. MetricsProcessorMerger works for a processor in an Ops Agent pipeline. In comparison, InternalLoggingProcessorMerger works for a processor within a LoggingReceiver definition.

Updates

  • Created InternalLoggingProcessorMerger with method MergeInternalLoggingProcessor.
  • Added MergeInternalLoggingProcessor method to LoggingReceiverFilesMixin.
    • This will merge LoggingProcessorParseMultilineRegex.Rules []MultilineRules into the files receiver when possible.
  • Updated elasticseach_json receiver and processor with this refactoring.
    • Transformation test with multiline processing work as expected.

Context

As described before in #2013, there are fluent-bit downsides/bugs related to having MultilineRules in a separate FILTER processor outside of the INPUT. This solution enables the processor to merge this MultilineRules to the INPUT receiver when possible to avoid the downsides/bugs.

Related issue

b/413410475

How has this been tested?

Checklist:

  • Unit tests
    • Unit tests do not apply.
    • Unit tests have been added/modified and passed for this PR.
  • Integration tests
    • Integration tests do not apply.
    • Integration tests have been added/modified and passed for this PR.
  • Documentation
    • This PR introduces no user visible changes.
    • This PR introduces user visible changes and the corresponding documentation change has been made.
  • Minor version bump
    • This PR introduces no new features.
    • This PR introduces new features, and there is a separate PR to bump the minor version since the last release already.
    • This PR bumps the version.

@franciscovalentecastro franciscovalentecastro mentioned this pull request Aug 8, 2025
Closed
8 tasks
quentinmit
quentinmit reviewed Aug 8, 2025
View reviewed changes
confgenerator/logging_macros.go Outdated
Copy link
Member

@quentinmit quentinmit Aug 8, 2025

Choose a reason for hiding this comment

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

I take it you're adding this so you can register only the receiver. If we correctly handle the merging, though, can't we just continue to register it as a receiver + processor?

Copy link
Contributor Author

@franciscovalentecastro franciscovalentecastro Aug 11, 2025

Choose a reason for hiding this comment

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

I did this because all of this 3P app receivers that have MultilineRules in the INPUT tail receiver don't register a processor. Arguably, after this change, there is no restriction anymore since the processor now could work as a standalone without the multiline parsing in the INPUT.

@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-processor-merger branch from 28e4dca to a2aef14 Compare August 12, 2025 15:05
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-processor-merger branch from ff85480 to ddb2b27 Compare August 12, 2025 19:33
franciscovalentecastro
franciscovalentecastro commented Aug 13, 2025
View reviewed changes
Copy link
Contributor Author

@franciscovalentecastro franciscovalentecastro left a comment

Choose a reason for hiding this comment

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

LGTM!

There are some test failures (e.g. missing update goldens) which may be solved after removing some debug statements.

Thank you for all the help in this PR @quentinmit !

@quentinmit
Restore blank hostname labels to reduce golden diff
3e2c2f5
@quentinmit
Update goldens
79414ec
@quentinmit
Reduce diff
add61c3 
`yamlTag == "-"` doesn't work because the golden test contains its own
copy of the code (!), so the change was a noop.
@quentinmit
Register elasticsearch_json as a macro processor
750a899 
Now that we correctly merge multiline rules into inputs, it can safely
be used as the first processor in a file pipeline.
@quentinmit
Merge remote-tracking branch 'origin/master' into fcovalente-processo…
a05f4ec 
…r-merger
@quentinmit
Don't generate an empty multiline parser, in case that is what causes…
154dbdc 
… segfaults
@quentinmit
Update goldens
7aefe17
@quentinmit
Copy link
Member

quentinmit commented Aug 15, 2025

Unfortunately we were still getting fluent-bit segfaults on some of the transformation tests. I think I might have finally fixed them (by removing the empty multiline filter we were generating after moving the rules into the input), so if the checks all pass tonight I'll merge this. Otherwise we need to do more segfault hunting tomorrow.

@quentinmit
Copy link
Member

quentinmit commented Aug 15, 2025

The four remaining test failures all appear to be unrelated flakes due to quota issues in our CI environment, so I'm going to merge this.

@quentinmit quentinmit merged commit f5ef95d into master Aug 15, 2025
58 of 62 checks passed
@quentinmit quentinmit deleted the fcovalente-processor-merger branch August 15, 2025 20:13
@franciscovalentecastro franciscovalentecastro mentioned this pull request Aug 18, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@quentinmit quentinmit quentinmit approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@franciscovalentecastro @quentinmit