getlantern/lantern#2232 Code review fixes #96
Conversation
| @@ -8,7 +8,7 @@ VERSION="`git describe --abbrev=0 --tags --exact-match || git rev-parse --short | |||
| BUILD_DATE="`date -u +%Y%m%d%.%H%M%S`" | |||
| export VERSION_STRING="$VERSION ($BUILD_DATE)" | |||
| LOGGLY_TOKEN="469973d5-6eaf-445a-be71-cf27141316a1" | |||
| LDFLAGS="-w -X main.version $VERSION -X main.buildDate $BUILD_DATE -X main.logglyToken LOGGLY_TOKEN" | |||
| LDFLAGS="-w -X main.version $VERSION -X main.buildDate $BUILD_DATE -X github.com/getlantern/flashlight/logging.logglyToken $LOGGLY_TOKEN" | |||
There was a problem hiding this comment.
This never worked, so nothing ever actually got logged to Loggly. I fixed it to embed the token correctly and verified that we're getting data to Loggly now.
There was a problem hiding this comment.
Thanks Ox! that's a testing-only token, right?
There was a problem hiding this comment.
Nope, that's the production token.
There was a problem hiding this comment.
Hmm... that's a thing. Since that's the only thing one needs to post to loggly I was going to suggest we remove it from the public view and add it to too-many-secrets but then I realized it will be embedded into the binary anyway, so I guess we may as well leave it public for now.
There was a problem hiding this comment.
Yeah, by definition it has to be public because our clients will use it to post.
|
Changes Unknown when pulling 691776d on issue-2232-ox into * on issue-2232*. |
|
Changes Unknown when pulling c7c4934 on issue-2232-ox into * on issue-2232*. |
|
@xiam How does it look? |
|
@xiam I just thought of one more improvement. Will check in shortly. |
|
@xiam Okay, change is made. |
| lang, _ := jibber_jabber.DetectLanguage() | ||
| logglyWriter := &logglyErrorWriter{ | ||
| lang: lang, | ||
| tz: time.Now().Format("MST"), |
There was a problem hiding this comment.
The way we were getting the time zone before just printed "local". Now this prints a timezone string like "CST".
There was a problem hiding this comment.
We're forcing UTC in line 109:
+ return fmt.Fprintf(w, "%s - ", time.Now().In(time.UTC).Format(logTimestampFormat))
Should not this timezone match the actual date we're logging?
There was a problem hiding this comment.
Great question. We actually use the timezone to identify the user's location in case geolocation doesn't work, so we actually want the user's timezone, not the reported time.
When you look in Loggly, you can see that the timestamp is displayed with its timezone, so there should be little confusion.
I am going to rename timeZone to userTimeZone just to make it clearer.
There was a problem hiding this comment.
Actually, our old logs already use the name "timeZone", so I'm going to leave it alone.
|
Thanks @oxtoacart, I'm into it. |
|
Changes Unknown when pulling 46e944d on issue-2232-ox into * on issue-2232*. |
| @@ -4,15 +4,17 @@ import ( | |||
| "fmt" | |||
There was a problem hiding this comment.
I guess this systray_windows made it to here by accident, right?
|
@oxtoacart Looks good to me and I've just checked that the systray_windows.go is the same one that is on the valencia branch (after the last merge), so it won't be a problem. |
|
Merged, let me check my branch again. Thank you @oxtoacart! |
|
@xiam Thanks! |
@xiam Suggested updates for your logging implementation