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

No request logs in dropwizard 1.0.1 #1737

Closed
m-at-nu opened this Issue Sep 23, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@m-at-nu

m-at-nu commented Sep 23, 2016

Hello,
since I've updated dropwizard to version 1.0.1 I have no request logs anymore, using dropwizard version 1.0.0 everything works. Is this a known bug or was anything changed regarding the configuration? According to the release logs, issue #1678 may be related to this? Even so, having only the requestLog's appenders without logging appenders doesn't work either with version 1.0.1.

I've attached my server configuration server.yml file below:

server:
  applicationContextPath: /
  applicationConnectors:
    - type: http
      port: 8080
  requestLog:
    appenders:
      - type: console
logging:
  appenders:
    - type: console

I've also attached a small minimal working example. Please change the dropwizard version to 1.0.0 and 1.0.1 respectively to test the issue.

Thanks in advance!
test.zip

@nickbabcock

This comment has been minimized.

Show comment
Hide comment
@nickbabcock

nickbabcock Sep 23, 2016

Contributor

I can confirm that this is a bug affecting v1.0.1

@m-at-nu correctly identified where the bug was introduced.

I can also confirm that checking out the 1.0.x branch, the following command fixed the issue

git revert -m 1 e7c16c97
Contributor

nickbabcock commented Sep 23, 2016

I can confirm that this is a bug affecting v1.0.1

@m-at-nu correctly identified where the bug was introduced.

I can also confirm that checking out the 1.0.x branch, the following command fixed the issue

git revert -m 1 e7c16c97

@nickbabcock nickbabcock added the bug label Sep 23, 2016

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Sep 23, 2016

Member

Yes, I can also confirm this regression. Which is strange, because it's exactly the problem which had to fixed by #1678. I would like to get some feedback from @evnm and @tjcutajar before reverting the commit.

Member

arteam commented Sep 23, 2016

Yes, I can also confirm this regression. Which is strange, because it's exactly the problem which had to fixed by #1678. I would like to get some feedback from @evnm and @tjcutajar before reverting the commit.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Sep 23, 2016

Member

Oh, now I see @tjcutajar pointed out to this problem in #1672, we ignored his comment. How inconsiderate of us.

Member

arteam commented Sep 23, 2016

Oh, now I see @tjcutajar pointed out to this problem in #1672, we ignored his comment. How inconsiderate of us.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Sep 23, 2016

Member

Digging into internals I see that logback doesn't log messages because of a NullPointerException in org.eclipse.jetty.server.Request#extractQueryParameters method because the _metadata object is null.

Member

arteam commented Sep 23, 2016

Digging into internals I see that logback doesn't log messages because of a NullPointerException in org.eclipse.jetty.server.Request#extractQueryParameters method because the _metadata object is null.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Sep 23, 2016

Member

It looks like there is a race between calling the org.eclipse.jetty.server.Request#recycle method by Jetty and extracting information from the request by Logback.

Member

arteam commented Sep 23, 2016

It looks like there is a race between calling the org.eclipse.jetty.server.Request#recycle method by Jetty and extracting information from the request by Logback.

@evnm

This comment has been minimized.

Show comment
Hide comment
@evnm

evnm Sep 23, 2016

Member

Those are the same Logback codepaths that I've been spelunking in while debugging #1699. There seems to be a growing number of fingers pointing to Dropwizard's Logback-enabled request logging.

Member

evnm commented Sep 23, 2016

Those are the same Logback codepaths that I've been spelunking in while debugging #1699. There seems to be a growing number of fingers pointing to Dropwizard's Logback-enabled request logging.

@tjcutajar

This comment has been minimized.

Show comment
Hide comment
@tjcutajar

tjcutajar Sep 23, 2016

Contributor

This is a duplicate of #1686

Apologies that perhaps the title and description of it is not clear. Considering the state of request logging is actually worse than it was before it makes sense for #1678 to be reverted.

Contributor

tjcutajar commented Sep 23, 2016

This is a duplicate of #1686

Apologies that perhaps the title and description of it is not clear. Considering the state of request logging is actually worse than it was before it makes sense for #1678 to be reverted.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Sep 23, 2016

Member

I've pushed to the Maven Central Repository Dropwizard 1.0.2 which reverts #1678. Should be available in a couple of hours.

Member

arteam commented Sep 23, 2016

I've pushed to the Maven Central Repository Dropwizard 1.0.2 which reverts #1678. Should be available in a couple of hours.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Sep 26, 2016

Member

Dropwizard 1.0.2 has been published to the Maven Central repository.

Member

arteam commented Sep 26, 2016

Dropwizard 1.0.2 has been published to the Maven Central repository.

@arteam arteam added this to the 1.0.1 milestone Sep 26, 2016

@m-at-nu

This comment has been minimized.

Show comment
Hide comment
@m-at-nu

m-at-nu Sep 27, 2016

Thanks a lot! In version 1.0.2 both loggers are working again :-).

m-at-nu commented Sep 27, 2016

Thanks a lot! In version 1.0.2 both loggers are working again :-).

@joschi

This comment has been minimized.

Show comment
Hide comment
@joschi

joschi Sep 27, 2016

Member

@m-at-nu Thanks for your feedback!

Member

joschi commented Sep 27, 2016

@m-at-nu Thanks for your feedback!

@joschi joschi closed this Sep 27, 2016

arteam added a commit that referenced this issue Nov 10, 2016

Bring back request logging with logback-classic
Several users reported that they have issues with the logback-access implementation.
See #1801, #1812, #1737. It would be great to allow these users to continue to use `logback-classic`,
because they could not move their appenders to `logback-access`.

arteam added a commit that referenced this issue Nov 11, 2016

Bring back request logging with logback-classic
Several users reported that they have issues with the logback-access implementation.
See #1801, #1812, #1737. It would be great to allow these users to continue to use `logback-classic`,
because they could not move their appenders to `logback-access`.

arteam added a commit that referenced this issue Nov 11, 2016

Bring back request logging with logback-classic
Several users reported that they have issues with the logback-access implementation.
See #1801, #1812, #1737. It would be great to allow these users to continue to use `logback-classic`,
because they could not move their appenders to `logback-access`.

arteam added a commit that referenced this issue Nov 14, 2016

Bring back request logging with logback-classic
Several users reported that they have issues with the logback-access implementation.
See #1801, #1812, #1737. It would be great to allow these users to continue to use `logback-classic`,
because they could not move their appenders to `logback-access`.

(cherry picked from commit 2893fba)

arteam added a commit that referenced this issue Nov 14, 2016

Bring back request logging with logback-classic
Several users reported that they have issues with the logback-access implementation.
See #1801, #1812, #1737. It would be great to allow these users to continue to use `logback-classic`,
because they could not move their appenders to `logback-access`.

(cherry picked from commit 2893fba)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment