-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add json format support for Traefik logs #2056
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏
a7a9fd1
to
cd7df42
Compare
@juliens thanks for the review! I added one more commit in order to keep the colors when the logs are written to stdout :D :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @marco-jantke .
Great PR!
I just have one remark to do :
I think it can be great to modify and/or add a test in the access_log_test.go
file.
WDYT about this suggestion?
@nmengin it's not about access logs :-) I could, of course, add an integration test for the traefik log itself, but I am not sure on what to assert.. What I could is pick some lines and verify that they are valid JSON, without looking into the contents. Do you think this would help? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marco-jantke
Sorry for the misunderstood 😉
Do you think this would help?
IMHO, if there are tests, the code is easier to maintain. Even if, of course, these modifications are not about the main code part.
LGTM 🗞️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure because we've touched the subject recently: does the handling of the deprecated vs. new configuration options work as expected?
Also, have we documented which version takes precedence?
This is certainly a good point Timo. I tested with all the different constellations with CLI and toml configuration. The behaviour is consistent and as expected. Given the new configuration I now added a warning about the usage of the deprecated and documented at the deprecated option documentation, that the new option takes precedence. To summarise shortly:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for double checking @marco-jantke. LGTM.
This PR adds the possibility to use JSON format for the traefik logs.
Also it aligns the configuration to the accessLogs and deprecates the
traefikLogsFile
option.Sample log line:
This fixes #780.