[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6620: Adding access.log to Traffic Monitor

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #6620:
URL: https://github.com/apache/trafficcontrol/pull/6620#discussion_r824158150



##########
File path: traffic_monitor/build/traffic_monitor.logrotate
##########
@@ -34,3 +34,13 @@
         rotate 5
         copytruncate
 }
+
+/opt/traffic_monitor/var/log/access.log {
+        compress
+        maxage 30
+        missingok
+        nomail
+        size 10M
+        rotate 5
+        copytruncate
+}

Review comment:
       missing newline at end of file

##########
File path: traffic_monitor/config/config.go
##########
@@ -132,6 +135,17 @@ func (c Config) WarningLog() log.LogLocation { return log.LogLocation(c.LogLocat
 func (c Config) InfoLog() log.LogLocation    { return log.LogLocation(c.LogLocationInfo) }
 func (c Config) DebugLog() log.LogLocation   { return log.LogLocation(c.LogLocationDebug) }
 func (c Config) EventLog() log.LogLocation   { return log.LogLocation(c.LogLocationEvent) }
+func (c Config) AccessLog() log.LogLocation  { return log.LogLocation(c.LogLocationAccess) }
+
+func GetAccessLogWriters(cfg Config) (io.WriteCloser, error) {
+	accessLoc := cfg.AccessLog()
+
+	accessW, err := log.GetLogWriter(accessLoc)
+	if err != nil {
+		return nil, fmt.Errorf("getting log access writer %v: %v", accessLoc, err)
+	}
+	return accessW, nil
+}
 
 // DefaultConfig is the default configuration for the application, if no configuration file is given, or if a given config setting doesn't exist in the config file.
 var DefaultConfig = Config{

Review comment:
       Should we update `DefaultConfig` to set it to `LogLocationNull`?

##########
File path: traffic_monitor/config/config.go
##########
@@ -132,6 +135,17 @@ func (c Config) WarningLog() log.LogLocation { return log.LogLocation(c.LogLocat
 func (c Config) InfoLog() log.LogLocation    { return log.LogLocation(c.LogLocationInfo) }
 func (c Config) DebugLog() log.LogLocation   { return log.LogLocation(c.LogLocationDebug) }
 func (c Config) EventLog() log.LogLocation   { return log.LogLocation(c.LogLocationEvent) }
+func (c Config) AccessLog() log.LogLocation  { return log.LogLocation(c.LogLocationAccess) }
+
+func GetAccessLogWriters(cfg Config) (io.WriteCloser, error) {

Review comment:
       This being plural makes it seem like it returns a slice of writers rather than a single writer -- should we singularize it?

##########
File path: traffic_monitor/datareq/datareq.go
##########
@@ -286,24 +287,47 @@ func WrapAgeErr(errorCount threadsafe.Uint, f func() ([]byte, time.Time, error),
 	}
 }
 
+func accessLogTime(t time.Time) float64 {
+	return float64(t.Unix()) + (float64(t.Nanosecond()) / 1e9)
+}
+
+func accessLogStr(
+	timestamp time.Time, // prefix
+	remoteAddress string, // addr
+	reqMethod string, // method
+	reqPath string, // path
+	reqProto string, // proto
+	statusCode int, // statusCode
+	respSize int, // respSize
+	reqServeTimeMs int, // reqServeTimeMs
+	userAgent string, // uas
+) string {
+	return fmt.Sprintf("%.3f addr=%s method=%s path=%s proto=%s statusCode=%d respSize=%d reqServeTimeMs=%d uas=%s", accessLogTime(timestamp), remoteAddress, reqMethod, reqPath, reqProto, statusCode, respSize, reqServeTimeMs, userAgent)

Review comment:
       Since this line is really long, should we put each parameter on its own line?
   
   Also, I think it makes sense to mimic ATS logging fields here since that is what we do for other ATC components (TR at least):
   ```
   %.3f chi=%s cqhm=%s url=\"%s?%s\" cqhv=%s pssc=%d b=%d ttms=%d uas=\"<user agent>\"
   ```
   for the `url` field, notice I've quoted it and made it `%s?%s` which should consist of `r.URL.Path` and `r.URL.RawQuery` so that we have the full URL. Also, the uas field should be quoted as well.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org