Skip to content
This repository has been archived by the owner on Sep 21, 2021. It is now read-only.

Improve logging by introducing slf4j and logback #501

Merged
merged 8 commits into from
Mar 28, 2018

Conversation

dennisgranath
Copy link
Contributor

@dennisgranath dennisgranath commented Mar 28, 2018

Description

Change logging from java util logging (jul) to slf4j. Add option to log in jsonFormat with command line argument --logJson. All "native" selenium logs are sent to logback using jul-to-slf4j by setting the SLF4JBridgeHandler (logging_info.properties). In order to that in a smooth way i added the selenium jar into the resulting zalenium.jar.

Motivation and Context

We would like to be able to log in json output and add support for MDC (mapped diagnotic context). Using this approach one could build dashboards in tools like for example kibana.

How Has This Been Tested?

  • Unit tests run
  • Manual testing of DockerSeleniumStarterProxy and BrowserStackProxy

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • All new and existing tests passed.

@codecov-io
Copy link

codecov-io commented Mar 28, 2018

Codecov Report

Merging #501 into merge-501 will increase coverage by 0.18%.
The diff coverage is 55.15%.

@@               Coverage Diff               @@
##             merge-501     #501      +/-   ##
===============================================
+ Coverage        80.27%   80.45%   +0.18%     
- Complexity         572      578       +6     
===============================================
  Files               32       32              
  Lines             2662     2656       -6     
  Branches           232      232              
===============================================
  Hits              2137     2137              
+ Misses             384      378       -6     
  Partials           141      141

@diemol
Copy link
Contributor

diemol commented Mar 28, 2018

Thanks @dennisgranath for this PR!
I'll run a few tests and then merge it to a protected branch so Travis can run all the tests. I'll get back to you soon.

@diemol diemol changed the base branch from master to merge-501 March 28, 2018 19:29
@@ -260,32 +287,7 @@
</filter>
</filters>
<artifactSet>
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes so much sense! I don't know how we overlooked that :) Thanks!

@diemol
Copy link
Contributor

diemol commented Mar 28, 2018

Merging to a protected branch to run CI tests.

@diemol diemol merged commit d9d08f2 into zalando:merge-501 Mar 28, 2018
@diemol diemol mentioned this pull request Mar 28, 2018
@@ -16,8 +16,7 @@
# By default we only configure a ConsoleHandler, which will only
# show messages at the INFO and above levels.
#handlers= java.util.logging.ConsoleHandler
handlers= java.util.logging.FileHandler, java.util.logging.ConsoleHandler

handlers = org.slf4j.bridge.SLF4JBridgeHandler
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one note here. If someone is dependent on that logs are written to a file I think we need to add an optional filehandler in logback.xml.

diemol added a commit that referenced this pull request Mar 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants