-
Notifications
You must be signed in to change notification settings - Fork 174
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
Feature/add host, exception, MDC[properties], and marker support #29
Feature/add host, exception, MDC[properties], and marker support #29
Conversation
Bumped version number to 1.5.2
Fixes #28 |
CLA signed. |
@afinkenstadt thanks for doing this work! We will take a look |
@@ -337,329 +337,3 @@ Introduction to the Splunk product and some of its capabilities | |||
## Contact | |||
|
|||
You can reach the Dev Platform team at [[email protected]](mailto:[email protected]). |
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.
@afinkenstadt Can you refresh against master? The PR shouldn't show removing all of the content below if you are just making slight updates.
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.
I'll figure that part out. But this particular removal was due to duplicate sections in README.md
in the first place.
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.
@afinkenstadt We've just merged your changed to |
Awesome! |
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.
We're not quite ready to merge yet due to this hard dependency on logback for non-logback users
@@ -18,23 +18,46 @@ | |||
* under the License. | |||
*/ | |||
|
|||
import ch.qos.logback.classic.spi.IThrowableProxy; |
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.
@afinkenstadt This isn't quite ready to go. Using IThrowableProxy
adds a hard dependency on logback, which is something we definitely don't want (we also need support java.util.logging
and log4) so forcing people to also depend on logback is not possible.
Can we refactor this so references to IThrowableProxy
are isolated to HttpEventCollectorLogbackAppender.java
? If not, how do you feel about reverting those changes?
This means removing IThrowableProxy
from the following files:
HttpEventCollectorEventInfo.java
HttpEventCollectorLog4jAppender.java
(unless there's a log4j equivalent)HttpEventCollectorLoggingHandler.java
HttpEventCollectorSender.java
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.
Perhaps #38 will address the inadvertent dependency taking on ch.qos.logback
?
Per Issue #29, removed dependency on ch.qos.logback
Markers are now logged