gui: Add log viewer (fixes #2644) #4604

AudriusButkevicius · 2017-12-17T17:49:00Z

Just because there are a ton of people struggling to set env vars.
Perhaps this should live in advanced settings, and perhaps we should have a button to view the log.

Just because there are a ton of people struggling to set env vars. Perhaps this should live in advanced settings, and perhaps we should have a button to view the log.

canton7 · 2017-12-17T18:21:25Z

Awesome! A big +1 to having a button which shows the log -- that's going to be far easier for users to grok than "figure out where, if anywhere, your system is logging to".

imsodin · 2017-12-17T19:00:36Z

gui/default/syncthing/core/syncthingController.js

+            $http.get(urlbase + '/system/debug').success(function (data) {
+                var debug = {};
+                data.enabled = data.enabled || [];
+                $.each(data.facilities, function(key, value) {


Is this the same as data.facilities.forEach? Just asking because $.each isn't used in syncthingController.js anywhere else.

Same thing. forEach I think is less cross browsery.

imsodin · 2017-12-17T19:01:19Z

lib/fs/debug.go

@@ -14,9 +14,9 @@ import (
 )

 var (
-	l = logger.DefaultLogger.NewFacility("filesystem", "Filesystem access")
+	l = logger.DefaultLogger.NewFacility("fs", "Filesystem access")


This needs to be changed in the help text in main too (and subsequently in docs).

Help text in main is generated from available facilities.

AudriusButkevicius · 2017-12-18T20:09:10Z

Now displays logs, and "tails" them by polling every 2s.

imsodin · 2017-12-19T08:47:53Z

The log view keeps re-writing tags from the beginning (cmdline confirms it isn't actually restarting):

Also the trailing is very jumpy and doesn't actually follow the last line, but that may be related to the above problem. I think (de-)activating trailing should be done with a button that toggles it (while still also disabling when clicking into the text area).

What is the intention of the log view? If it is meant as the main source when asking users for logs, 250 lines might not be enough and a button to copy or save the log would be neat. To not increase memory consumption by raising the limit, maybe there could be an additional debug config option to keep all the logs.

AudriusButkevicius · 2017-12-19T08:55:14Z

It's rewriting logs if you haven't built syncthing, as there is a bug in the since parsing logic. I guess when we tell people to look at the log, they atleast can glimpse at the last 250 lines.

You can open the log and keep it open for a while I guess, but I suspect this would only be used to peek at the last few lines.

imsodin · 2017-12-19T09:05:38Z

I have rebuilt before and now with clean & assets for good measure, the problem with repeating logs and trailing not working remains (nothing in the JS console).

AudriusButkevicius · 2017-12-19T09:24:52Z

Do the http calls have a since query string?

Whats the issue with trailing apart from you not liking the way you enable disable it?

AudriusButkevicius · 2017-12-19T10:38:03Z

Also, the message is a red herring. Essentially, it stops tailing when the field gains focus, and the message comes up as 'click on me' just for the user to lose focus on the textarea (clicking the text doesn't actually do anything).

imsodin · 2017-12-19T11:26:48Z

I am confused: In JS console it looks like the last variable is correct, but in gui.go the since query string is empty. When I try from command line with the example for RFC3339 given in Go's docs like this:

 curl -X GET -H "X-API-Key: *" "localhost:8384/rest/system/log?since=2006-01-02T15:04:05Z07:00'

it fails to parse it (custom debug log after time.Parse):

[CBBHG] 2017/12/19 12:18:32.604678 gui.go:961: INFO: getSystemlog q.Get("since"): 2006-01-02T15:04:05Z07:00 err: parsing time "2006-01-02T15:04:05Z07:00": extra text: 07:00

The "test case"

	t, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z07:00")
	fmt.Println(t, err)

also results in

0001-01-01 00:00:00 +0000 UTC parsing time "2006-01-02T15:04:05Z07:00": extra text: 07:00.

What is wrong here?

AudriusButkevicius · 2017-12-19T11:30:33Z

Strange that it can generate timestamps with that signature yet not parse them, anyway, that's definately a bug.

AudriusButkevicius · 2017-12-19T11:31:43Z

I guess I could replace the text with "Click anywhere outside of the text box to continue tailing the log"

calmh · 2017-12-19T12:37:17Z

"Z07:00" is definitely a broken time zone specifier, so wherever that is generated should be fixed.

imsodin · 2017-12-19T12:40:40Z

EDIT: Missed calmh's comment: That is not generated, that was me copy-pasting from the Go docs.

Strange that it can generate timestamps with that signature yet not parse them, anyway, that's definately a bug.

Nah, it's non-obvious format notation combined with me not RTFM (thoroughly). The time zone is either Z or +-xx:xx, so Z07:00 is invalid.

That's actually a pretty annoying format for html queries, because + gets converted to -> error again. Therefore we need to either convert to UTC or encode the timezone + as %2B. Also the format from JS is with nanoseconds, that should be stripped in the request.

canton7 · 2017-12-19T12:48:56Z

Well, anything you put in a query string should be automatically escaped by whatever library you're using to do the query, so that shouldn't be an issue...

imsodin · 2017-12-19T12:50:53Z

How does the library know whether you mean a literal + or not? Curl and angularjs (I am assume $http.get is angularjs) don't do it.

Also I would propose to change the log rest functions such that a problem like this is a bit more obvious:

--- a/cmd/syncthing/gui.go
+++ b/cmd/syncthing/gui.go
@@ -955,20 +955,36 @@ func (s *apiService) postSystemErrorClear(w http.ResponseWriter, r *http.Request
 
 func (s *apiService) getSystemLog(w http.ResponseWriter, r *http.Request) {
 	q := r.URL.Query()
-	since, err := time.Parse(time.RFC3339, q.Get("since"))
-	l.Debugln(err)
+	var t time.Time
+	if since := q.Get("since"); since != "" {
+		var err error
+		if t, err = time.Parse(time.RFC3339, since); err != nil {
+			l.Debugln(err)
+			http.Error(w, err.Error(), 400)
+			return
+		}
+	}
+
 	sendJSON(w, map[string][]logger.Line{
-		"messages": s.systemLog.Since(since),
+		"messages": s.systemLog.Since(t),
 	})
 }
 
 func (s *apiService) getSystemLogTxt(w http.ResponseWriter, r *http.Request) {
 	q := r.URL.Query()
-	since, err := time.Parse(time.RFC3339, q.Get("since"))
-	l.Debugln(err)
+	var t time.Time
+	if since := q.Get("since"); since != "" {
+		var err error
+		if t, err = time.Parse(time.RFC3339, since); err != nil {
+			l.Debugln(err)
+			http.Error(w, err.Error(), 400)
+			return
+		}
+	}
+
 	w.Header().Set("Content-Type", "text/plain; charset=utf-8")
 
-	for _, line := range s.systemLog.Since(since) {
+	for _, line := range s.systemLog.Since(t) {
 		fmt.Fprintf(w, "%s: %s\n", line.When.Format(time.RFC3339), line.Message)
 	}
 }

canton7 · 2017-12-19T12:59:32Z

Apparently you should use encodeURIComponent with $http.get.

How does the library know whether you mean a literal + or not?

You pass in some values. They're url encoded. The server url decodes them, and gets your values. If you pass in a +, that's urlencoded to %2B and the server decodes it to +. If you pass in a , that's urlencoded to + and the server decodes it to .

imsodin · 2017-12-19T14:40:29Z

That's it - using encodeURIComponent on last the log display (including trailing) works smoothly here.

imsodin · 2017-12-21T10:07:47Z

gui/default/syncthing/core/logViewerModalView.html

+      <div id="log-viewer-log" class="tab-pane in active">
+        <label translate ng-if="logging.logEntries.length == 0">Loading...</label>
+        <textarea ng-focus="logging.paused = true" ng-blur="logging.paused = false" id="logViewerText" class="form-control" rows="20" ng-if="logging.logEntries.length != 0" readonly style="font-family: Consolas; font-size: 11px; overflow: auto;">{{ logging.content() }}</textarea>
+        <p translate class="help-block" ng-style="{'visibility': logging.paused ? 'visible' : 'hidden'}">Log tailing paused. Click here to continue</p>


The click on me as text seems fine and better than the new "Click anywhere..." proposal: Otherwise someone might click on the background and complain when the log modal disappears :P
I would add a dot (or three) at the end though.

Three dots to existing text?

To be consistent (first sentence gets a final dot): "Log tailing paused. Click here to continue."
On second thought, I don't like three dots ("continue..."). The idea was, that it signifies that something (trailing) will continue when you click, but that seems unnecessary and might be a problem for translation.

imsodin · 2017-12-21T10:11:56Z

@calmh The mac agent is missing - is it a planned "revision outage" or something unforeseen? More to the point: Will it be up soon(ish) or should we start skipping that test to merge stuff?

Apart from the very nitpicky comment about punctuation, this is lgtm.

calmh · 2017-12-21T11:28:45Z

Mac builder is down because it’s at my home, internet there is apparently down, and I’m currently in Dubai and unable to troubleshoot. I’ll be back in a couple of days and will fix it then.

imsodin · 2017-12-21T21:29:55Z

Translation strings need to be updated.

AudriusButkevicius · 2017-12-21T21:35:13Z

@calmh does that before every release anyway

calmh · 2017-12-24T20:42:42Z

@st-review lgtm

st-review · 2017-12-24T20:42:43Z

@calmh: Noted! Need another LGTM or explicit merge command.

calmh · 2017-12-24T20:43:31Z

I think the viewer deserves to be full screen and not a modal but that's a valid future improvement and this is great as is.

AudriusButkevicius · 2017-12-24T21:11:12Z

@imsodin needs to leave his mark.

imsodin · 2017-12-24T22:26:00Z

@st-review lgtm

st-review · 2017-12-24T22:26:11Z

👌 Merged as c58b383. Thanks, @AudriusButkevicius!

Just because there are a ton of people struggling to set env vars. Perhaps this should live in advanced settings, and perhaps we should have a button to view the log. GitHub-Pull-Request: #4604 LGTM: calmh, imsodin

gui: Add debug tab to settings (ref syncthing#2644)

1f8a656

Just because there are a ton of people struggling to set env vars. Perhaps this should live in advanced settings, and perhaps we should have a button to view the log.

imsodin reviewed Dec 17, 2017

View reviewed changes

Add log tailer, move to a separate section

881fb3a

Scroll to end, add msg (fixes syncthing#2644)

a0bd933

AudriusButkevicius changed the title ~~gui: Add debug tab to settings (ref #2644)~~ gui: Add debug tab to settings (fixes #2644) Dec 18, 2017

AudriusButkevicius force-pushed the debugs branch from ef486d3 to a0bd933 Compare December 18, 2017 20:39

AudriusButkevicius changed the title ~~gui: Add debug tab to settings (fixes #2644)~~ gui: Add log viewer (fixes #2644) Dec 18, 2017

Urlencode

6a0ec1c

imsodin reviewed Dec 21, 2017

View reviewed changes

Add dot

9bf487f

st-review closed this Dec 24, 2017

AudriusButkevicius deleted the debugs branch December 24, 2017 22:43

st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Dec 25, 2018

syncthing locked and limited conversation to collaborators Dec 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gui: Add log viewer (fixes #2644) #4604

gui: Add log viewer (fixes #2644) #4604

AudriusButkevicius commented Dec 17, 2017

canton7 commented Dec 17, 2017

imsodin Dec 17, 2017

AudriusButkevicius Dec 17, 2017

imsodin Dec 17, 2017

AudriusButkevicius Dec 18, 2017

AudriusButkevicius commented Dec 18, 2017

imsodin commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

imsodin commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

imsodin commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

calmh commented Dec 19, 2017 via email

imsodin commented Dec 19, 2017 •

edited

canton7 commented Dec 19, 2017

imsodin commented Dec 19, 2017

canton7 commented Dec 19, 2017

imsodin commented Dec 19, 2017

imsodin Dec 21, 2017

AudriusButkevicius Dec 21, 2017

imsodin Dec 21, 2017

imsodin commented Dec 21, 2017

calmh commented Dec 21, 2017 via email

imsodin commented Dec 21, 2017

AudriusButkevicius commented Dec 21, 2017

calmh commented Dec 24, 2017

st-review commented Dec 24, 2017

calmh commented Dec 24, 2017

AudriusButkevicius commented Dec 24, 2017

imsodin commented Dec 24, 2017

st-review commented Dec 24, 2017

gui: Add log viewer (fixes #2644) #4604

gui: Add log viewer (fixes #2644) #4604

Conversation

AudriusButkevicius commented Dec 17, 2017

canton7 commented Dec 17, 2017

imsodin Dec 17, 2017

Choose a reason for hiding this comment

AudriusButkevicius Dec 17, 2017

Choose a reason for hiding this comment

imsodin Dec 17, 2017

Choose a reason for hiding this comment

AudriusButkevicius Dec 18, 2017

Choose a reason for hiding this comment

AudriusButkevicius commented Dec 18, 2017

imsodin commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

imsodin commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

imsodin commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

AudriusButkevicius commented Dec 19, 2017

calmh commented Dec 19, 2017 via email

imsodin commented Dec 19, 2017 • edited

canton7 commented Dec 19, 2017

imsodin commented Dec 19, 2017

canton7 commented Dec 19, 2017

imsodin commented Dec 19, 2017

imsodin Dec 21, 2017

Choose a reason for hiding this comment

AudriusButkevicius Dec 21, 2017

Choose a reason for hiding this comment

imsodin Dec 21, 2017

Choose a reason for hiding this comment

imsodin commented Dec 21, 2017

calmh commented Dec 21, 2017 via email

imsodin commented Dec 21, 2017

AudriusButkevicius commented Dec 21, 2017

calmh commented Dec 24, 2017

st-review commented Dec 24, 2017

calmh commented Dec 24, 2017

AudriusButkevicius commented Dec 24, 2017

imsodin commented Dec 24, 2017

st-review commented Dec 24, 2017

imsodin commented Dec 19, 2017 •

edited