elastic / logstash-filter-elastic_integration

The Elastic Integrations filter for Logstash, which enables running Elastic Integrations inside of Logstash pipelines
Other
5 stars 8 forks source link

Sync up with Elasticsearch main branch. #121

Closed mashhurs closed 7 months ago

mashhurs commented 7 months ago

Description

This change syncs up with Elasticsearch main branch source on ThreadPool constructor changes. ThreadPool now requires a metric but can accept No-op metric.

!BREAKING CHANGE?!

Now, the plugin build is broken with <=8.12.0 versions (see tested CI) because ThreadPool constructor was NOT expecting metric so far, 8.13.0 versions will require it. For me, this change should NOT be a broken if ES introduces separate interface and uses NOOP metric by default. We can still build plugin against main and use with 8.12 versions. Since it is not a default plugin yet, manual installation always fetch the latest version. Not sure if this will be a breaking change.

@yaauie @jsvd how do you think if we ask ES team (or contribute) to keep OLD constructor and separate recent interface

public ThreadPool(Settings settings, MeterRegistry meterRegistry, ExecutorBuilder<?>... customBuilders) {
    this(settings, customBuilders);
    this.executors.forEach((k, v) -> {
        this.instruments.put(k, setupMetrics(meterRegistry, k, v)); // instruments is not final
    });
}

OR another approach would be OLD constructor uses no-op metric by default (need to confirm with ES team if default is a desired behavior) on top of current ES change:

public ThreadPool(Settings settings, ExecutorBuilder<?>... customBuilders) {
    this(settings, MeterRegistry.NOOP, customBuilders);
}
yaauie commented 7 months ago

Because we embed the relevant Elasticsearch bits, it will be safe to make this change when we begin targeting 8.13; our builds that target 8.12 will continue to embed the relevant 8.12 bits, and will need to compile against them. I can't think of an easy way to merge a change that supports both simultaneously.

We could also ask the Elasticsearch team to restore the previous constructor for ThreadPool using a default value for the MeterRegistry parameter (perhaps even as a deprecated constructor?), although doing so would be an implicit burden on their team as they likely want to encourage use of the ThreadPool to include at least consideration for using a MeterRegistry.