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.

Sign up for GitHub

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

Jump to bottom

Simplify logging using static invocations and bytecode transformation #14586

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Simplify logging using static invocations and bytecode transformation #14586

merged 1 commit into from
Sep 1, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jan 25, 2021

Resolves #10929.

@ghost
Copy link

ghost commented Jan 25, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@ghost ghost added area/arc Issue related to ARC (dependency injection) area/core area/dependencies Pull requests that update a dependency file labels Jan 25, 2021
@Ladicek Ladicek force-pushed the logging-with-panache branch from 207ee6d to 01150c9 Compare January 25, 2021 16:07
@Ladicek Ladicek changed the title simplify logging using static invocations and bytecode transformation Simplify logging using static invocations and bytecode transformation Jan 25, 2021
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 25, 2021

Creating as a draft to let CI run in my fork, and perhaps to solicit some early reviews. The entire thing works, but need to decide where to put the code (probably shouldn't be a standalone extension, but part of core?) and documentation (probably in https://quarkus.io/guides/logging).

@Ladicek Ladicek mentioned this pull request Jan 25, 2021
Closed
@FroMage
Copy link
Member

FroMage commented Jan 25, 2021

VERY NICE!!!

@geoand
Copy link
Contributor

geoand commented Jan 26, 2021

Very nice indeed!

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 26, 2021

One thing I wanted to ask everyone: should this be an extra extension (it is right now), or part of something else? There's just one class in runtime, so perhaps this could move to core, so that it's always available? There's a ton of other stuff in core/runtime in io.quarkus.runtime.logging, so not exactly sure that's the best place either...

@geoand
Copy link
Contributor

geoand commented Jan 26, 2021

One thing I wanted to ask everyone: should this be an extra extension (it is right now), or part of something else?

It feels like a core thing to me

@FroMage
Copy link
Member

FroMage commented Jan 26, 2021

Definitely core material IMO.

@FroMage
Copy link
Member

FroMage commented Jan 26, 2021

We want people to have this OOTB and it doesn't cost if people don't use it and there are no new dependencies.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 26, 2021

Agree, thanks, I'll move it around.

@Ladicek Ladicek force-pushed the logging-with-panache branch from 01150c9 to 6d4bd65 Compare January 27, 2021 13:49
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 27, 2021

Moved to core and added documentation.

@Ladicek Ladicek force-pushed the logging-with-panache branch from 6d4bd65 to 51ab404 Compare January 27, 2021 14:55
@ghost ghost added the area/testing label Jan 27, 2021
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 27, 2021

And also added a devmode test.

@Ladicek Ladicek force-pushed the logging-with-panache branch 2 times, most recently from 2955769 to 556a8d8 Compare January 28, 2021 08:49
@FroMage
Copy link
Member

FroMage commented Feb 16, 2021

So why is this still a draft?

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 16, 2021

Waiting for @n1hility to merge smallrye/jandex#112 and release Jandex 3.0 :-)

@FroMage
Copy link
Member

FroMage commented Feb 16, 2021

Mmm, who else can merge this?

Base automatically changed from master to main March 12, 2021 15:55
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 26, 2021

CI passed in my fork. After 8 months, marking as ready for review :-)

@Ladicek Ladicek marked this pull request as ready for review August 26, 2021 10:45
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 26, 2021

Hmm I should probably get the Jandex Maven plugin also updated...

@famod
Copy link
Member

famod commented Aug 26, 2021

Hmm I should probably get the Jandex Maven plugin also updated...

👍, was about to leave a note on that!

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 26, 2021

It's there: wildfly/jandex-maven-plugin#32 :-)

gastaldi
gastaldi approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Works as advertised! Good job!

@gastaldi
Copy link
Contributor

Let's merge this once #14586 (comment) is fixed

gastaldi
gastaldi requested changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Waiting for the documentation update. Feel free to dismiss this request for changes and merge once it's done

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 1, 2021

Jandex Maven plugin 1.2.0 released, I'll bump it in this PR when it reaches Central.

FroMage
FroMage approved these changes Sep 1, 2021
View reviewed changes
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

Looks great, congrats!

@Ladicek Ladicek force-pushed the logging-with-panache branch from 4596faa to 1c31ff4 Compare September 1, 2021 12:40
gastaldi
gastaldi approved these changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Let's get this merged after the plugin version bump. Good job @Ladicek !

@Ladicek
Simplify logging using static invocations and bytecode transformation
a5424ae 
Also update Jandex to 2.4.0.Final and Jandex Maven plugin to 1.2.0.
@Ladicek Ladicek force-pushed the logging-with-panache branch from 1c31ff4 to a5424ae Compare September 1, 2021 13:56
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 1, 2021

Jandex Maven plugin updated to 1.2.0, yay!

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform labels Sep 1, 2021
@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 1, 2021
@gastaldi gastaldi merged commit 55a5bfb into quarkusio:main Sep 1, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Sep 1, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 1, 2021
@Ladicek Ladicek deleted the logging-with-panache branch September 2, 2021 08:49
@manusa manusa mentioned this pull request Sep 2, 2021
Closed
This was referenced Sep 23, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet left review comments

@geoand geoand geoand left review comments

@gastaldi gastaldi gastaldi approved these changes

@FroMage FroMage FroMage approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/logging area/platform Issues related to definition and interaction with Quarkus Platform area/testing release/noteworthy-feature
Projects
None yet
Milestone
2.3.0.CR1
Development

Successfully merging this pull request may close these issues.

Logging with Panache
7 participants
@Ladicek @FroMage @geoand @Sanne @famod @gastaldi @gsmet