Structured logging with ziggurat #238
Conversation
anmol1vw13
force-pushed
the
structured_logging
branch
from
5af179f
to
53cb6c0
Compare
anmol1vw13
force-pushed
the
structured_logging
branch
from
bcb1d38
to
bd93b2a
Compare
| @@ -30,8 +30,6 @@ | |||
| [io.opentracing.contrib/opentracing-rabbitmq-client "0.1.11" :exclusions [com.rabbitmq/amqp-client]] | |||
| [org.apache.httpcomponents/fluent-hc "4.5.13"] | |||
| [org.apache.kafka/kafka-streams "2.8.0" :exclusions [org.slf4j/slf4j-log4j12 log4j]] | |||
| [org.apache.logging.log4j/log4j-core "2.14.1"] | |||
There was a problem hiding this comment.
Why are we removing these? Both loggings should be behind feature toggle initially.
There was a problem hiding this comment.
We have text and json logging behind feature flag. Having both implementations present was causing none of them to be picked for some reason (Didn't debug further). Hence removed log4j altogether.
There was a problem hiding this comment.
We have text and json logging behind feature flag
What do you mean? Text logs are supported how? Will this not be backward incompatible?
There was a problem hiding this comment.
Reason why we cannot have multiple slf4j implementations
SLF4J API is designed to bind with one and only one underlying logging framework at a time. If more than one binding is present on the class path, SLF4J will emit a warning, listing the location of those bindings.
When multiple bindings are available on the class path, select one and only one binding you wish to use, and remove the other bindings. For example, if you have both slf4j-simple-2.0.0-alpha5.jar and slf4j-nop-2.0.0-alpha5.jar on the class path and you wish to use the nop (no-operation) binding, then remove slf4j-simple-2.0.0-alpha5.jar from the class path.
The list of locations that SLF4J provides in this warning usually provides sufficient information to identify the dependency transitively pulling in an unwanted SLF4J binding into your project. In your project's pom.xml file, exclude this SLF4J binding when declaring the unscrupulous dependency.
There was a problem hiding this comment.
What do you mean? Text logs are supported how? Will this not be backward incompatible?
text logs with MDC are supported via logback itself, for reference please check logback.test.xml in this PR. To ensure multiple bindings are not present we have added a section in the doc. We will add in the upgrade guide as well
| </then> | ||
| </if> | ||
|
|
||
| <root level="error"> |
There was a problem hiding this comment.
this should be level="info"?
There was a problem hiding this comment.
by default info makes sense.
There was a problem hiding this comment.
info will printout kakfa info logs during tests, in the older log4j2.test.xml the level was set to error
| @@ -85,4 +85,5 @@ | |||
| :value-deserializer-class-config "org.apache.kafka.common.serialization.ByteArrayDeserializer"}} | |||
| :tracer {:enabled [true :bool] | |||
| :custom-provider ""} | |||
| :new-relic {:report-errors false}}} | |||
| :new-relic {:report-errors false} | |||
| :log-format "text"}} | |||
There was a problem hiding this comment.
default log format in test should be json?
There was a problem hiding this comment.
json wouldn't be required in tests. It isn't very readable
There was a problem hiding this comment.
I would say use default log format as json
resources/logback.test.xml
Outdated
| </jsonFormatter> | ||
| <!-- <context>api</context> --> | ||
| <timestampFormat>yyyy-MM-dd'T'HH:mm:ss.SSS'Z'</timestampFormat> | ||
| <timestampFormatTimezoneId>UTC</timestampFormatTimezoneId> |
There was a problem hiding this comment.
Does this make sense to hardcode the timezone?
There was a problem hiding this comment.
We should let it pick the timezone and not hardcode
| @@ -39,7 +39,8 @@ | |||
| :http-server {:middlewares {:swagger {:enabled false}} | |||
| :port 8080 | |||
| :thread-count 100} | |||
| :new-relic {:report-errors false}}}) | |||
| :new-relic {:report-errors false} | |||
| :log-format "text"}}) | |||
There was a problem hiding this comment.
initially it would be better if the default were text. This will allow users to make a gradual shift to JSON logging
src/ziggurat/init.clj
Outdated
| @@ -247,6 +253,7 @@ | |||
| (try | |||
| (let [derived-modes (validate-modes modes stream-routes batch-routes actor-routes)] | |||
| (initialize-config) | |||
| (set-properties-for-structured-logging) | |||
There was a problem hiding this comment.
should be part of initialize-config rather than being a function here in this namespace.
* Add structured logs to ziggurat logs * Fix linting * changes log formats and removes timezone block * adds logging context to mapper flow and rabbitmq consume flow * Resolve PR comments Co-authored-by: Anmol Vijaywargiya <anmol.vijaywargiya@go-jek.com> Co-authored-by: shubhang.balkundi <2932-shubhang.balkundi@users.noreply.source.golabs.io>
No description provided.