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

Add support for JSON logs in Dropwizard #2232

Merged
merged 1 commit into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@arteam
Member

arteam commented Dec 14, 2017

The new dropwizard-json-log module allows users to configure a specific layout to convert logging message to JSON. The module supports event logs and access logs like that:

For general logging:

    logging:
      appenders:
        - type: console
          layout:
            type: json

For request logging:

    server:
      requestLog:
        appenders:
          - type: console
            layout:
              type: access-json

The default format produces logs like:

 {"level":"INFO","logger":"org.eclipse.jetty.server.Server","thread":"main","message":"Started @6505ms","timestamp":"2018-01-03T19:01:37.674Z"}

and access logs like

   {"method":"GET","userAgent":"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0","uri":"/hello-world","requestTime":5,"protocol":"HTTP/1.1","contentLength":37,"remoteAddress":"127.0.0.1","timestamp":"2018-01-03T19:04:48.448Z","status":200}

Solves #1388, #1451 as an alternative to #2180

@arteam arteam force-pushed the dropwizard_json_logging branch from 39d4ea4 to 7d42e15 Dec 14, 2017

@jplock jplock added this to the 1.3.0 milestone Dec 15, 2017

@jplock jplock added the feature label Dec 15, 2017

@arteam arteam force-pushed the dropwizard_json_logging branch 5 times, most recently from 88d9fed to 5a84b60 Dec 15, 2017

@arteam arteam force-pushed the dropwizard_json_logging branch 4 times, most recently from e398ae8 to 6b4cf69 Dec 29, 2017

@evnm

A few nits, mostly on documentation.

======================= =========== ================
Name Default Description
======================= =========== ================
includeTimestamp true Whether to include the timestamp of the event to the JSON map.

This comment has been minimized.

@evnm

evnm Jan 1, 2018

Member

Here and below, I think the phrasing should be "include x in the JSON map" rather than "include x to the JSON map".

Name Default Description
======================= =========== ================
includeTimestamp true Whether to include the timestamp of the event to the JSON map.
timestampFormat (none) By default, the timestamp is not formatted; To format the timestamp using set the property with the

This comment has been minimized.

@evnm

evnm Jan 1, 2018

Member

"To customize how timestamps are formatted, set the property to the corresponding DateFormatter string"

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

Done

includeMdc true Whether to include the MDC properties to the JSON message as the ``mdc`` field.
includeLoggerName true Whether to include the logger name to the JSON message as the ``logger`` field.
includeFormattedMessage true Whether to include the formatted message to the JSON message as the ``message`` field .
The raw (unformatted) message is available as ``includeMessage``. Most people will

This comment has been minimized.

@evnm

evnm Jan 1, 2018

Member

Reword last sentence here.

want the formatted message as the raw message does not reflect any log message
arguments.
includeMessage true Whether to include the raw message to the JSON message as the ``raw-message`` field.
includeException true Whether to log exceptions. If the property enabled and there is an exception,

This comment has been minimized.

@evnm

evnm Jan 1, 2018

Member

Ditto, reword last sentence.

* <td>Whether to include the thread name.</td>
* </tr>
* <tr>
* <td>{@code includeMdc}</td>

This comment has been minimized.

@evnm

evnm Jan 1, 2018

Member

This is includeMDC (final two chars capitalized) below.

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

Done

import java.time.format.DateTimeFormatter;
/**
* A more fast timestamp formatter than the default one in `logback-contrib`.

This comment has been minimized.

@evnm

evnm Jan 1, 2018

Member

"more fast" -> "faster"

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

Done

@arteam arteam force-pushed the dropwizard_json_logging branch 6 times, most recently from 3a0659a to 51af1cc Jan 1, 2018

@sabarivasan

This comment has been minimized.

sabarivasan commented Jan 2, 2018

Hi @arteam, @evnm, let me know if we can help in any way to move this along. As Brent Ryan said, we at Cvent are waiting on this 1.3.0 milestone internally before we upgrade to latest dropwizard version. We'd strongly prefer not do this off a fork.

@arteam arteam force-pushed the dropwizard_json_logging branch 5 times, most recently from 6a641f0 to 4a49f57 Jan 2, 2018

includeRequestURL: false
includeRemoteHost: false
includeServerName: false
includeRequestHeaders: false

This comment has been minimized.

@mattnelson

mattnelson Jan 2, 2018

Contributor

I would like to see the option to explicitly opt into specific mdc fields and request/response headers instead of all. Logging all request headers would make it quite easy to accidentally log an Authorization header.

requestHeaders:
  - X-Request-Id
responseHeaders:
  - X-Request-Id

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

Yes, that's makes much more sense.

prettyPrint: false
includeTimestamp: true
appendLineSeparator: true
includeRemoteAddr: false

This comment has been minimized.

@mattnelson

mattnelson Jan 2, 2018

Contributor

The include prefixes seem unnecessary.

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

It looks to me it makes sense to represent included boolean options as a set:

include:
  - remoteAddress
  - remoteUser
  - requestTime

or

include: [remoteAddress, remoteUser, requestTime]
Name Default Description
======================= =========== ================
includeTimestamp true Whether to include the timestamp of the event to the JSON map.
timestampFormat (none) By default, the timestamp is not formatted; To format the timestamp using set the property with the

This comment has been minimized.

@mattnelson

mattnelson Jan 2, 2018

Contributor

Should there also be support for the predefined formats? Would make it easier to adopt standard formatting and reduce the possibility of typos in the format string while still leaving the option for custom formatting.

timestampFormat: yyyy-MM-dd HH:mm:ss.SSSZ
timestampFormat: ISO_DATE_TIME

https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#predefined

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

Should be easy to implement and a nice feature for simplifying configs!

includeProtocol true Whether to include the request HTTP protocol in the JSON map as the ``protocol`` field.
includeContentLength true Whether to include the response content length, if it's known in the JSON map as the ``contentLength`` field.
includeUserAgent true Whether to include the user agent of the request in the JSON map as the ``userAgent`` field.
includeRequestURL false Whether to include the request URL (method, URI, query parameters, protocol) in the JSON map as the ``contentLength`` field.

This comment has been minimized.

@mattnelson

mattnelson Jan 2, 2018

Contributor

Update description to the correct field name.

This comment has been minimized.

@arteam

arteam Jan 2, 2018

Member

Will do

@arteam arteam force-pushed the dropwizard_json_logging branch 4 times, most recently from e6310cc to 38b04e5 Jan 2, 2018

@arteam arteam force-pushed the dropwizard_json_logging branch 4 times, most recently from 7ebb47e to 5d1e4e2 Jan 3, 2018

@arteam arteam changed the title from WIP: Work on Dropwizard json log to Add support for JSON logs in Dropwizard Jan 3, 2018

@arteam arteam force-pushed the dropwizard_json_logging branch 3 times, most recently from 04649a7 to ac8a034 Jan 3, 2018

@arteam

This comment has been minimized.

Member

arteam commented Jan 3, 2018

Okay, this is now looks good to merge to me. @evnm @mattnelson Could you have a look?

@mattnelson

Looks good, only a few nits.

return ImmutableMap.of();
}
return headers.entrySet().stream()
.filter(e -> filteredHeaderNames.stream().anyMatch(h -> h.equalsIgnoreCase(e.getKey())))

This comment has been minimized.

@mattnelson

mattnelson Jan 3, 2018

Contributor

Can the filteredHeaderNames be put into a case insensitive TreeSet? Then this nested loop could be converted to a contains.

This comment has been minimized.

@arteam

arteam Jan 4, 2018

Member

Good idea! I will use ImmutableSortedSet from Guava for that (the same behavior, but it's a contagious set and provides a better memory layout than TreeSet).

@@ -767,6 +767,126 @@ Name Default Description
type REQUIRED The filter type.
====================== =========== ================
.. _man-configuration-json-layout:
JSON layout

This comment has been minimized.

@mattnelson

mattnelson Jan 3, 2018

Contributor

I think it would help to see some examples of the json produced in the docs.

This comment has been minimized.

@arteam

arteam Jan 4, 2018

Member

Right! I will add some examples from the commit.

prettyPrint false Whether the JSON output should be formatted for human readability.
appendLineSeparator true Whether to append a line separator at the end of the message formatted as JSON.
includes (timestamp, level,
threadName, mdc,

This comment has been minimized.

@mattnelson

mattnelson Jan 3, 2018

Contributor

Do you want to support mdc filtering? I can see both sides, include all mdc keys or support a whitelist of mdc keys. Generally I think you would want to include all in the case of dynamic diagnostics, but there may users that are abusing MDC as a way to share state. This is something that can be addressed in a follow up if there is demand, as long as that change could be passively introduced later.

This comment has been minimized.

@arteam

arteam Jan 4, 2018

Member

Looks like a good idea. https://github.com/logstash/logstash-logback-encode and https://github.com/mp911de/logstash-gelf support filtering of MDC, so it makes sense to us to support it too.

<dependency>
<groupId>io.dropwizard</groupId>
<artifactId>dropwizard-json-logging</artifactId>
<version>{$dropwizard.version}</version>

This comment has been minimized.

@jplock

jplock Jan 3, 2018

Member

nit: ${dropwizard.version}

This comment has been minimized.

@arteam

arteam Jan 4, 2018

Member

Good catch!

@arteam arteam force-pushed the dropwizard_json_logging branch 2 times, most recently from 824bf71 to bd8491d Jan 4, 2018

Add support for JSON logs in Dropwizard
The new `dropwizard-json-log` module allows users to configure
a specific layout to convert logging message to JSON. The module
supports event logs and access logs.

The default format produces logs like:
``
{"level":"INFO","logger":"org.eclipse.jetty.server.Server","thread":"main","message":"Started @6505ms","timestamp":"2018-01-03T19:01:37.674Z"}
``
and access logs like
``
{"method":"GET","userAgent":"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0","uri":"/hello-world","requestTime":5,"protocol":"HTTP/1.1","contentLength":37,"remoteAddress":"127.0.0.1","timestamp":"2018-01-03T19:04:48.448Z","status":200}
``

@arteam arteam force-pushed the dropwizard_json_logging branch from bd8491d to ab2b0e7 Jan 4, 2018

@arteam

This comment has been minimized.

Member

arteam commented Jan 4, 2018

@evnm @jplock @mattnelson Please review one more time.

@evnm

evnm approved these changes Jan 5, 2018

@arteam arteam merged commit 6e5c9c5 into master Jan 5, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@arteam arteam deleted the dropwizard_json_logging branch Jan 5, 2018

@arteam

This comment has been minimized.

Member

arteam commented Jan 5, 2018

Thanks everyone!

@arteam

This comment has been minimized.

Member

arteam commented Jan 5, 2018

@sabarivasan I've released Dropwizard 1.3.0-rc2 to Maven Central. You can grab it from there and experiment with JSON logs.

aaanders added a commit to aaanders/dropwizard that referenced this pull request Sep 20, 2018

Add support for JSON logs in Dropwizard (dropwizard#2232)
The new `dropwizard-json-log` module allows users to configure
a specific layout to convert logging message to JSON. The module
supports event logs and access logs.

The default format produces logs like:
``
{"level":"INFO","logger":"org.eclipse.jetty.server.Server","thread":"main","message":"Started @6505ms","timestamp":"2018-01-03T19:01:37.674Z"}
``
and access logs like
``
{"method":"GET","userAgent":"Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0","uri":"/hello-world","requestTime":5,"protocol":"HTTP/1.1","contentLength":37,"remoteAddress":"127.0.0.1","timestamp":"2018-01-03T19:04:48.448Z","status":200}
``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment