Rename "elasticsearch" Graylog server properties

danotorrey commented 1 year ago

What?

Several server configuration properties (supported in the server.conf) file have the word elasticsearch in them. These should be renamed to have the general indexer name instead.

See config files:

Why?

With the added support of Opensearch, using the name elasticsearch in the properties is no longer correct. Renaming them to the general indexer name is probably best.

This issue was created as a continuation of the discussion in https://github.com/Graylog2/graylog2-server/pull/13719#issuecomment-1282281972

Note that there were previously deprecated elasticsearch indexer properties, which are no longer deprecated with PR https://github.com/Graylog2/graylog2-server/pull/13018 since the plan is to use those as initializer values for the first boot going forward. Also see https://github.com/Graylog2/graylog-plugin-enterprise/issues/4091#issuecomment-1255131604.

Naming

As mentioned above, I think we should globally rename elasticsearch_... properties to indexer_..., but in addition, I suggest that those properties used to initialize the new in-database index set defaults in https://github.com/Graylog2/graylog2-server/pull/13018 (see the properties referenced in https://github.com/Graylog2/graylog2-server/blob/55f5ca2991858fb7b94be0eba1ac85fafe97b5b2/graylog2-server/src/main/java/org/graylog2/migrations/V202211021200_CreateDefaultIndexDefaultsConfig.java) also include init_default in their name to clearly indicate that those are used for index default initialization (on first boot only).

Backwards compatibility

@todvora Added backwards compatible naming support for the configuration library in https://github.com/Graylog2/JadConfig/pull/113. So, we can use the new fallbackPropertyName option to maintain support for the previous names.

Documentation updates

Note that documentation updates will also be required, since this page lists some supported configuration values: https://docs.graylog.org/docs/server-conf

Note: This content is planned for Graylog 5.1.0.

danotorrey commented 1 year ago

@bernd @todvora I created this issue as a continuation of https://github.com/Graylog2/graylog-plugin-enterprise/issues/4091#issuecomment-1255131604 for the naming changes that were discussed there. I thought we could discuss/confirm the details here before @todvora works on the naming changes.

The PR for index set defaults configuration is complete, but will take some time to review and merge.

danotorrey commented 1 year ago

FYI @kroepke @boosty

bernd commented 1 year ago

@danotorrey @todvora I think the name indexer_* is a bit too specific, but I don't have a very good suggestion either. (maybe datastore or something similar)

I agree with the _init_default suffix for config options that only set defaults once and don't have an effect once the setup has been started for the first time. :+1:

boosty commented 1 year ago

I think the name indexer_* is a bit too specific, but I don't have a very good suggestion either. (maybe datastore or something similar)

datastore sounds a bit too generic for my taste, as we also store data in MongoDB, for example.

I was wondering: Is a naming abstraction for the log store actually useful at this point?

During the time where we support both Elasticsearch and OpenSearch, how about supporting both elasticsearch_ and opensearch_ prefixes simultaneously?

Should we ever switch to another log storage, it will most likely offer different settings than we have today, at which point it might warrant its own, specific prefix (e.g. gl_log_store_).

danotorrey commented 1 year ago

I was wondering: Is a naming abstraction for the log store actually useful at this point?

@boosty That is a good point. After thinking this through, I don't think an abstraction helps right now. I think it is only really useful if we planned to support many indexers, or plan to change at a faster pace, and neither are the case right now.

During the time where we support both Elasticsearch and OpenSearch, how about supporting both elasticsearch and opensearch prefixes simultaneously?

I like this idea. I am curious what @bernd and @todvora think.

todvora commented 1 year ago

I think supporting both names will cause us some headaches in the long run. We'll have to deal with situations where both or none are defined, where they are mixed together, we'll have to document them twice. The code will be harder to read, search and maintain.

Last week we had a support ticket of a client that mixed is_master and is_leader through the nodes configuration. And we were not sure if that can be a problem. Now imagine that, but with 10 or 15 properties.

I'd prefer a clean cut and generic name that will cover both products.

boosty commented 1 year ago

Then maybe just search_ as the new prefix?

bernd commented 1 year ago

I was wondering: Is a naming abstraction for the log store actually useful at this point?

During the time where we support both Elasticsearch and OpenSearch, how about supporting both elasticsearch_ and opensearch_ prefixes simultaneously?

Should we ever switch to another log storage, it will most likely offer different settings than we have today, at which point it might warrant its own, specific prefix (e.g. gl_log_store_).

@boosty Good points. I am fine with switching from elasticsearch_ to opensearch_. But I also don't have a strong opinion. It should be clear to users what it is if we use something generic.

The name we choose should work well with the existing settings that we are going to rename:

elasticsearch_hosts 
elasticsearch_version_probe_attempts 
elasticsearch_version_probe_delay 
elasticsearch_connect_timeout 
elasticsearch_socket_timeout 
elasticsearch_idle_timeout 
elasticsearch_max_total_connections 
elasticsearch_max_total_connections_per_route 
elasticsearch_max_retries 
elasticsearch_discovery_enabled 
elasticsearch_discovery_filter 
elasticsearch_discovery_frequency 
elasticsearch_discovery_default_scheme 
elasticsearch_compression_enabled 
elasticsearch_use_expect_continue 
elasticsearch_max_docs_per_index 
elasticsearch_max_size_per_index 
elasticsearch_max_time_per_index 
elasticsearch_max_write_index_age 
elasticsearch_disable_version_check 
elasticsearch_max_number_of_indices 
elasticsearch_shards 
elasticsearch_replicas 
elasticsearch_index_prefix 
elasticsearch_template_name 
elasticsearch_analyzer 
elasticsearch_index_optimization_timeout 
elasticsearch_index_optimization_jobs 
elasticsearch_mute_deprecation_warnings

danotorrey commented 1 year ago

Please also see this related issue for adding a prefix on index set default configuration properties: https://github.com/Graylog2/graylog2-server/issues/14194

danotorrey commented 1 year ago

@dennisoelkers Do you have any input on this topic?

Graylog2 / graylog2-server