[plugin/apm-data]Fallback to ILM for non-dsl-only mode if DSL unavailable

[lahsivjar]

An alternative PR to #112615 and #112614. With this PR, If a component or index template is named ending @ilm then the file is NOT loaded for datastream-only mode.

Related to

Closes: #112137
Closes: elastic/apm-server#13898

[elasticsearchmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[axw]

Thanks, I like this a lot more than having override files and spreading things across plugins. Looks great.

Should we add YAML REST tests to verify the ILM fallback works correctly?

[lahsivjar]

Should we add YAML REST tests to verify the ILM fallback works correctly?

@axw Added test for checking if ILM files are installed in normal mode. I wanted to also add tests for dsl-only-mode set to true but that didn't work since the setting is not exported I think so the test cluster fails to start.

[axw]

@lahsivjar what's there looks good. I'd really like to see functional tests that confirm the ILM policies are effective, not just that the component templates exist (checking that the backing indices are managed by ILM would be good enough); but this can also be done in a followup.

[lahsivjar]

(checking that the backing indices are managed by ILM would be good enough); but this can also be done in a followup

@axw This is a bit more complicated. If there were an older version of the plugin that had ILM it would have been easier, for testing it at the current state we would have to emulate some of the APM integration templates and ILMs... not impossible but would take a bit more effort. I will look into it as a followup.

[gmarouli]

@lahsivjar thanks for working on this! I was thinking what if we perform this filtering in the APMIndexTemplateRegistry? Because this is not yet a well-established solution, but a way to address this in the APM case, I think it would be better to keep it localised in APMIndexTemplateRegistry. Maybe we override componentTemplates in that class. What do you think?

[lahsivjar]

I was thinking what if we perform this filtering in the APMIndexTemplateRegistry? Because this is not yet a well-established solution, but a way to address this in the APM case, I think it would be better to keep it localised in APMIndexTemplateRegistry.

The YamlTemplateRegistry seems to be created from the apm-data plugin when the otel-data plugin was introduced (both are owned by the intake-services team) so, at least for now, it is only used by our plugins. However, I think it makes sense to have the interface be more generic.

Maybe we override componentTemplates in that class. What do you think?

How about we allow overriding for shouldLoadTemplate method? The YamlTemplateRegistry can always return true but inheritors of YamlTemplateRegistry can override it to filter out some of the templates conditionally as required.

[lahsivjar]

@gmarouli I have pushed a fix for scoping logic within the APM plugin (commit ref). It is based on overriding shouldLoadTemplate method I proposed above, let me know if it still doesn't look good - will replace it with overriding the component template methods

[gmarouli]

How about we allow overriding for shouldLoadTemplate method? The YamlTemplateRegistry can always return true but inheritors of YamlTemplateRegistry can override it to filter out some of the templates conditionally as required.

I really like this idea!

[gmarouli]

I really like your proposed solution of providing a filter to YamlTemplateRegstry; however, I find the current way a bit conflicting.

• It has a generic name shouldLoadTemplate which is based on the name of the template but then it also needs a boolean (the dslOnlyMode) which feels a bit random.
• If we would extract the dslOnlyMode and apply it inlined in the map, we might be limiting the scope of the shouldLoadTemplate filter.

So, what if we pass the predicate in the constructor:

public YamlTemplateRegistry(
        Settings nodeSettings,
        ClusterService clusterService,
        ThreadPool threadPool,
        Client client,
        NamedXContentRegistry xContentRegistry,
        FeatureService featureService
    ) {
        this(nodeSettings, clusterService, threadPool, client, xContentRegistry, featureService, ignored -> true);
    }
    
......
public YamlTemplateRegistry(
        Settings nodeSettings,
        ClusterService clusterService,
        ThreadPool threadPool,
        Client client,
        NamedXContentRegistry xContentRegistry,
        FeatureService featureService,
        Predicate<String> templateFilter
    ) {
    .....
    componentTemplates = Optional.ofNullable(componentTemplateNames)
                .orElse(Collections.emptyList())
                .stream()
                .map(o -> (String) o)
                .filter(templateFilter)
                .collect(Collectors.toMap(name -> name, name -> loadComponentTemplate(name, version)));
                .......
}

And in ApmIndexTemplateRegistry:

public APMIndexTemplateRegistry(
        Settings nodeSettings,
        ClusterService clusterService,
        ThreadPool threadPool,
        Client client,
        NamedXContentRegistry xContentRegistry,
        FeatureService featureService
    ) {
        super(
            nodeSettings,
            clusterService,
            threadPool,
            client,
            xContentRegistry,
            featureService,
            templateName -> DataStreamLifecycle.isDataStreamsLifecycleOnlyMode(clusterService.getSettings()) == false
                || templateName.endsWith("@ilm") == false
        );
    }

This way the filter is generic enough to be anything we want and the definition of the filter is fully the responsibility of the subclass which allows it to fulfil a variety of use cases.

What do you think?

[lahsivjar]

What do you think?

@gmarouli Makes a lot of sense to me, I have updated the PR. Should be ready for another review!

[gmarouli]

Hi @lahsivjar , some small comments and then it's good to go! Great work!

[lahsivjar]

Updated! Thanks for the detailed review.

[gmarouli]

@elasticmachine update branch

Updating with main to see if this will fix the ci errors

[gmarouli]

Assuming ci is happy, LGTM :) Thank you for brining this home @lahsivjar , great work!

[gmarouli]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation