Skip to content

Context bound logger #212

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

Closed
wants to merge 2 commits into from

Conversation

grunzwei
Copy link

@grunzwei grunzwei commented Mar 24, 2020

this PR has been a tale of woes in some parts.

first, it appears that you can't have a base class, for instance a trait ALogger, that is extended by both Logger and LoggerTakingArgInConstructor, which could be used in an abstract class that needs some logger, but doesn't know itself which logger should be used and needs it to be mixed in or inherited.

for some reason, once the abstract class member is of type ALogger, the macros are not invoked and you reach the default implementation of the ALogger trait.
sigh.

next, creating a trait ALogger and wrapping the macro loggers with it via delegation for the purposes mentioned above also fails - because putting a dependency from compiled code on macro requires separate compilation runs, and there is only one here.
sigh.

i had such hopes.

in this PR the ALogger trait is included but the wrappers allowing custom logger support are only in the test package as examples of how this could be achieved.

let's start talking about it ...

… you don't want to pass it each time via implicit for every call, just set it once when you instantiate the logger.

see lightbend-labs#208
… you don't want to pass it each time via implicit for every call, just set it once when you instantiate the logger.

see lightbend-labs#208
@lightbend-cla-validator
Copy link
Collaborator

Hi @grunzwei,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@grunzwei
Copy link
Author

sigh, no, this approach to extensibility with wrappers is no good, all the log messages will show incorrect log call site location :\

maybe you guys have another idea how the macros can inherit a trait ALogger, and later an abstract class can have a member logger: ALogger but still when methods are invoked on it the macro is invoked and not the trait?

@grunzwei
Copy link
Author

I'm checking about the CLA thing with my workplace Wix, where this is an authorized side project. Will sign once i get instructions.

@grunzwei
Copy link
Author

i recreated the issue that's the original source or grief here,
asked on stackoverflow:
https://stackoverflow.com/questions/60846904/scala-using-method-of-extending-class-based-on-macros-invokes-trait-default-imp

@grunzwei
Copy link
Author

So, according to the answer in stackoverflow, there's no way of invoking an abstract type and getting the implementing macro.

This leaves the approach i took of wrapping with an adapter ALogger and using it instead.

I was concerned that this will lose call-site information for the logger, but from what i see, in scala, this isn't a big deal.
in Node, for example, the logger also logs file and line number, but in Scala apparently not so... so wrapping the logger is apparently no big deal. correct me if i'm wrong?

Also, this makes me wonder... if callsite information is not the problem, why use macros to do the delayed logging based on log level? why not "pass by name" parameters?

class Logger(underlying: Underlying) {
  def info(msg: =>String): Unit = {
    if (underlying.isInfoEnabled) underlying.info(msg)
  }
}

no macros, still lazy... what am i missing?

@grunzwei grunzwei marked this pull request as ready for review March 26, 2020 08:48
@or-shachar
Copy link

or-shachar commented Apr 12, 2020

@Ophirr33 @eugeniyk @analytically - any chance you can take a look?

@grunzwei
Copy link
Author

so caller data is a thing in logback, for example, apparently :
i wonder how configurable this is, so it can ignore an additional layer

https://github.com/qos-ch/logback/blob/master/logback-classic/src/main/java/ch/qos/logback/classic/spi/CallerData.java

@grunzwei
Copy link
Author

apparently not a big deal to configure,
not necessarily a blocker
http://logback.qos.ch/manual/layouts.html#caller

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

Successfully merging this pull request may close these issues.

3 participants