[Meta] [FEATURE] Add getSettings support for AD

[owaiskazi19]

This issue is to register custom settings support for extensions.

We can split this into 3 issues:

• GetSettings: Remove unused settings to unblock creating a detector for AD extension. #121
• Add support for transporting Setting from Extension to OpenSearch.
• Add an API to register settings from an extension.

[saratvemulapalli]

Taking a stab at this.
Looks like all the settings we are registering are not needed for create detector[1].
This issue can be split into 3 issues:

• Remove unused settings to unblock creating a detector for AD extension.
• Add support for transporting Setting from Extension to OpenSearch.
• Add an API to register settings from an extension.

[1] https://github.com/opensearch-project/anomaly-detection/blob/feature/extensions/src/main/java/org/opensearch/ad/AnomalyDetectorPlugin.java#L797-L809

[joshpalis]

getSettings support is required for creating a detector.

During createComponents , Anomaly Detection creates three components for use in creating a detector : SearchFeatureDao, ADTaskManager, and AnomalyDetectionIndices. During component construction, each component sets fields based on values retrieved from Anomaly Detector settings registered within OpenSearch. Additionally, these components also add setting update consumers, essentially listeners, for each setting it has registered, in order to update these fields whenever a setting has been changed. Therefore, it is necessary for these settings to be first registered within OpenSearch prior to creating these components, and also prior to creating a detector.

For ADTaskManager, the required settings are :

• MAX_OLD_AD_TASK_DOCS_PER_DETECTOR
• BATCH_TASK_PIECE_INTERVAL_SECONDS
• DELETE_AD_RESULT_WHEN_DELETE_DETECTOR
• MAX_BATCH_TASK_PER_NODE
• MAX_RUNNING_ENTITIES_PER_DETECTOR_FOR_HISTORICAL_ANALYSIS
• REQUEST_TIMEOUT

For SearchFeatureDao, the required settings are :

• MAX_ENTITIES_FOR_PREVIEW
• PAGE_SIZE

For AnomalyDetectionIndices, the required settings are :

• AD_RESULT_HISTORY_ROLLOVER_PERIOD
• AD_RESULT_HISTORY_MAX_DOCS_PER_SHARD
• AD_RESULT_HISTORY_RETENTION_PERIOD
• MAX_PRIMARY_SHARDS

All of these settings are registered within opensearch via getSettings extension point here : https://github.com/opensearch-project/anomaly-detection/blob/11d094aaa1ab5efc9ff80532884f714ff38680a3/src/main/java/org/opensearch/ad/AnomalyDetectorPlugin.java#L805

Note : Settings that are not used for creating a detector should also be removed from getSettings

For information on how these settings are used during create components is documented here : #78 (comment)

[dbwiddis]

We're going to have to re-order something in the Node / SettingsModule / ExtensionsOrchestrator initialization sequence.

In Node.java constructor:

• ExtensionsOrchestrator instantiated line 426 (passed to some objects)
• additionalSettings gets plugin settings line 473
• additionalSettingsFilter gets plugin settings filter line 474
• SettingsModule initialized line 492 (this is where settings are registered)
• ExtensionsOrchestrator initialized line 781 (registers request handlers)
  In Node.java start():
• ExtensionsOrchestrator extensionsInitialize line 1174 (initializes extensions; earliest point we can get settings from them)

Can we delay initializing SettingsModule until after start at line 1174? No: the plugin settings (indexScopedSettings) and settings filters are used extensively in other objects.

• clusterSettings used line 498 and 504 (scriptModule)
• indexScopedSettings used line 651 and 680 (indicesService, MetadataCreateIndexService)
• clusterSettings, indexScopedSettings and settingsFilter used line 720-722 (ActionModule)
• indexScopedSettings used line 755 (MetadataIndexUpgradeService)
• clusterSettings used line 774 and 794 (TransportService, RecoverySettings)
• settingFilter used line 877 (NodeService)
• clusterSettings used line 964 (test only)

Can we move ExtensionsOrchestrator initialization steps before line 492 (best) or 651 (where settings first used)? No: it needs other services initialized, which get the settings!

Solution requires updating settings after initialization. This requires also updating settings on all other modules. This is simply not part of the design; settings are assumed not to change after initialization. Attempting to hack a change into all these would be error-prone.

Possibility: Create a new module designed to get extension settings after initialization. Pass the (empty) module object around to other modules where it is needed. Add settings to it after initialization. When looking up settings, use the existing stored settings first and if the settings aren't there, try the extension settings module as a backup.

[owaiskazi19]

To register settings dynamically after the bootstrap @saratvemulapalli worked on opensearch-project/OpenSearch#3753.

[dbwiddis]

And I even reviewed that PR! 🤦‍♂️ That solves the problem.

[dbwiddis]

Why didn't you list that among our assets in the first place?

[owaiskazi19]

Why didn't you list that among our assets in the first place?

Your good set of questions, my poor memory and Sarat's vision to foresee the future!

[dbwiddis]

OK, so we still have a significant issue.

Settings<T> is not easily serialized. Well, that's not true, I can easily serialize it to JSON with toXContent(). It's not easily _de-_serialized, though, as two of its fields are functional interfaces.

It looks like there are a few approaches to this, including Java Serialization (yuck) that I'll have to look into. Suggestions welcome.

[dbwiddis]

OK, the good news is the <T> is not as open-ended as I thought. There are a limited number of settings types presently implemented (int, long, boolean, float, double, String, some unit-based ones combining a number and a unit).

The transport request can use a switch/case based on <T> to serialize and deserialize these known types. See Setting class methods boolSetting(...), intSetting(...), etc.

There are a few extra things with nested/recursive settings objects (fallback settings, group settings, etc.) needed for a complete implementation, but the simple ones with no fallback handle all the use cases in AnomalyDetectorSettings class and can unblock the near-term work.

[dbwiddis]

FYI @joshpalis and @owaiskazi19 I will be creating a helper/wrapper class to implement the Setting-to-stream and stream-to-Setting code described in the previous comment. Name may change (because naming things is hard) but expect something along the lines of:

class WriteableSettings implements Writeable {
    private Settings<?> settings;
  
    WriteableSettings(Settings<?> settings) {
        this.settings = settings;
    }

    @Override
    public void writeTo(StreamOutput out) {
        // switch/case based on the known types
    }

    @Override
    Settings<?> read(StreamInput in) {
        // switch/case based on the known types
    }
}

[dbwiddis]

Getting the type isn't as easy as I thought.... default values for the win!

public class WriteableSetting implements Writeable {

    private Setting<?> setting;

    public WriteableSetting(Setting<?> setting) {
        this.setting = setting;
    }

    @Override
    public void writeTo(StreamOutput out) throws IOException {
    }

    private String getTypeOfSetting() {
        return setting.getDefault(null).getClass().getName();
    }

    public static void main(String[] args) {
        Setting<Integer> s = Setting.intSetting("foo", 42, 0, Setting.Property.IndexScope);
        WriteableSetting ws = new WriteableSetting(s);
        System.out.println(ws.getTypeOfSetting());

        Setting<ByteSizeValue> bs = Setting.memorySizeSetting("bar", new ByteSizeValue(8, ByteSizeUnit.GB), Setting.Property.IndexScope);
        WriteableSetting wbs = new WriteableSetting(bs);
        System.out.println(wbs.getTypeOfSetting());
    }
}

java.lang.Integer
org.opensearch.common.unit.ByteSizeValue

[dbwiddis]

Original
{
  "key" : "the answer to life",
  "properties" : [
    "Final",
    "IndexScope"
  ],
  "is_group_setting" : false,
  "default" : "42"
}
After transport across a (byte)stream
{
  "key" : "the answer to life",
  "properties" : [
    "Final",
    "IndexScope"
  ],
  "is_group_setting" : false,
  "default" : "42"
}

[dbwiddis]

Initial work for (draft) review in opensearch-project/OpenSearch#4519 @joshpalis @owaiskazi19 @saratvemulapalli