Fix ambiguous duration units and add format check (#12225)

- ambiguous value-based std::chrono::{clock_type}::duration(value) constructors
  result in stdlib implementation specific default time units which are
  hard to read and potentially different on different platforms
- this change removes any instances of these ambiguous constructions and adds
  a format check to prevent them; developers should specify an explicit unit of time
- we explicitly saw this issue in #11915 where
  the assumed duration unit was different on Windows, causing test failures

Additional Description:
Risk Level: Low
Testing: Adds format check test and adjust existing unit tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

include/envoy/event/timer.h

@@ -81,6 +81,10 @@ class TimeSystem : public TimeSource {
   ~TimeSystem() override = default;
 
   using Duration = MonotonicTime::duration;
+  using Nanoseconds = std::chrono::nanoseconds;
+  using Microseconds = std::chrono::microseconds;
+  using Milliseconds = std::chrono::milliseconds;
+  using Seconds = std::chrono::seconds;
 
   /**
    * Creates a timer factory. This indirection enables thread-local timer-queue management,

test/extensions/filters/network/kafka/broker/BUILD

@@ -26,6 +26,7 @@ envoy_extension_cc_test(
     srcs = ["filter_unit_test.cc"],
     extension_name = "envoy.filters.network.kafka_broker",
     deps = [
+        "//include/envoy/event:timer_interface",
         "//source/extensions/filters/network/kafka:kafka_broker_filter_lib",
         "//test/mocks/network:network_mocks",
         "//test/mocks/stats:stats_mocks",

test/extensions/filters/network/kafka/broker/filter_unit_test.cc

@@ -1,3 +1,5 @@
+#include "envoy/event/timer.h"
+
 #include "extensions/filters/network/kafka/broker/filter.h"
 #include "extensions/filters/network/kafka/external/requests.h"
 
@@ -218,7 +220,7 @@ TEST_F(KafkaMetricsFacadeImplUnitTest, ShouldRegisterRequest) {
 
   EXPECT_CALL(*request_metrics_, onRequest(api_key));
 
-  MonotonicTime time_point{MonotonicTime::duration(1234)};
+  MonotonicTime time_point{Event::TimeSystem::Milliseconds(1234)};
   EXPECT_CALL(time_source_, monotonicTime()).WillOnce(Return(time_point));
 
   // when
@@ -248,10 +250,10 @@ TEST_F(KafkaMetricsFacadeImplUnitTest, ShouldRegisterResponse) {
   const int32_t correlation_id = 1234;
   AbstractResponseSharedPtr response = std::make_shared<MockResponse>(api_key, correlation_id);
 
-  MonotonicTime request_time_point{MonotonicTime::duration(1234000000)};
+  MonotonicTime request_time_point{Event::TimeSystem::Milliseconds(1234)};
   testee_.getRequestArrivalsForTest()[correlation_id] = request_time_point;
 
-  MonotonicTime response_time_point{MonotonicTime::duration(2345000000)};
+  MonotonicTime response_time_point{Event::TimeSystem::Milliseconds(2345)};
 
   EXPECT_CALL(*response_metrics_, onResponse(api_key, 1111));
   EXPECT_CALL(time_source_, monotonicTime()).WillOnce(Return(response_time_point));

tools/code_format/check_format.py

@@ -103,6 +103,7 @@
 HISTOGRAM_SI_SUFFIX_REGEX = re.compile(r"(?<=HISTOGRAM\()[a-zA-Z0-9_]+_(b|kb|mb|ns|us|ms|s)(?=,)")
 TEST_NAME_STARTING_LOWER_CASE_REGEX = re.compile(r"TEST(_.\(.*,\s|\()[a-z].*\)\s\{")
 EXTENSIONS_CODEOWNERS_REGEX = re.compile(r'.*(extensions[^@]*\s+)(@.*)')
+DURATION_VALUE_REGEX = re.compile(r'\b[Dd]uration\(([0-9.]+)')
 
 # yapf: disable
 PROTOBUF_TYPE_ERRORS = {
@@ -620,6 +621,12 @@ def checkSourceLine(line, file_path, reportError):
        "std::chrono::system_clock::now" in line or "std::chrono::steady_clock::now" in line or \
        "std::this_thread::sleep_for" in line or hasCondVarWaitFor(line):
       reportError("Don't reference real-world time sources from production code; use injection")
+  duration_arg = DURATION_VALUE_REGEX.search(line)
+  if duration_arg and duration_arg.group(1) != "0" and duration_arg.group(1) != "0.0":
+    # Matching duration(int-const or float-const) other than zero
+    reportError(
+        "Don't use ambiguous duration(value), use an explicit duration type, e.g. Event::TimeSystem::Milliseconds(value)"
+    )
   if not allowlistedForRegisterFactory(file_path):
     if "Registry::RegisterFactory<" in line or "REGISTER_FACTORY" in line:
       reportError("Don't use Registry::RegisterFactory or REGISTER_FACTORY in tests, "

tools/code_format/check_format_test_helper.py

@@ -166,6 +166,10 @@ def runChecks():
       "Don't reference real-world time sources from production code; use injection")
   errors += checkUnfixableError("real_time_source.cc", real_time_inject_error)
   errors += checkUnfixableError("real_time_system.cc", real_time_inject_error)
+  errors += checkUnfixableError(
+      "duration_value.cc",
+      "Don't use ambiguous duration(value), use an explicit duration type, e.g. Event::TimeSystem::Milliseconds(value)"
+  )
   errors += checkUnfixableError("system_clock.cc", real_time_inject_error)
   errors += checkUnfixableError("steady_clock.cc", real_time_inject_error)
   errors += checkUnfixableError(
@@ -282,6 +286,7 @@ def runChecks():
       "term absl::make_unique< should be replaced with standard library term std::make_unique<")
 
   errors += checkFileExpectingOK("real_time_source_override.cc")
+  errors += checkFileExpectingOK("duration_value_zero.cc")
   errors += checkFileExpectingOK("time_system_wait_for.cc")
   errors += checkFileExpectingOK("clang_format_off.cc")
   return errors

tools/testdata/check_format/duration_value.cc

@@ -0,0 +1,9 @@
+#include <chrono>
+
+namespace Envoy {
+
+std::chrono::duration<long int, std::nano> foo() {
+  return std::chrono::steady_clock::duration(12345);
+}
+
+} // namespace Envoy

tools/testdata/check_format/duration_value_zero.cc

@@ -0,0 +1,13 @@
+#include <chrono>
+
+namespace Envoy {
+
+std::chrono::duration<long int, std::nano> foo_int() {
+  return std::chrono::steady_clock::duration(0);
+}
+
+std::chrono::duration<long int, std::nano> foo_decimal() {
+  return std::chrono::steady_clock::duration(0.0);
+}
+
+} // namespace Envoy