Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable logging to stdout for access logs #1683

Merged
merged 1 commit into from
Jun 13, 2017

Conversation

m3co-code
Copy link
Contributor

This PR enables sending the access logs to the stdout. It should therefore solve #1306. Consistently to TraefikLogsFile, it uses stdout now when the config for AccessLogsFile is omitted or empty.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left a few remarks.

}
file = f
} else {
file = os.Stdout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else case could be dropped if we set file's default to stdout above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if logdata, err := ioutil.ReadFile(logfilePath); err != nil {
if logdata, err := ioutil.ReadFile(logfilePath); err == nil {
assertValidLogData(t, logdata)
} else {
fmt.Printf("%s\n%s\n", string(logdata), err.Error())
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a require I think.

And if it is, we can drop the else by handling the err != nil case first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, done.

if logdata, err := ioutil.ReadFile(logfilePath); err != nil {
if logdata, err := ioutil.ReadFile(logfilePath); err == nil {
assertValidLogData(t, logdata)
} else {
fmt.Printf("%s\n%s\n", string(logdata), err.Error())
Copy link
Contributor

@timoreimann timoreimann Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move the details into the last arguments of the NoError call on the next line.

Apart from saving us one line, it will also make sure that the error output and our custom details are shown together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compared the messages a bit with and without. I dropped it then completely as it doesn't really add value. The logdata will always be empty given there is an error and err infos we get anyway using require.NoError.

func TestNewLogHandlerOutputStdout(t *testing.T) {
file, err := ioutil.TempFile("", "testlogger")
if err != nil {
t.Fatalf("failed to create temp file: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should stick to either stdlib testing or testify but not mix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cleaned up the whole test case to use testify completely.

if len(filePath) > 0 {
f, err := openAccessLogFile(filePath)
if err != nil {
return nil, err
Copy link
Contributor

@timoreimann timoreimann Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will just returning err give us enough context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I added some specific error message that adds more context to the previous one.

defer file.Close()
// capture stdout in a file
stdOut := os.Stdout
os.Stdout = file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it'd be nice to use a memory-mapped file. I did a quick search and couldn't find an easy solution, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also checked and saw no "native" way of doing this, whereas the current approach with TempFile is included. Therefore I would stick with the current solution.


logger.ServeHTTP(&logtestResponseWriter{}, req, LogWriterTestHandlerFunc)

written, err := ioutil.ReadFile(file.Name())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably require.NoError on err?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I was a bit surprised by the compiler does not complain even though we do not use err anymore after this call. The reason is clear though, its already initialised and used before.

logger.ServeHTTP(&logtestResponseWriter{}, req, LogWriterTestHandlerFunc)

written, err := ioutil.ReadFile(file.Name())
assert.NotZero(t, len(written), "expected access log message on stdout")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require maybe? Does assertValidLogData make sense if the written length is zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope it doesn't, good catch.

@@ -92,7 +138,7 @@ func printLogdata(logdata []byte) string {
return fmt.Sprintf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this part will be executed even in the case where all assertions succeed. Do we want that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm its the error message we have to provide in a "static" way to the assert/require invocations. Therefore we have to do it at least once for each test case. I improved the message a bit and inlined the formatting into the requireValidLogData, so that we only do it once :)

@@ -82,7 +128,7 @@ func TestLogger(t *testing.T) {
assert.Equal(t, fmt.Sprintf("%d", len(helloWorld)), tokens[7], printLogdata(logdata))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the part up to and include this line somewhat hard to read. Maybe a combination of a requires and a switch statement could improve readability? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to use require and its much clearer imho.

@m3co-code m3co-code force-pushed the 1306-access-logs-to-stdout branch 2 times, most recently from ee10958 to f78d0a1 Compare June 7, 2017 09:01
@ldez
Copy link
Contributor

ldez commented Jun 7, 2017

@marco-jantke can you resolve conflicts?

@m3co-code m3co-code force-pushed the 1306-access-logs-to-stdout branch 3 times, most recently from c6da678 to c7a1048 Compare June 7, 2017 09:58
@m3co-code
Copy link
Contributor Author

@ldez thanks for the notice, done.
@timoreimann thanks for the review. I added the feedback, though I had to do a rebase in order to resolve the conflicts.

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change the behavior?

We want this behavior:

[accessLog]
# with nothing

:-> StdOut

[accessLog]
filePath = "/path/to/log/log.txt"

:-> file

# nothing at all

:-> no output

docs/toml.md Outdated
Using the default CLF option is simple, e.g.

```toml
[accessLog]
filePath = "/path/to/access.log"
filePath = "/path/to/access.log" # If empty, logs to stdout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you remove this comment? Prefer add a line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done.

docs/toml.md Outdated
```

To write JSON format logs, specify `json` as the format:
```toml
[accessLog]
filePath = "/path/to/access.log"
filePath = "/path/to/access.log" # If empty, logs to stdout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you remove this comment? Prefer add a line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done.

docs/toml.md Outdated
@@ -41,6 +41,7 @@
# traefikLogsFile = "log/traefik.log"

# Access logs file
# If not defined, logs to stdout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this option is deprecated, you need to add the comment in the new option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

}
}
return false
func requireValidLogData(t *testing.T, logData []byte) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer assert to require in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having require in the function name makes it clear that a function call to it might abort the test evaluation. This is something the function name should communicate for me and guards against unexpected behaviour. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand but:

  • it's your main assertion. For me, you don't need to mimic Testify in this case.
  • this method contains a mix of require and assert, you cannot guarantee it might abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I renamed it.

@m3co-code
Copy link
Contributor Author

@ldez thats exactly the behaviour we have now. Improved docs to match your description.

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Job 👍

docs/toml.md Outdated
Alternatively logs can be written in JSON.
Using the default CLF option is simple, e.g.
Alternatively logs can be written in JSON.
By default logs will be written to stdout and use the CLF format.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this part in order to understand that without the [accessLog] section, we will not have any accesslog ?

Moreover can you add the same information in the traefik.sample.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rephrased the configuration a bit to make it hopefully more clear and easier to read. Also I added and adjusted documentation in the traefik.sample.toml file. Good catch, didn't think about this one..

@@ -45,7 +45,7 @@ type GlobalConfiguration struct {
CheckNewVersion bool `description:"Periodically check if a new version has been released"`
AccessLogsFile string `description:"(Deprecated) Access logs file"` // Deprecated
AccessLog *types.AccessLog `description:"Access log settings"`
TraefikLogsFile string `description:"Traefik logs file"`
TraefikLogsFile string `description:"Traefik logs file. Stdout is used when omitted or empty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information must be in AccessLog.FilePath description ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it here by intend, as this is actually the behaviour of the TraefikLogsFile and as it wasn't documented yet, I added this comment here as well. Nevertheless, I also extended the documentation in AccessLog.FilePath. Good point.

@ldez
Copy link
Contributor

ldez commented Jun 13, 2017

@marco-jantke it's time to squash 😃

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@ldez ldez added this to the 1.4 milestone Jun 13, 2017
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @marco-jantke !
LGTM

@ldez ldez merged commit 885b9f3 into traefik:master Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logs kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants