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

Clean up fluent-bit component construction #1537

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

Conversation

igorpeshansky
Copy link
Member

Description

Remove explicit construction of fluent-bit components from all third-party application receivers and processors. Factor out all fluent-bit components into helper functions and/or logging processors.

Related issue

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.

@@ -275,6 +275,55 @@ func (p LoggingProcessorNestWildcard) Components(ctx context.Context, tag, uid s
}
}

type LoggingProcessorLift struct {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't name these logging processors; we're unlikely to ever expose them as-is, and someone in the future might think that this was already designed and try to use it publicly.

Grep and lift belong as functions (or possibly still structs, for convenience) in the fluentbit package.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually hoping to eventually expose the actual grep, nest, and lift processors to third-party receivers/processors, and use them to construct auxiliary pipelines. This change gets us part of the way there. I am happy to change them to functions again, and lift them into processors as part of the pipelines change (I'd have to move the other proto-processors that were already present, like NestWildcard).

Copy link
Member

Choose a reason for hiding this comment

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

I think this suggests that there needs to a new category of processors: internal-only processors that can exist as objects in pipelines but can't be used in user config. Can you just move these into the fluentbit package and change the names slightly (maybe just call it fluentbit.Lift?)? I think they should still be usable as processors since they'll still implement the interface, and it will be clear they're not for user config.

(I think these are fluentbit-specific because nest/lift/etc. are all just done as workarounds specific to fluent-bit, and for an otel version of these third-party processors we wouldn't use them.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect we can achieve internal-only processors by defining the Type() method, but not calling RegisterType. Done.

While these translate directly into fluent-bit filters, the actions performed by those are more general (extract values that are nested under a field into a top-level field). That should be translatable to an otel logging filter as well.

Copy link
Member

Choose a reason for hiding this comment

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

I do not think that avoiding RegisterType is sufficient, because this is a loaded foot-gun. Also, we already have a clear pattern for how these should be implemented - as functions in confgenerator/fluentbit/processors.go that return []fluentbit.Component (and which AFAICT you are not touching in this PR).

While it's theoretically possible to translate these actions into otel filters, we would never do that because they are specific workarounds for fluent-bit bugs. There are better ways to do all of these things in otel without combining these specific action primitives.

confgenerator/logging_processors.go Outdated Show resolved Hide resolved
@@ -55,6 +56,40 @@ func healthChecksLogsPath() string {
return path.Join("${logs_dir}", "health-checks.log")
}

type LoggingProcessorSampleLogs struct {
Copy link
Member

Choose a reason for hiding this comment

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

This is bespoke enough that it's fine staying as raw fluent-bit config. I don't think it's ever going to be reused in another processor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, I was hoping to turn this into an internal logging processor that was used in an actual pipeline…

@igorpeshansky igorpeshansky force-pushed the igorpeshansky-refactor-fluent-bit-components branch from 1bdf6dc to 0a594ed Compare December 7, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants