WIP: Add diagnostic session service #1578

desa · 2017-09-20T18:56:33Z

No description provided.

desa · 2017-09-28T19:21:14Z

@nathanielc I still need to add a few tests, but for the time it should be reviewable at this point.

nathanielc

Looks great.

I have made a few comments on how to deal with the fact that this streams forever.

nathanielc · 2017-09-28T21:24:41Z

client/v1/client.go

@@ -659,6 +660,39 @@ func (c *Client) Do(req *http.Request, result interface{}, codes ...int) (*http.
 	return resp, nil
 }

+func (c *Client) Logs(w io.Writer, q map[string]string) error {


This really isn't too useful since it never returns, unless there is an error.

We should add a context.Context argument so users can cancel the request if they like.

nathanielc · 2017-09-28T21:26:22Z

cmd/kapacitor/main.go

+	if len(args) != 1 {
+		return errors.New("must provide task ID.")
+	}
+	err := cli.Logs(os.Stdout, map[string]string{"task": args[0]})


Should we setup a signal handler here to cancel the context and cleanly as possible exit?

yup. fixed.

nathanielc · 2017-09-28T21:27:49Z

cmd/kapacitor/main.go

@@ -2246,3 +2258,48 @@ func doBackup(args []string) error {
 	}
 	return nil
 }
+
+func watchUsage() {
+	var u = `Usage: kapacitor watch <output file>


I think writing to Stdout is fine, no need for an output file. Also allowing optional list of key value pairs after the task id would be nice. That way you could filter down by node or whatever within a task easily.

typo :) fixed.

nathanielc · 2017-09-28T21:28:12Z

cmd/kapacitor/main.go

+		}
+		m[pair[0]] = pair[1]
+	}
+	err := cli.Logs(os.Stdout, m)


Same here I think we want to catch signals and cancel the context.

nathanielc · 2017-09-28T21:28:34Z

server/server.go

@@ -421,6 +423,14 @@ func (s *Server) appendTaskStoreService() {
 	s.AppendService("task_store", srv)
 }

+func (s *Server) appendSessionService() {
+	// TODO: add diagnostic


The irony :)

nathanielc · 2017-09-28T21:29:30Z

services/diagnostic/api.go

+			Method:      "GET",
+			Pattern:     sessionsPath,
+			HandlerFunc: s.handleSessions,
+			NoGzip:      true,


A comment here explaining why these flag are set would be helpful, since it took us quite a while to figure out why they are needed.

nathanielc · 2017-09-28T21:30:00Z

services/diagnostic/api.go

+}
+
+func (s *SessionService) Close() error {
+	return nil


In close you should remove the routes from the HTTPD service.

nathanielc · 2017-09-28T21:33:32Z

services/diagnostic/sessions.go

+	defer kv.mu.Unlock()
+
+	flusher, ok := w.(http.Flusher)
+	var wf WriteFlusher = &noopWriteFlusher{w: w}


You could put this creation of the noopWriteFlusher in the else block. It would save an allocation :)

nathanielc · 2017-09-28T21:35:33Z

services/diagnostic/sessions.go

+	flusher, ok := w.(http.Flusher)
+	var wf WriteFlusher = &noopWriteFlusher{w: w}
+	if ok {
+		wf = &httpWriteFlusher{w: w, f: flusher}


You shouldn't need the type httpWriteFlusher, as w is already a writer and you have asserted that its a flusher, so you can now just type assert it to be a WriteFlusher interface.

wf = w.(WriteFlusher)

Actually you can skip the direct flusher check and just check it its a WriteFlusher to begin with.

nathanielc · 2017-09-29T20:17:16Z

cmd/kapacitor/main.go

+
+	Examples:
+
+		$ kapacitor logs mytask


These examples are wrong

nathanielc · 2017-09-29T20:18:36Z

cmd/kapacitor/main.go

+		done = true
+	}()
+
+	if err := cli.Logs(ctx, os.Stdout, m); err != nil && !done {


This creates a race on the done variable.

nathanielc · 2017-09-29T20:19:15Z

cmd/kapacitor/main.go

+}
+
+func doLogs(args []string) error {
+	m := map[string]string{}


Should we create a helper function that both doLogs and doWatch use so that this logic is not duplicated?

nathanielc · 2017-09-29T20:19:28Z

cmd/kapacitor/main.go

+		done = true
+	}()
+
+	if err := cli.Logs(ctx, os.Stdout, m); err != nil && !done {


Again same race on done.

desa · 2017-09-29T21:48:32Z

@nathanielc made the changes you suggested.

desa · 2017-10-02T13:20:06Z

@nathanielc made changes you suggested

desa · 2017-10-05T18:21:50Z

@nathanielc anything else here that needs to be done?

nathanielc · 2017-10-05T18:34:42Z

cmd/kapacitor/main.go

+	ctx, cancel := context.WithCancel(context.Background())
+	done := false
+	sigs := make(chan os.Signal, 1)
+	signal.Notify(sigs, syscall.SIGINT)


This should read like this https://github.com/influxdata/kapacitor/blob/master/cmd/kapacitord/main.go#L92

desa · 2017-10-06T15:41:12Z

@nathanielc Just rebased. Should be good to merge on your signal.

nathanielc · 2017-10-06T16:03:26Z

@desa The server shutsdown if an invalid TICkscript is provided. We need the soft behavior, i.e. keep running if at all possible. A typo is a script should not cause the server to shutdown.

We can still fail to start if there is an invalid script, but should not shutdown if it fails to reload.

desa · 2017-10-06T16:11:12Z

@nathanielc Fixed. Removed the logic for hard errors previously, but forgot to ignore the error it returned.

desa · 2017-10-06T16:12:12Z

whoops just realized that I think the previous comment applies to the other PR, or does

The server shutsdown if an invalid TICkscript is provided. We need the soft behavior, i.e. keep running if at all possible. A typo is a script should not cause the server to shutdown.

apply to the logging stuff?

nathanielc · 2017-10-06T16:23:46Z

Oops, yeah I commented on the wrong one :-P

nathanielc · 2017-10-06T16:43:43Z

@desa Now actually testing this PR...

How can we make it easy to get logs for all levels greater than info?

nathanielc · 2017-10-06T17:05:28Z

Testing this out I realized we haven't updated the bash completion scripts for the new CLI options.

nathanielc · 2017-10-06T17:09:21Z

I know this isn't directly related but why is starting a task warn?

ts=2017-10-06T11:06:53.850-06:00 lvl=warn msg="starting task" service=kapacitor task_master=main task=log

Looking around I see quite a few things are warns. In general I don't think we need warns. In fact I'd be fine if we removed it entirely. Maybe we should open up a separate PR for remove warns?

desa · 2017-10-06T19:36:37Z

I know this isn't directly related but why is starting a task warn?

I think this is an issue with the sessions logger. I fixed it. It should be debug.

Maybe we should open up a separate PR for remove warns?

I'll add another PR for removing warns in general.

@nathanielc added completions.

desa · 2017-10-06T20:35:40Z

@nathanielc just rebased. I think things should be good to go now. (hopefully :))

desa · 2017-10-09T16:48:52Z

@nathanielc fixed issue with kapacitor logs lvl=error+ not correctly logging levels.

desa · 2017-10-09T17:03:37Z

going to add some tests related to the log level setting

desa · 2017-10-10T14:54:49Z

@nathanielc added test for issue you uncovered

Rename constants Add pruning of sessions Add changelog entry Close channel on service close Restructure everything into a single package Fix tests SQUASH: saving state Get logs via the API Switch to streaming http Add JSON logging Add table test template for json logger Add tests for JSON logs Add todo comment Fixup todo comment Add diagnostic session changes Add kapacitor CLI commands for logs Change dao to sessions Reorganize code Reorganize Add context to Log function Fix kapacitor subcommand logic Address issues from PR Add diagnostic to session service Remove unreachable code Remove deadlock introduced by using diagnostic Remove TODOs Add test for content type Fix race on watch/logs subcommand And pull out similar logic Fix notify signals Fix bad usage of warn Expose loglevels with + in kv pairs Update bash-completion Make logLevelFromName case insensitive Add tests for handleSessions http hander

desa force-pushed the md-log-api branch 3 times, most recently from 7fc5a7f to 013f733 Compare September 21, 2017 18:16

ghost assigned desa Sep 26, 2017

ghost added the in progress label Sep 26, 2017

desa requested a review from nathanielc September 28, 2017 19:20

nathanielc suggested changes Sep 28, 2017

View reviewed changes

desa force-pushed the md-log-api branch from 9bb45f4 to 55a95a7 Compare September 29, 2017 15:24

nathanielc suggested changes Sep 29, 2017

View reviewed changes

nathanielc suggested changes Oct 5, 2017

View reviewed changes

desa force-pushed the md-log-api branch 2 times, most recently from 27a6512 to a71dc4f Compare October 6, 2017 15:40

desa closed this Oct 6, 2017

ghost removed the in progress label Oct 6, 2017

desa reopened this Oct 6, 2017

ghost added the in progress label Oct 6, 2017

desa force-pushed the md-log-api branch from 4007a40 to 5b83322 Compare October 6, 2017 19:07

desa force-pushed the md-log-api branch from 7967288 to 319773d Compare October 6, 2017 20:35

desa force-pushed the md-log-api branch from 796084a to 76e711c Compare October 9, 2017 19:25

nathanielc approved these changes Oct 10, 2017

View reviewed changes

desa force-pushed the md-log-api branch from 54185d6 to 97396b3 Compare October 10, 2017 15:18

desa merged commit 953362a into master Oct 10, 2017

desa deleted the md-log-api branch October 10, 2017 15:24

ghost removed the in progress label Oct 10, 2017

WIP: Add diagnostic session service #1578

WIP: Add diagnostic session service #1578

Conversation

desa commented Sep 20, 2017

desa commented Sep 28, 2017

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Sep 29, 2017

desa commented Oct 2, 2017

desa commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Oct 6, 2017

nathanielc commented Oct 6, 2017 • edited Loading

desa commented Oct 6, 2017

desa commented Oct 6, 2017 • edited Loading

nathanielc commented Oct 6, 2017

nathanielc commented Oct 6, 2017

nathanielc commented Oct 6, 2017

nathanielc commented Oct 6, 2017

desa commented Oct 6, 2017

desa commented Oct 6, 2017

desa commented Oct 9, 2017

desa commented Oct 9, 2017

desa commented Oct 10, 2017

nathanielc commented Oct 6, 2017 •

edited

Loading

desa commented Oct 6, 2017 •

edited

Loading