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

Logging doc #10672

Merged
merged 3 commits into from
Jul 23, 2020
Merged

Logging doc #10672

merged 3 commits into from
Jul 23, 2020

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Jul 13, 2020 •
edited
Loading

Improve the logging documentation guide.

Fixes #9667
Fixes #5789
Fixes #4629

@boring-cyborg boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file area/documentation area/logging labels Jul 13, 2020
@loicmathieu loicmathieu marked this pull request as ready for review July 13, 2020 08:50
@loicmathieu loicmathieu mentioned this pull request Jul 13, 2020
Closed
gsmet
gsmet previously requested changes Jul 13, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I'm afraid this will conflict with the ongoing PR we had here for a while: #10474 .

I merged it right now so you can build on it rather than discarding it entirely.

@loicmathieu
Copy link
Contributor Author

@gsmet OK I'll rebase on master and rework my PR

@loicmathieu loicmathieu marked this pull request as draft July 13, 2020 11:56
@loicmathieu loicmathieu force-pushed the logging-doc branch 2 times, most recently from 6c1da56 to a7af457 Compare July 13, 2020 16:24
@loicmathieu loicmathieu marked this pull request as ready for review July 13, 2020 16:25
@loicmathieu
Copy link
Contributor Author

@gsmet I rebase on master and made some changes based on early feedbacks

@loicmathieu
Copy link
Contributor Author

@dmlloyd should I switch our config classes to use the Level from JBoss Log Manager instead of JUL ? It's not very visible so I don't know if it's really needed.

@dmlloyd
Copy link
Member

dmlloyd commented Jul 13, 2020

@dmlloyd should I switch our config classes to use the Level from JBoss Log Manager instead of JUL ? It's not very visible so I don't know if it's really needed.

I would say "no", just because that would make it impossible for users to ever use customized log levels, and would also make it impossible to configure JUL levels that don't have Apache equivalents (because this could in theory be desirable for certain libraries). Using the base class means that any possible log level can be specified.

abelsromero
abelsromero reviewed Jul 13, 2020
View reviewed changes
docs/src/main/asciidoc/logging.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/logging.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/logging.adoc Outdated Show resolved Hide resolved
@loicmathieu
Copy link
Contributor Author

@dmlloyd @abelsromero is it OK now with the modifications I made ?

dmlloyd
dmlloyd reviewed Jul 16, 2020
View reviewed changes
core/runtime/src/main/java/io/quarkus/runtime/logging/LogConfig.java
* {@link org.jboss.logmanager.Level#INFO}
* {@link org.jboss.logmanager.Level#DEBUG}
* {@link org.jboss.logmanager.Level#TRACE}
* * {@link org.jboss.logmanager.Level#FATAL}
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to use AsciiDoc style or JavaDoc style? If the latter, this should be a <ul>; if the former, then they shouldn't use {@link}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<ul> list are not well rendered on configuration reference section on the website.
I'll remove the {@link}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand {@link} are ignored by the asciidoc processing so it didn't appears on the website. Removing them will restrict API discovery on IDE ...

Copy link
Member

Choose a reason for hiding this comment

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

This is a long standing point of contention. :-) @gsmet do you know what we settled on? Is the doc renderer using AsciiDoc, or do we need a separate JavaDoc and AsciiDoc section, or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is both a JavaDoc and an asciidoc, and the doc build generates the HTML for the website based on asciidoc processing.
I think that the current aproach works even if it can seems disturbing because it mixes JavaDoc directive and asciidoc one.

@abelsromero
Copy link
Contributor

Fine for me 👍 (srry for double post...corp account)

@loicmathieu loicmathieu force-pushed the logging-doc branch 2 times, most recently from 72d8d17 to 24f915b Compare July 16, 2020 15:40
@gsmet gsmet dismissed their stale review July 18, 2020 14:52

Comment addressed.

@loicmathieu
Copy link
Contributor Author

@dmlloyd @gsmet is it OK now ?
Can we merge ?

@gastaldi gastaldi added this to the 1.7.0 - master milestone Jul 21, 2020
@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 21, 2020
@gastaldi gastaldi merged commit b50faf5 into quarkusio:master Jul 23, 2020
@loicmathieu loicmathieu deleted the logging-doc branch July 28, 2020 08:14
@galderz galderz mentioned this pull request Nov 9, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abelsromero abelsromero abelsromero left review comments

@gastaldi gastaldi gastaldi left review comments

@dmlloyd dmlloyd dmlloyd approved these changes

@gsmet gsmet gsmet left review comments

@stuartwdouglas stuartwdouglas Awaiting requested review from stuartwdouglas

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file area/documentation area/logging triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.7.0.CR1
5 participants
@loicmathieu @dmlloyd @abelsromero @gastaldi @gsmet