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

slf4j LogAppender #479

Merged
merged 5 commits into from
Aug 12, 2022
Merged

slf4j LogAppender #479

merged 5 commits into from
Aug 12, 2022

Conversation

justcoon
Copy link
Contributor

@justcoon justcoon commented Jul 25, 2022

slf4j log appender

  • key-values - LogAppender.appendKeyValue - logged into MDC
  • cause - LogAppender.appendCause - as throwable, Cause transformed into FiberFailure
  • slf4j logger name extracted from Trace

open question -

  • do we need rootLogLevel in private def slf4jLogger(rootLoggerName: Trace => String, rootLogLevel: LogLevel, format: LogFormat): ZLogger[String, Unit] ? - in general, log level setup is in specific logger configuration
  • LogFormat combinators/composition - in current design, it is not clear, where specific LogFormat part will be "inserted" (message, MDC, cause ...), there is a question, if it should not be more clear in LogFormat itself
  • should we allow custom definition for log logger name (maybe by annotation as it was in version 1) ?

related to: #485, #453, #445

@987Nabil
Copy link
Contributor

987Nabil commented Aug 4, 2022

@justcoon I guess this is also the reason, why updating to 2.0.1 broke our slf4j + logstash json logging?
Any plan when this would be fixed? Or do you need some help?

And to answer your last question: Yes we definitely need the ability to set logger names. In the 2xRC version we did this with the rootLoggerName: Trace => String

@justcoon
Copy link
Contributor Author

justcoon commented Aug 4, 2022

hello @987Nabil,

in relation to

2.0.1 broke our slf4j + logstash json logging

do you mean update from 2.0.0 -> 2.0.1? If yes, maybe it will be good if you open some ticket with details (like logstash configuration, zio logging slf4j setup - log format ..., code snippet, if possible)

in case of logger name - SLF4J.getLoggerName
basically if there is trace zio.logging.example.LivePingService.ping(PingService.scala:22), then logger name is zio.logging.example.LivePingService - service class name

in my opinion, this is reasonable default logger name resolution, based on trace
but at the other side, i think, have option to say, what is logger name, can be very usefull

* Appends a key/value pair, with the value it created with the log appender.
*/
final def appendKeyValue(key: String, appendValue: LogAppender => Unit): Unit = {
def appendKeyValue(key: String, value: String): Unit = {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not modify the appendKeyValue. Instead, generate an anonymous LogAppender that will simply produce string for any possible combination of operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdegoes: previously removed function def appendKeyValue(key: String, appendValue: LogAppender => Unit) of LogAppender- returned back

@987Nabil
Copy link
Contributor

987Nabil commented Aug 4, 2022

hello @987Nabil,

in relation to

2.0.1 broke our slf4j + logstash json logging

do you mean update from 2.0.0 -> 2.0.1? If yes, maybe it will be good if you open some ticket with details (like logstash configuration, zio logging slf4j setup - log format ..., code snippet, if possible)

I'll do.

in case of logger name - SLF4J.getLoggerName basically if there is trace zio.logging.example.LivePingService.ping(PingService.scala:22), then logger name is zio.logging.example.LivePingService - service class name

in my opinion, this is reasonable default logger name resolution, based on trace but at the other side, i think, have option to say, what is logger name, can be very usefull

Maybe it is a reasonable default, but in my opinion having it configurable is a must. There are architecture guidelines out there in the wild, that require special logger names for access logging for example. One reason for something like this can be, that messages with a certain name get treated in your logging tool based on logger names.
Also it is available in every (JVM) logging lib out there and I do not see any reason to break this pattern.

@987Nabil
Copy link
Contributor

987Nabil commented Aug 5, 2022

@justcoon I opened #489 and would not be surprised, if it is related.

@justcoon justcoon force-pushed the slf4j_log_appender branch from b54a645 to a5a8dfa Compare August 10, 2022 08:56
@justcoon
Copy link
Contributor Author

latest changes

  • previously removed function def appendKeyValue(key: String, appendValue: LogAppender => Unit) of LogAppender- returned back
  • added SLF4J.loggerName aspect, for custom slf4j logger name
  • layers, with LogLevel are deprecated
  • there are scala-2 tests only, for slf4j, as there is issue with Trace in scala-3 (Align Traces zio#7177)
  • updated documentation and examples

@justcoon justcoon changed the title slf4j LogAppender - DRAFT slf4j LogAppender Aug 11, 2022
@justcoon justcoon marked this pull request as ready for review August 11, 2022 17:08
@justcoon justcoon requested a review from a team as a code owner August 11, 2022 17:08
@jdegoes jdegoes merged commit 8e64f98 into zio:master Aug 12, 2022
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