refactor: move timing info to extra access log fields logic

jobs/gorouter/spec

@@ -328,7 +328,7 @@ properties:
       this property and operators have to explicitly enable them. This is done to prevent breaking
       log parsing in existing setups by the introduction of new fields. Does not affect stdout /
       stderr logs.
-      Available fields are: local_address
+      Available fields are: backend_time, dial_time, dns_time, failed_attempts, failed_attempts_time, local_address, tls_time
     default: []
   router.logging.format.timestamp:
     description: |

jobs/gorouter/templates/gorouter.yml.erb

@@ -391,11 +391,19 @@ if !['rfc3339', 'deprecated', 'unix-epoch'].include?(p('router.logging.format.ti
   raise "'#{p('router.logging.format.timestamp')}' is not a valid timestamp format for the property 'router.logging.format.timestamp'. Valid options are: 'rfc3339', 'deprecated', and 'unix-epoch'."
 end
 
+
+extra_access_log_fields = []
+# Support `enable_log_attempts_details` which was internally migrated to the new dynamic system.
+if p('router.enable_log_attempts_details')
+  extra_access_log_fields.concat(['failed_attempts', 'failed_attempts_time', 'dns_time', 'dial_time', 'tls_time', 'backend_time'])
+end
+
 if_p('router.logging.extra_access_log_fields') do |extra_fields|
-  valid_extra_log_fields=['local_address']
+  valid_extra_log_fields=['backend_time', 'dial_time', 'dns_time', 'failed_attempts', 'failed_attempts_time', 'local_address', 'tls_time']
   if !extra_fields.all? { |el| valid_extra_log_fields.include?(el) }
     raise "router.logging.extra_access_log_fields (#{extra_fields}) contains invalid values, valid are #{valid_extra_log_fields}"
   end
+  extra_access_log_fields.concat(extra_fields)
 end
 
 
@@ -410,13 +418,12 @@ params['logging'] = {
   'syslog_addr' => p('router.logging.syslog_addr'),
   'syslog_network' => p('router.logging.syslog_network'),
   'level' => p('router.logging_level'),
-  'extra_access_log_fields' => p('router.logging.extra_access_log_fields'),
+  'extra_access_log_fields' => extra_access_log_fields,
   'loggregator_enabled' => true,
   'metron_address' => "localhost:#{p('metron.port')}",
   'disable_log_forwarded_for' => p('router.disable_log_forwarded_for'),
   'disable_log_source_ip' => p('router.disable_log_source_ip'),
   'redact_query_params' => p('router.redact_query_parameters'),
-  'enable_attempts_details' => p('router.enable_log_attempts_details'),
   'format' => {
     'timestamp' => timestamp_format,
   },

spec/gorouter_templates_spec.rb

@@ -1334,8 +1334,8 @@
         end
 
         context 'when enable_detailed_attempts_logging is not provided' do
-          it 'it defaults to false' do
-            expect(parsed_yaml['logging']['enable_attempts_details']).to eq(false)
+          it 'it does not set the correspoding extra access log values' do
+            expect(parsed_yaml['logging']['extra_access_log_fields']).not_to include(['backend_time', 'dial_time', 'dns_time', 'failed_attempts', 'failed_attempts_time', 'tls_time'])
           end
         end
 
@@ -1344,7 +1344,7 @@
             deployment_manifest_fragment['router']['enable_log_attempts_details'] = true
           end
           it 'it properly sets the value' do
-            expect(parsed_yaml['logging']['enable_attempts_details']).to eq(true)
+            expect(parsed_yaml['logging']['extra_access_log_fields']).to eq(['failed_attempts', 'failed_attempts_time', 'dns_time', 'dial_time', 'tls_time', 'backend_time'])
           end
         end
 
@@ -1365,7 +1365,7 @@
             end
 
             it 'raises an error' do
-              expect { parsed_yaml }.to raise_error(RuntimeError, 'router.logging.extra_access_log_fields (["foobar"]) contains invalid values, valid are ["local_address"]')
+              expect { parsed_yaml }.to raise_error(RuntimeError, /router\.logging\.extra_access_log_fields \(\["foobar"\]\) contains invalid values, valid are .*/)
             end
           end
 
@@ -1375,11 +1375,22 @@
             end
 
             it 'still raises an error' do
-              expect { parsed_yaml }.to raise_error(RuntimeError, 'router.logging.extra_access_log_fields (["local_address", "foobar"]) contains invalid values, valid are ["local_address"]')
+              expect { parsed_yaml }.to raise_error(RuntimeError, /router\.logging\.extra_access_log_fields \(\["local_address", "foobar"\]\) contains invalid values, valid are .*/)
             end
           end
         end
 
+        context 'when enable_detailed_attempts_logging and extra_access_log_fields is set' do
+          before do
+            deployment_manifest_fragment['router']['enable_log_attempts_details'] = true
+            deployment_manifest_fragment['router']['logging'] = { 'extra_access_log_fields' => ['local_address'] }
+          end
+
+          it 'puts the extra_access_log_fields to the end to preserve order' do
+            expect(parsed_yaml['logging']['extra_access_log_fields']).to eq(['failed_attempts', 'failed_attempts_time', 'dns_time', 'dial_time', 'tls_time', 'backend_time', 'local_address'])
+          end
+        end
+
         context 'when access log streaming via syslog is enabled' do
           before do
             deployment_manifest_fragment['router']['write_access_logs_locally'] = false