[GitHub] [trafficcontrol] rob05c commented on a diff in pull request #6960: add t3c-tail sub-app

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

rob05c commented on code in PR #6960:
URL: https://github.com/apache/trafficcontrol/pull/6960#discussion_r923611437


##########
cache-config/t3c-tail/t3c-tail.go:
##########
@@ -0,0 +1,131 @@
+package main
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"regexp"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/t3cutil"
+	"github.com/apache/trafficcontrol/lib/go-log"
+
+	"github.com/nxadm/tail"
+	"github.com/pborman/getopt/v2"
+)
+
+const AppName = "t3c-tail"
+
+// Version is the application version.
+// This is overwritten by the build with the current project version.
+var Version = "0.4"
+
+// GitRevision is the git revision the application was built from.
+// This is overwritten by the build with the current project version.
+var GitRevision = "nogit"
+
+//default time out is 15 seconds, if not included in json input.
+var timeOutSeconds = 15
+
+func main() {
+	version := getopt.BoolLong("version", 'V', "Print version information and exit.")
+	help := getopt.BoolLong("help", 'h', "Print usage information and exit")
+	getopt.Parse()
+
+	log.Init(os.Stderr, os.Stderr, os.Stderr, os.Stderr, os.Stderr)
+
+	if *help {
+		fmt.Println(usageStr())
+		os.Exit(0)
+	} else if *version {
+		fmt.Println(t3cutil.VersionStr(AppName, Version, GitRevision))
+		os.Exit(0)
+	}
+
+	tailCfg := &t3cutil.TailCfg{}
+	if err := json.NewDecoder(os.Stdin).Decode(tailCfg); err != nil {

Review Comment:
   Would you object to making these settings (`file`, `match`, `end-match`, `timeout`) flags instead of `stdin`? 
   I think flags are more common and expected for Linux/POSIX apps, when they're static values not at risk of being arbitrarily long and overflowing Linux command buffers.



##########
cache-config/t3c-tail/t3c-tail.go:
##########
@@ -0,0 +1,131 @@
+package main
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"regexp"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/t3cutil"
+	"github.com/apache/trafficcontrol/lib/go-log"
+
+	"github.com/nxadm/tail"
+	"github.com/pborman/getopt/v2"
+)
+
+const AppName = "t3c-tail"
+
+// Version is the application version.
+// This is overwritten by the build with the current project version.
+var Version = "0.4"
+
+// GitRevision is the git revision the application was built from.
+// This is overwritten by the build with the current project version.
+var GitRevision = "nogit"
+
+//default time out is 15 seconds, if not included in json input.
+var timeOutSeconds = 15
+
+func main() {
+	version := getopt.BoolLong("version", 'V', "Print version information and exit.")
+	help := getopt.BoolLong("help", 'h', "Print usage information and exit")
+	getopt.Parse()
+
+	log.Init(os.Stderr, os.Stderr, os.Stderr, os.Stderr, os.Stderr)
+
+	if *help {
+		fmt.Println(usageStr())
+		os.Exit(0)
+	} else if *version {
+		fmt.Println(t3cutil.VersionStr(AppName, Version, GitRevision))
+		os.Exit(0)
+	}
+
+	tailCfg := &t3cutil.TailCfg{}
+	if err := json.NewDecoder(os.Stdin).Decode(tailCfg); err != nil {

Review Comment:
   Also, mind making the timeout in milliseconds, e.g. `--timeout-ms`? Users might want sub-second timeouts



##########
CHANGELOG.md:
##########
@@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Added layered profile feature to 4.0 for `GET` /deliveryservices/{id}/servers/ and /deliveryservices/{id}/servers/eligible.
 - Change to t3c regex_revalidate so that STALE is no longer explicitly added for default revalidate rule for ATS version backwards compatibility.
 - Change to t3c diff to flag a config file for replacement if owner/group settings are not `ats` [#6879](https://github.com/apache/trafficcontrol/issues/6879).
+- Added a sub-app t3c-tail to tail diags.log and caputre output when t3c reloads and restarts trafficserver  

Review Comment:
   Typo, `caputre`



##########
cache-config/t3c-apply/torequest/cmd.go:
##########
@@ -274,6 +275,36 @@ func sendUpdate(cfg config.Cfg, configApplyTime, revalApplyTime *time.Time, conf
 	return nil
 }
 
+//doTail calls t3c-tail and will run a tail on the log file provided with string for a regex to
+//maatch on default is .* endMatch will make t3c-tail exit when a pattern is matched otherwise
+//a timeout in a given number of seconds will occur.
+func doTail(cfg config.Cfg, file string, logMatch string, endMatch string, timeoutSeconds int) error {
+	tailData := t3cutil.TailCfg{
+		File:     &file,
+		LogMatch: &logMatch,
+		EndMatch: &endMatch,
+		TimeOut:  &timeoutSeconds,
+	}
+	tailInput, err := json.Marshal(&tailData)
+	if err != nil {
+		return fmt.Errorf("error loading json input")
+	}
+	stdOut, stdErr, code := t3cutil.DoInput(tailInput, `t3c-tail`, "")
+	if code > 1 {
+		return fmt.Errorf("t3c-tail returned error code %v stdout '%v' stderr '%v'", code, string(stdOut), string(stdErr))
+	}
+	logSubApp(`t3c-tail`, stdErr)
+
+	stripDate := regexp.MustCompile(`\[\w{3}\s{1,2}\d{1,2}\s\d{2}:\d{2}:\d{2}\.\d{3}\]\s`)

Review Comment:
   Would you object to moving this to a variable outside the function? `MustCompile` will cause the app to panic and crash if it fails. It _shouldn't_ ever fail, but moving it as a `var` outside the func will make it crash as soon as the application starts, instead of in the middle of a run, which should be easier to debug.



##########
cache-config/t3c-apply/torequest/torequest.go:
##########
@@ -47,6 +47,15 @@ const (
 	UpdateTropsFailed     UpdateStatus = 3
 )
 
+const (
+	TailDiagsLog       = "/opt/trafficserver/var/log/trafficserver/diags.log"

Review Comment:
   Not everyone installs to `/opt`. `t3c-apply/config` will attempt to find the install dir, and put it in `Cfg.TsHome`. Can this be changed to the relative `var/log/trafficserver/diags.log` and the things that use it changed to `filepath.Join(cfg.TsHome, TailDiagsLogRelative)`?



##########
cache-config/t3c-tail/t3c-tail.go:
##########
@@ -0,0 +1,131 @@
+package main
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"encoding/json"
+	"fmt"
+	"os"
+	"regexp"
+	"time"
+
+	"github.com/apache/trafficcontrol/cache-config/t3cutil"
+	"github.com/apache/trafficcontrol/lib/go-log"
+
+	"github.com/nxadm/tail"
+	"github.com/pborman/getopt/v2"
+)
+
+const AppName = "t3c-tail"
+
+// Version is the application version.
+// This is overwritten by the build with the current project version.
+var Version = "0.4"
+
+// GitRevision is the git revision the application was built from.
+// This is overwritten by the build with the current project version.
+var GitRevision = "nogit"
+
+//default time out is 15 seconds, if not included in json input.
+var timeOutSeconds = 15

Review Comment:
   Mind renaming this `defaultTimeoutSeconds`, just so it's clear it's a default, not always the timeout?
   
   Also, the comment needs to start with the variable name for GoDoc, e.g. ` // defaultTimeoutSeconds time out is 15 seconds, if not included in json input.
   
   (Technically unexported symbols aren't used by GoDoc, but we try to keep unexported symbols GoDoc-compliant for ease of reading, and so they're automatically compliant if they're changed to be exported in the future.)



##########
cache-config/t3cutil/t3cutil.go:
##########
@@ -43,6 +43,13 @@ type ATSConfigFile struct {
 	Warnings    []string `json:"warnings"`
 }
 
+type TailCfg struct {
+	File     *string `json:"file"`
+	LogMatch *string `json:"logMatch"`
+	EndMatch *string `json:"endMatch"`
+	TimeOut  *int    `json:"timeOut"`

Review Comment:
   Mind adding the units to the name here, just so it's easier for readers to see? E.g. `TimeoutSeconds` or `TimeoutMS`



-- 
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