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

Add global setting to that changes the behavior to use Heap memory - affects beats, http and tcp input #16051

Closed
Tracked by #16042
andsel opened this issue Apr 2, 2024 · 3 comments · Fixed by #16054
Assignees

Comments

@andsel
Copy link
Contributor

andsel commented Apr 2, 2024

Add a setting in logstash.yml that permit to the user to enable or disable the Java Heap allocation, instead of Direct memory, for buffers used by Netty related plugins.
This setting, off by default, requires explicit opt-in from user. Once it's switched on all Netty's plugins instances (like logstash-input-beats logstash-input-http logstash-input-tcp) will allocated buffers on Java heap.
This permit to better tracking on OOM heap dump the most demanding consumers of byte buffers, and spot easily memory problems.

@yaauie
Copy link
Member

yaauie commented Apr 2, 2024

I am not against a global opt-in flag, but I think there is some danger lurking here.

Currently the netty jars are provided separately by each of the plugins that rely on them, and since they are loaded into the ruby runtime I believe we effectively have no guarantees about which set of jars are loaded at runtime. Upon first-use, netty does some platform-dependent capabilities-detection and pairs that with system properties that are present to determine global defaults. We rely on these implicit defaults in each of the above-mentioned plugins.

I am a little wary of propagating a logstash.yml into setting a system property that we hope to be observed by jars that we don't directly provide. Yes, we transitively provide the jars by nature of bundling the plugins that provide them, and yes we can guarantee that we modify the system settings before plugins are loaded (and therefore before Netty itself), but it feels fragile.

I believe that the plugins could each use ChannelConfig#setAllocator(ByteBufAllocator) explicitly instead of relying on the implicit default allocator. We may have to do the same for the SSL-wrapper in each.

We could introduce a logstash-mixin-netty_support support adapter that (a) provides the netty jars (reducing bloat and ensuring a single netty is present) and (b) provides a ByteBufAllocator whose behaviour is configurable in a standardized way. Like the ecs_compatibility support adapter, this could pull its defaults from LS core when running on a Logstash that provides the settings, or fall back to the known-legacy behaviour. This would allow us to configure individual plugins, whole pipelines, or the whole process.

If we wish to introduce the global flag quickly, and to defer the work of breaking out the shared dependency, then we should implement the flag in a way that doesn't preclude that future effort and doesn't bind us more-tightly with netty's internals, something like:

  • pipeline.receive_buffer.type: direct, heap
    • pipeline: this controls pipeline behaviour; it cannot be overridden per-pipeline yet, but it may be possible in the future
    • receive_buffer: namespace doesn't include netty intentionally, so that it can be used here and to inform receive buffers for other plugins.
    • type: allows us to have other configurable attributes in the future (such as the pooling threshold, or a future effort for shared buffer pools)

@andsel
Copy link
Contributor Author

andsel commented Apr 3, 2024

Thank's @yaauie to chime in! I like the proposal to have a more structured and stable behaviour then relying on system properties, but as first step we could implement in the ugly way , using the system properties. We know that plugin's dependencies are loaded by JRuby when the plugin start, so on the bootstrapping phase of Logstash runner we are quite sure that Netty classes hasn't yet been loaded and we are still in time to set "our" system properties to configure it.

In a second phase of development we could introduce the netty-support dependency and adjust all the interested plugins, and later improve such support plugin to handle the allocator on a more fine grained way.

@yaauie
Copy link
Member

yaauie commented Apr 3, 2024

but as first step we could implement in the ugly way

Absolutely.

My suggestion is to make the interface for configuring this (a setting named like pipeline.receive_buffer.type) forward-compatible with a future where it is implemented in a less-ugly way, so that a user who begins using the feature now doesn't have to migrate to the "less ugly" implementation that is on our wishlist if/when we eventually deliver it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants