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

adding marker property to ExLogRecord #335

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

dfa1
Copy link
Contributor

@dfa1 dfa1 commented Mar 17, 2021

This new property will be used to propagate the Marker to the actual
logging library. Since both SLF4J and Log4j define a different Marker
class without any common interface, here an Object is used.

This new property will be used to propagate the Marker to the actual
logging library. Since both SLF4J and Log4j define a different Marker
class without any common interface, here an Object is used.
@dfa1
Copy link
Contributor Author

dfa1 commented Mar 17, 2021

@jamezp as mentioned in dmlloyd/jboss-logmanager-embedded#3, adding marker property to ExtLogRecord /cc @dmlloyd

Please have a look!

@jamezp
Copy link
Member

jamezp commented Mar 17, 2021

It's going to maybe be a bit before I can look at this and think it through completely. However for the integration I think we'll need to use some kind of reflection.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 17, 2021

It's a pity that we don't have a Version class on JBoss LogManager.

In terms of reflection, I think it would be important to do the check up front (i.e. during static initialization) and set a static final boolean field. We may then be able to call the method directly, as long as we bracket it in an if block or perhaps within an enclosed private method bracketed in an if block.

@jamezp
Copy link
Member

jamezp commented Mar 17, 2021

It's a pity that we don't have a Version class on JBoss LogManager.

In terms of reflection, I think it would be important to do the check up front (i.e. during static initialization) and set a static final boolean field. We may then be able to call the method directly, as long as we bracket it in an if block or perhaps within an enclosed private method bracketed in an if block.

Yes agreed we should definitely check during static initialization. It definitely would be nice to invoke the method directly. I don't see why that wouldn't work at least.

@dfa1
Copy link
Contributor Author

dfa1 commented Mar 17, 2021

tentative implementation here: jboss-logging/slf4j-jboss-logmanager@2391fc2 @jamezp @dmlloyd

Copy link
Member

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

This seems fine to me.

@dfa1
Copy link
Contributor Author

dfa1 commented Apr 19, 2021

Hi @jamezp , @dmlloyd

just checking if you need something on my side in order to merge this PR. I also tried to implement a backward/forward compatible change in the sfl4j driver (that must be "repeated" in the log4j driver).

Please advise!

@jamezp
Copy link
Member

jamezp commented Apr 19, 2021

@dfa1 Sorry for the delay. I don't see an issue with this really. The only real reason I haven't merged it is because we don't really use master anywhere currently.

@jamezp
Copy link
Member

jamezp commented Apr 19, 2021

Clicked comment too soon :) I want to look at maybe backporting this as well to the 2.2 branch. It seems it could be useful there as well.

@jamezp jamezp merged commit 08e2781 into jboss-logging:master Apr 19, 2021
@dfa1 dfa1 deleted the support-for-markers branch May 10, 2021 19:10
@dfa1
Copy link
Contributor Author

dfa1 commented May 26, 2021

@jamezp no worries... and thank you for moving this forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants