settings: deprecate unit-less TimeValue values

We have "required" units for a variety of `TimeValue` settings when they are
provided as a `String`, but unquoted values in YAML have been passed through as
Integers, where we long assumed nanosecond units. This frequently leads to
surprise (e.g., when `config.reload.interval` is set to `60`, we consume 100%
of CPU in a tight loop trying to reload and re-parse the configs every 60
nanoseconds).

By making the setting retain the TimeValue object for the entirety of its
lifecycle, we can issue a deprecation notice the first time an Integer value is
encountered. As a secondary benefit, our usage of the setting value in code
becomes more clear since we are empowered to ask `TimeValue` for a numeric
value in a specific scale.

Fixes #11803

logstash-core/lib/logstash/agent.rb

@@ -83,7 +83,7 @@ class LogStash::Agent
     end
 
     # Normalize time interval to seconds
-    @reload_interval = setting("config.reload.interval") / 1_000_000_000.0
+    @reload_interval = setting("config.reload.interval").to_seconds
 
     @collect_metric = setting("metric.collect")
 

logstash-core/lib/logstash/java_pipeline.rb

@@ -239,7 +239,7 @@ module LogStash; class JavaPipeline < JavaBasePipeline
       config_metric.gauge(:batch_size, batch_size)
       config_metric.gauge(:batch_delay, batch_delay)
       config_metric.gauge(:config_reload_automatic, settings.get("config.reload.automatic"))
-      config_metric.gauge(:config_reload_interval, settings.get("config.reload.interval"))
+      config_metric.gauge(:config_reload_interval, settings.get("config.reload.interval").to_nanos)
       config_metric.gauge(:dead_letter_queue_enabled, dlq_enabled?)
       config_metric.gauge(:dead_letter_queue_path, dlq_writer.get_path.to_absolute_path.to_s) if dlq_enabled?
       config_metric.gauge(:ephemeral_id, ephemeral_id)

logstash-core/lib/logstash/plugin.rb

@@ -65,10 +65,10 @@ class LogStash::Plugin
     @deprecation_logger = self.deprecation_logger
     # need to access settings statically because plugins are initialized in config_ast with no context.
     settings = LogStash::SETTINGS
-    @slow_logger = self.slow_logger(settings.get("slowlog.threshold.warn"),
-                                    settings.get("slowlog.threshold.info"),
-                                    settings.get("slowlog.threshold.debug"),
-                                    settings.get("slowlog.threshold.trace"))
+    @slow_logger = self.slow_logger(settings.get("slowlog.threshold.warn").to_nanos,
+                                    settings.get("slowlog.threshold.info").to_nanos,
+                                    settings.get("slowlog.threshold.debug").to_nanos,
+                                    settings.get("slowlog.threshold.trace").to_nanos)
     @params = LogStash::Util.deep_clone(params)
     # The id should always be defined normally, but in tests that might not be the case
     # In the future we may make this more strict in the Plugin API

logstash-core/lib/logstash/settings.rb

@@ -588,13 +588,22 @@ module LogStash
     end
 
     class TimeValue < Coercible
+      include LogStash::Util::Loggable
+
       def initialize(name, default, strict=true, &validator_proc)
-        super(name, ::Integer, default, strict, &validator_proc)
+        super(name, Util::TimeValue, default, strict, &validator_proc)
       end
 
       def coerce(value)
-        return value if value.is_a?(::Integer)
-        Util::TimeValue.from_value(value).to_nanos
+        if value.is_a?(::Integer)
+          deprecation_logger.deprecated("Integer value for `#{name}` does not have a time unit and will be interpreted in nanoseconds. " +
+                                        "Time units will be required in a future release of Logstash. " +
+                                        "Acceptable unit suffixes are: `d`, `h`, `m`, `s`, `ms`, `micros`, and `nanos`.")
+
+          return Util::TimeValue.new(value, :nanosecond)
+        end
+
+        Util::TimeValue.from_value(value)
       end
     end
 

logstash-core/lib/logstash/util/time_value.rb

@@ -24,9 +24,10 @@ module LogStash
       end
 
       def self.from_value(value)
-        if value.is_a?(TimeValue)
-          TimeValue.new(value.duration, value.time_unit)
-        elsif value.is_a?(::String)
+        case value
+        when TimeValue
+          return value # immutable
+        when ::String
           normalized = value.downcase.strip
           if normalized.end_with?("nanos")
             TimeValue.new(parse(normalized, 5), :nanosecond)
@@ -71,8 +72,12 @@ module LogStash
         end
       end
 
+      def to_seconds
+        self.to_nanos / 1_000_000_000.0
+      end
+
       def ==(other)
-        self.duration == other.duration and self.time_unit == other.time_unit
+        (self.duration == other.duration && self.time_unit == other.time_unit) || self.to_nanos == other.to_nanos
       end
 
       def self.parse(value, suffix)

logstash-core/spec/logstash/agent/converge_spec.rb

@@ -191,7 +191,7 @@ describe LogStash::Agent do
           end
 
           it "it will keep trying to converge" do
-            sleep(agent_settings.get("config.reload.interval") / 1_000_000_000.0 * 20) # let the interval reload a few times
+            sleep(agent_settings.get("config.reload.interval").to_seconds * 20) # let the interval reload a few times
             expect(subject.pipelines_count).to eq(0)
             expect(source_loader.fetch_count).to be > 1
           end

logstash-core/spec/logstash/settings/time_value_spec.rb

@@ -22,7 +22,8 @@ describe LogStash::Setting::TimeValue do
   subject { described_class.new("option", "-1") }
   describe "#set" do
     it "should coerce the default correctly" do
-      expect(subject.value).to eq(LogStash::Util::TimeValue.new(-1, :nanosecond).to_nanos)
+      expect(subject.value).to eq(LogStash::Util::TimeValue.new(-1, :nanosecond))
+      expect(subject.value.to_nanos).to eq(-1)
     end
 
     context "when a value is given outside of possible_values" do
@@ -33,14 +34,22 @@ describe LogStash::Setting::TimeValue do
     context "when a value is given as a time value" do
       it "should set the value" do
         subject.set("18m")
-        expect(subject.value).to eq(LogStash::Util::TimeValue.new(18, :minute).to_nanos)
+        expect(subject.value).to eq(LogStash::Util::TimeValue.new(18, :minute))
+        expect(subject.value.to_nanos).to eq(18 * 60 * 1_000_000_000)
       end
     end
 
     context "when a value is given as a nanosecond" do
+      let(:deprecation_logger_stub) { double("DeprecationLogger").as_null_object }
+      before(:each) do
+        allow(subject).to receive(:deprecation_logger).and_return(deprecation_logger_stub)
+      end
       it "should set the value" do
         subject.set(5)
-        expect(subject.value).to eq(LogStash::Util::TimeValue.new(5, :nanosecond).to_nanos)
+        expect(subject.value).to eq(LogStash::Util::TimeValue.new(5, :nanosecond))
+        expect(subject.value.to_nanos).to eq(5)
+
+        expect(deprecation_logger_stub).to have_received(:deprecated).with(/units will be required/).once
       end
     end
   end

x-pack/lib/monitoring/monitoring.rb

@@ -207,8 +207,8 @@ module LogStash
       private
       def retrieve_collection_settings(settings, prefix="")
         opt = {}
-        opt[:collection_interval] = settings.get("#{prefix}monitoring.collection.interval")
-        opt[:collection_timeout_interval] = settings.get("#{prefix}monitoring.collection.timeout_interval")
+        opt[:collection_interval] = settings.get("#{prefix}monitoring.collection.interval").to_nanos
+        opt[:collection_timeout_interval] = settings.get("#{prefix}monitoring.collection.timeout_interval").to_nanos
         opt[:extended_performance_collection] = settings.get("#{prefix}monitoring.collection.pipeline.details.enabled")
         opt[:config_collection] = settings.get("#{prefix}monitoring.collection.config.enabled")
         opt

x-pack/spec/config_management/bootstrap_check_spec.rb

@@ -40,7 +40,7 @@ describe LogStash::ConfigManagement::BootstrapCheck do
 
     it "sets the `config.reload.interval`" do
       expect { subject.check(settings) }
-        .to change { settings.get_value("config.reload.interval") }.to(interval * 1_000_000_000)
+        .to change { settings.get_value("config.reload.interval").to_nanos }.to(interval * 1_000_000_000)
     end
 
 

x-pack/spec/config_management/extension_spec.rb

@@ -29,7 +29,7 @@ describe LogStash::ConfigManagement::Extension do
     describe "#additionals_settings" do
       define_settings(
         "xpack.management.enabled" => [LogStash::Setting::Boolean, false],
-        "xpack.management.logstash.poll_interval" => [LogStash::Setting::TimeValue, 5000000000],
+        "xpack.management.logstash.poll_interval" => [LogStash::Setting::TimeValue, LogStash::Util::TimeValue.from_value("5s")],
         "xpack.management.pipeline.id" => [LogStash::Setting::ArrayCoercible, ["main"]],
         "xpack.management.elasticsearch.hosts" => [LogStash::Setting::ArrayCoercible, ["https://localhost:9200"]],
         "xpack.management.elasticsearch.username" => [LogStash::Setting::String, "logstash_system"],