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

Support for access log in Vert.x HTTP #7898

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Support for access log in Vert.x HTTP #7898

merged 1 commit into from
Apr 14, 2020

Conversation

stuartwdouglas
Copy link
Member

Fixes #5556

@stuartwdouglas stuartwdouglas force-pushed the access-logging branch 2 times, most recently from e8bcafc to d46578e Compare March 17, 2020 05:45
gastaldi
gastaldi reviewed Mar 17, 2020
View reviewed changes
...tx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/attribute/DateTimeAttribute.java

private static final String COMMON_LOG_PATTERN = "[dd/MMM/yyyy:HH:mm:ss Z]";

private static final ThreadLocal<SimpleDateFormat> COMMON_LOG_PATTERN_FORMAT = new ThreadLocal<SimpleDateFormat>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use DateTimeFormatter.ofPattern instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all just copied from Undertow, there are probably lots of things that could be better.

@gastaldi gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Mar 17, 2020
@gastaldi
Copy link
Contributor

Looks like it needs to be rebased

@gastaldi gastaldi added this to the 1.4.0 milestone Mar 17, 2020
@stuartwdouglas stuartwdouglas removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Mar 19, 2020
@stuartwdouglas
Copy link
Member Author

rebased

@gsmet
Copy link
Member

gsmet commented Mar 24, 2020

@ia3andy could you test that one and validate things are working properly?

The code is coming from an existing codebase so it's mostly about validating that the glue works and we get an access log with Quarkus when configuring things.

@ia3andy
Copy link
Contributor

ia3andy commented Mar 24, 2020

@gsmet sure!

@quarkusio quarkusio deleted a comment from gsmet Mar 24, 2020
@ia3andy
Copy link
Contributor

ia3andy commented Mar 24, 2020

@gsmet never mind :) (I removed the pollution)

ia3andy
ia3andy approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

It works great!

I would add another config property boolean logToFileEnabled and give a default value to baseFileName, IMO it would make it simpler to understand. If you don't agree, I at least suggested changes to the doc.

@gastaldi
Copy link
Contributor

Shouldn't license headers and @author tags be removed in this case? Or are they kept because they were copied from Undertow?

@stuartwdouglas
Copy link
Member Author

I removed them, should be ready to merge now.

@gsmet gsmet merged commit 328e5d0 into quarkusio:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi left review comments

@ia3andy ia3andy ia3andy approved these changes

Assignees
No one assigned
Labels
area/documentation area/vertx
Projects
None yet
Milestone
1.4.0.CR1
Development

Successfully merging this pull request may close these issues.

Support access log logging when using RESTEasy with Vert.x
4 participants
@stuartwdouglas @gastaldi @gsmet @ia3andy