{"action":"created","issue":{"url":"https://api.github.com/repos/jmxtrans/jmxtrans/issues/367","labels_url":"https://api.github.com/repos/jmxtrans/jmxtrans/issues/367/labels{/name}","comments_url":"https://api.github.com/repos/jmxtrans/jmxtrans/issues/367/comments","events_url":"https://api.github.com/repos/jmxtrans/jmxtrans/issues/367/events","html_url":"https://github.com/jmxtrans/jmxtrans/pull/367","id":119797699,"number":367,"title":"Fix logging control","user":{"login":"chumaitoe","id":3165263,"avatar_url":"https://avatars.githubusercontent.com/u/3165263?v=3","gravatar_id":"","url":"https://api.github.com/users/chumaitoe","html_url":"https://github.com/chumaitoe","followers_url":"https://api.github.com/users/chumaitoe/followers","following_url":"https://api.github.com/users/chumaitoe/following{/other_user}","gists_url":"https://api.github.com/users/chumaitoe/gists{/gist_id}","starred_url":"https://api.github.com/users/chumaitoe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chumaitoe/subscriptions","organizations_url":"https://api.github.com/users/chumaitoe/orgs","repos_url":"https://api.github.com/users/chumaitoe/repos","events_url":"https://api.github.com/users/chumaitoe/events{/privacy}","received_events_url":"https://api.github.com/users/chumaitoe/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-12-01T20:11:32Z","updated_at":"2015-12-07T07:19:06Z","closed_at":"2015-12-01T21:48:16Z","pull_request":{"url":"https://api.github.com/repos/jmxtrans/jmxtrans/pulls/367","html_url":"https://github.com/jmxtrans/jmxtrans/pull/367","diff_url":"https://github.com/jmxtrans/jmxtrans/pull/367.diff","patch_url":"https://github.com/jmxtrans/jmxtrans/pull/367.patch"},"body":"- Uses log4j as the implementation binding.  This is the only implementation binding that can currently be used, due to direct coding of org.slf4j.impl.Log4jLoggerFactory in org.jmxtrans.jmxtrans-output-log4j (com.googlecode.jmxtrans.model.output.KeyOutWriter).\r\n - Removed all other references to implementation bindings, as only one implementation bining can be used.\r\n - Removed all exclusions of log4j, since it must be the implementation.\r\n - Ensured all other logging mechanisms have the correct slf4j bindings.\r\n - Verified that setting LOG_LEVEL in /etc/default/jmxtrans changes the logging level (note:  there is almost no non-debug logging!)\r\n- Verified that extracting log4j.xml from jar, modifying, and re-inserting caused logging behavior to change as desired.\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23issuecomment-161084311%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46342941%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46343002%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46343769%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23issuecomment-161105829%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46349827%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46350209%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46354386%22%2C%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23issuecomment-161128455%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23issuecomment-161084311%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Note%3A%20%20not%20tested%20against%20RPM%20%28yet%29%2C%20but%20in%20theory%20this%20should%20not%20be%20a%20problem.%20%20Tested%20with%20deb%20package.%22%2C%20%22created_at%22%3A%20%222015-12-01T20%3A21%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3165263%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/chumaitoe%22%7D%7D%2C%20%7B%22body%22%3A%20%22That%20looks%20really%20good%21%20Thanks%20a%20lot%21%20I%27m%20still%20interested%20in%20getting%20rid%20of%20log4j%20based%20writers%20and%20of%20log4j%20itself%2C%20but%20in%20the%20meantime%2C%20this%20sounds%20like%20a%20pretty%20good%20PR%21%22%2C%20%22created_at%22%3A%20%222015-12-01T21%3A47%3A10Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1415765%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/gehel%22%7D%7D%2C%20%7B%22body%22%3A%20%22Also%2C%20I%20got%20something%20wrong%3A%20the%20code%20in%20com.googlecode.jmxtrans.model.output.KeyOutWriter%20calls%20into%20the%20%2Awrapper%20binding%2A%2C%20not%20the%20%2Aimplementation%20binding%2A%20%5B1%5D%2C%20so%20using%20logback%20as%20the%20implementation%20should%20be%20possible.%20%20I%27ll%20do%20some%20more%20testing...%5Cr%5Cn%5Cr%5Cn%5B1%5D%20The%20terms%20for%20the%20two%20types%20of%20bindings%20are%20my%20own%20contrivance.%20%20There%20is%20clear%20distinction%20between%20them%20in%20the%20SLF4J%20docs%2C%20but%20Ceki%20maddeningly%20just%20refers%20to%20them%20all%20as%20%5C%22bindings%5C%22.%22%2C%20%22created_at%22%3A%20%222015-12-01T23%3A18%3A30Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3165263%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/chumaitoe%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%2074f5377e048100d2cc68e00798772dfab9e31610%20jmxtrans-output/jmxtrans-output-log4j/pom.xml%2017%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46343002%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Same%20thing%2C%20we%20do%20have%20non%20optional%20dependencies%20on%20log4j.%22%2C%20%22created_at%22%3A%20%222015-12-01T21%3A39%3A44Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1415765%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/gehel%22%7D%7D%2C%20%7B%22body%22%3A%20%22This%20may%20be%20a%20mistake.%20%20I%27ll%20check%20on%20it%20and%20update.%22%2C%20%22created_at%22%3A%20%222015-12-01T22%3A30%3A52Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3165263%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/chumaitoe%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20jmxtrans-output/jmxtrans-output-log4j/pom.xml%3AL65-72%22%7D%2C%20%22Pull%2074f5377e048100d2cc68e00798772dfab9e31610%20pom.xml%2024%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46343769%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22We%20should%20be%20able%20to%20keep%20those%20exclusions.%20In%20the%20end%2C%20we%20choose%20at%20packaging%20time%20which%20logger%20we%20inject%20%28log4j-over-slf4j%20or%20log4j%20proper%29.%20Same%20for%20other%20exclusions%20of%20logging.%20I%27m%20still%20hoping%20to%20get%20rid%20of%20those%20log4j%20loggers%20and%20of%20log4j%20one%20of%20theses%20days%20%3B-%29%22%2C%20%22created_at%22%3A%20%222015-12-01T21%3A45%3A30Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1415765%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/gehel%22%7D%7D%2C%20%7B%22body%22%3A%20%22%5C%22In%20the%20end%2C%20we%20choose%20at%20packaging%20time%20which%20logger%20we%20inject%5C%22%5Cr%5Cn%5Cr%5CnAs%20long%20as%20there%20is%20hard%20code%20to%20an%20implementation%20binding%2C%20that%20is%20not%20true.%20%20You%20%2Acannot%2A%20bind%20to%20anything%20other%20than%20log4j%20%28I%20tried%20to%20keep%20the%20logback%20config%2C%20but%20could%20not%29.%5Cr%5Cn%5Cr%5CnBecause%20of%20this%2C%20it%20really%20doesn%27t%20make%20sense%20to%20pretend%20we%20can%20bind%20to%20anything%20else.%20%20Also%2C%20I%27m%20not%20sure%20that%20%5Blog4j%20calls%5D%20-%3E%20slf4j%3Aslf4j-log4j12%20-%3E%20slf4j%3Alog4j-over-slf4j%20is%20an%20identity%20operation.%5Cr%5Cn%5Cr%5CnWhich%20calls%20my%20attention%20to%20the%20fact%20that%20I%20bound%20to%20slf4j%3Aslf4j-log4j12%20in%20the%20jmxtrans%3Ajmxtrans%20pom%2C%20instead%20of%20slf4j%3Alog4j-over-slf4j...%20so%20why%20does%20this%20work%3F%21%5Cr%5Cn%5Cr%5Cn%2ASIGH%2A%22%2C%20%22created_at%22%3A%20%222015-12-01T23%3A10%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3165263%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/chumaitoe%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20pom.xml%3AL277-283%22%7D%2C%20%22Pull%2074f5377e048100d2cc68e00798772dfab9e31610%20jmxtrans-output/jmxtrans-output-log4j/pom.xml%209%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/jmxtrans/jmxtrans/pull/367%23discussion_r46342941%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20does%20not%20seem%20to%20be%20an%20optional%20dependency.%20At%20least%20%60TimeBasedRollingKeyOutWriter%60%20depends%20on%20it.%22%2C%20%22created_at%22%3A%20%222015-12-01T21%3A39%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1415765%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/gehel%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20optional%20flag%20means%20that%20we%20compile%20against%20it%2C%20but%20it%20is%20not%20picked%20up%20as%20a%20dependency%20by%20modules%20which%20depend%20on%20this%20pom.%20%20The%20logback%20components%20are%20linked%20to%20as%20if%20they%20were%20slf4j%3Aslf4j-api%2C%20which%20has%20the%20exact%20same%20interface%2C%20but%20different%20semantics%20from%20the%20viewpoint%20of%20SLF4J.%5Cr%5Cn%5Cr%5CnIf%20you%20find%20this%20confusing%2C%20you%20are%20not%20alone.%20%20It%20took%20me%20months%20the%20first%20time%20I%20had%20to%20untangle%20an%20SLF4J%20build.%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-12-01T22%3A33%3A35Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3165263%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/chumaitoe%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20jmxtrans-output/jmxtrans-output-log4j/pom.xml%3AL47-59%22%7D%7D%7D'></a>\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\n- [ ] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/jmxtrans/jmxtrans/pull/367#issuecomment-161084311'>General Comment</a></b>\n- <a href='https://github.com/chumaitoe'><img border=0 src='https://avatars.githubusercontent.com/u/3165263?v=3' height=16 width=16'></a> Note:  not tested against RPM (yet), but in theory this should not be a problem.  Tested with deb package.\n- <a href='https://github.com/gehel'><img border=0 src='https://avatars.githubusercontent.com/u/1415765?v=3' height=16 width=16'></a> That looks really good! Thanks a lot! I'm still interested in getting rid of log4j based writers and of log4j itself, but in the meantime, this sounds like a pretty good PR!\n- <a href='https://github.com/chumaitoe'><img border=0 src='https://avatars.githubusercontent.com/u/3165263?v=3' height=16 width=16'></a> Also, I got something wrong: the code in com.googlecode.jmxtrans.model.output.KeyOutWriter calls into the *wrapper binding*, not the *implementation binding* [1], so using logback as the implementation should be possible.  I'll do some more testing...\n[1] The terms for the two types of bindings are my own contrivance.  There is clear distinction between them in the SLF4J docs, but Ceki maddeningly just refers to them all as \"bindings\".\n- [ ] <a href='#crh-comment-Pull 74f5377e048100d2cc68e00798772dfab9e31610 jmxtrans-output/jmxtrans-output-log4j/pom.xml 9'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/jmxtrans/jmxtrans/pull/367#discussion_r46342941'>File: jmxtrans-output/jmxtrans-output-log4j/pom.xml:L47-59</a></b>\n- <a href='https://github.com/gehel'><img border=0 src='https://avatars.githubusercontent.com/u/1415765?v=3' height=16 width=16'></a> This does not seem to be an optional dependency. At least `TimeBasedRollingKeyOutWriter` depends on it.\n- <a href='https://github.com/chumaitoe'><img border=0 src='https://avatars.githubusercontent.com/u/3165263?v=3' height=16 width=16'></a> The optional flag means that we compile against it, but it is not picked up as a dependency by modules which depend on this pom.  The logback components are linked to as if they were slf4j:slf4j-api, which has the exact same interface, but different semantics from the viewpoint of SLF4J.\nIf you find this confusing, you are not alone.  It took me months the first time I had to untangle an SLF4J build.\n- [ ] <a href='#crh-comment-Pull 74f5377e048100d2cc68e00798772dfab9e31610 jmxtrans-output/jmxtrans-output-log4j/pom.xml 17'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/jmxtrans/jmxtrans/pull/367#discussion_r46343002'>File: jmxtrans-output/jmxtrans-output-log4j/pom.xml:L65-72</a></b>\n- <a href='https://github.com/gehel'><img border=0 src='https://avatars.githubusercontent.com/u/1415765?v=3' height=16 width=16'></a> Same thing, we do have non optional dependencies on log4j.\n- <a href='https://github.com/chumaitoe'><img border=0 src='https://avatars.githubusercontent.com/u/3165263?v=3' height=16 width=16'></a> This may be a mistake.  I'll check on it and update.\n- [ ] <a href='#crh-comment-Pull 74f5377e048100d2cc68e00798772dfab9e31610 pom.xml 24'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/jmxtrans/jmxtrans/pull/367#discussion_r46343769'>File: pom.xml:L277-283</a></b>\n- <a href='https://github.com/gehel'><img border=0 src='https://avatars.githubusercontent.com/u/1415765?v=3' height=16 width=16'></a> We should be able to keep those exclusions. In the end, we choose at packaging time which logger we inject (log4j-over-slf4j or log4j proper). Same for other exclusions of logging. I'm still hoping to get rid of those log4j loggers and of log4j one of theses days ;-)\n- <a href='https://github.com/chumaitoe'><img border=0 src='https://avatars.githubusercontent.com/u/3165263?v=3' height=16 width=16'></a> \"In the end, we choose at packaging time which logger we inject\"\nAs long as there is hard code to an implementation binding, that is not true.  You *cannot* bind to anything other than log4j (I tried to keep the logback config, but could not).\nBecause of this, it really doesn't make sense to pretend we can bind to anything else.  Also, I'm not sure that [log4j calls] -> slf4j:slf4j-log4j12 -> slf4j:log4j-over-slf4j is an identity operation.\nWhich calls my attention to the fact that I bound to slf4j:slf4j-log4j12 in the jmxtrans:jmxtrans pom, instead of slf4j:log4j-over-slf4j... so why does this work?!\n*SIGH*\n\n\n<a href='https://www.codereviewhub.com/jmxtrans/jmxtrans/pull/367?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/jmxtrans/jmxtrans/pull/367?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/jmxtrans/jmxtrans/pull/367'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/jmxtrans/jmxtrans/issues/comments/162435583","html_url":"https://github.com/jmxtrans/jmxtrans/pull/367#issuecomment-162435583","issue_url":"https://api.github.com/repos/jmxtrans/jmxtrans/issues/367","id":162435583,"user":{"login":"arohter","id":360418,"avatar_url":"https://avatars.githubusercontent.com/u/360418?v=3","gravatar_id":"","url":"https://api.github.com/users/arohter","html_url":"https://github.com/arohter","followers_url":"https://api.github.com/users/arohter/followers","following_url":"https://api.github.com/users/arohter/following{/other_user}","gists_url":"https://api.github.com/users/arohter/gists{/gist_id}","starred_url":"https://api.github.com/users/arohter/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arohter/subscriptions","organizations_url":"https://api.github.com/users/arohter/orgs","repos_url":"https://api.github.com/users/arohter/repos","events_url":"https://api.github.com/users/arohter/events{/privacy}","received_events_url":"https://api.github.com/users/arohter/received_events","type":"User","site_admin":false},"created_at":"2015-12-07T07:19:06Z","updated_at":"2015-12-07T07:19:06Z","body":":+1: "}}