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

Logging with Panache #10929

Closed
FroMage opened this issue Jul 23, 2020 · 7 comments · Fixed by #14586
Closed

Logging with Panache #10929

FroMage opened this issue Jul 23, 2020 · 7 comments · Fixed by #14586
Assignees
Labels
area/panache kind/enhancement New feature or request
Milestone

Comments

@FroMage
Copy link
Member

FroMage commented Jul 23, 2020

ATM this is how everyone must set up logging:

class X {
    private static final Logger log = Logger.getLogger(X.class);
    // ...
    void f(){
       log.info("foo");
    }
}

Which has the same problem as CDI in terms of requiring you to get out of your method, go up to find where to define statics, remember the magic incantation, and then figure out where you wanted to call it. It ruins the movie.

But unlike CDI injection, logging is something that happens super extra often. This is why a lot of people go for System.err.println rather than logging during dev, because it's so much easier.

Let's fix this.

I'd like to make it so this:

class X {
    // ...
    void f(){
       QuarkusLogger.info("foo");
    }
}

is turned into the previous example by bytecode transformation. We should be able to optimise this like we optimise other transformations by checking if the class has a quarkus marker and has QuarkusLogger in its constant pool.

I hope this ease of use will lead to more people leaving logging info in their code after debugging, resulting in better logging, as opposed to all println being just removed after debugging, never replaced with proper logging.

WDYT?

@FroMage FroMage added the kind/enhancement New feature or request label Jul 23, 2020
@quarkusbot
Copy link

/cc @loicmathieu

@Ladicek
Copy link
Contributor

Ladicek commented Nov 30, 2020

@FroMage continuing from #13510: my prototype implementation of this idea (https://github.com/Ladicek/quarkus/commits/logging-experiment) indeed scans all classes in the application, which is a waste of resources. The scanning is minimal, but it's still extra work -- one that Jandex already does, because it already scans all classes. In other words, I totally agree with you, we need to get constant pool entries into Jandex :-)

(I also still need to implement the transformation from the static calls to virtual calls properly, which requires some stack shuffling. The ASM API is very picky about who its friends are :-) )

@FroMage
Copy link
Member Author

FroMage commented Nov 30, 2020

Great news. If you have some time, could you get smallrye/jandex#84 unstuck by measuring index size changes with the patch to some common quarkus extensions or sample applications?

@Ladicek
Copy link
Contributor

Ladicek commented Nov 30, 2020

I can do that, sure.

FYI, I got into this because I wanted to play with something interesting for a bit, so this isn't my top priority. But I should be able to dedicate a few hours here and there to it.

@FroMage
Copy link
Member Author

FroMage commented Nov 30, 2020

Great news. I think this is a big UX win if we get this done.

@Ladicek
Copy link
Contributor

Ladicek commented Nov 30, 2020

Just FTR, I have mostly finished the bytecode transformation in my branch, the only thing that needs finalizing is how to best declare the logger field. So I'll get to the Jandex work next.

@Ladicek
Copy link
Contributor

Ladicek commented Jan 25, 2021

I've just assigned this to myself, because I submitted a draft PR: #14586.

@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/panache kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants