fix(logging): Log 2 json #34
Conversation
There was a problem hiding this comment.
Seems sensible to me, though I don't have the required domain knowledge.
@bsanchezb, might know better
| @@ -19,9 +19,13 @@ FROM tomcat:9 | |||
|
|
|||
| RUN apt-get update && apt-get upgrade -y | |||
|
|
|||
| RUN wget -O dd-java-agent.jar https://dtdg.co/latest-java-tracer | |||
| RUN wget -O dd-java-agent.jar https://dtdg.co/latest-java-tracer && \ | |||
There was a problem hiding this comment.
Consider adding a comment with a link to the documentation where you're getting this from
| @@ -266,6 +270,19 @@ | |||
| <artifactId>logback-classic</artifactId> | |||
| </dependency> | |||
|
|
|||
| <dependency> | |||
| <groupId>ch.qos.logback.contrib</groupId> | |||
| <artifactId>logback-jackson</artifactId> | |||
There was a problem hiding this comment.
Is this correct?
| <artifactId>logback-jackson</artifactId> | |
| <artifactId>logback-json</artifactId> |
There was a problem hiding this comment.
Yes - this is used by the logback config <jsonFormatter class="ch.qos.logback.contrib.jackson.JacksonJsonFormatter"/>
| export CATALINA_OPTS="$CATALINA_OPTS -javaagent:./dd-java-agent.jar" | ||
| export CLASSPATH="$CLASSPATH:/usr/local/tomcat/bin/ecs-logging-core-1.6.0.jar:/usr/local/tomcat/bin/jul-ecs-formatter-1.6.0.jar" |
There was a problem hiding this comment.
Is your intention to append or prepend to the classpath? Here you're appending.
Also, consider adding a comment
| RUN wget -O dd-java-agent.jar https://dtdg.co/latest-java-tracer | ||
| RUN wget -O dd-java-agent.jar https://dtdg.co/latest-java-tracer && \ | ||
| wget -O /usr/local/tomcat/bin/jul-ecs-formatter-1.6.0.jar https://repo1.maven.org/maven2/co/elastic/logging/jul-ecs-formatter/1.6.0/jul-ecs-formatter-1.6.0.jar && \ | ||
| wget -O /usr/local/tomcat/bin/ecs-logging-core-1.6.0.jar https://repo1.maven.org/maven2/co/elastic/logging/ecs-logging-core/1.6.0/ecs-logging-core-1.6.0.jar |
There was a problem hiding this comment.
Try to avoid adding new jars directly. Add the dependency within pom.xml instead: https://mvnrepository.com/artifact/co.elastic.logging/ecs-logging-core/1.6.0
There was a problem hiding this comment.
Those have to be accessible on the path somewhere so that the JVM logs can be formatted. Note - this is only necessary for the tomcat/catalina log output formatting and not the application so technically not a part of the project (e.g. ef you were to migrate to spring boot this would become redundant).
I can add those via maven - do you know where those jar then end up as I will have to expose them to the JVM by putting them on the path somewhere (see setenv.sh)
There was a problem hiding this comment.
The jars will be part of a WAR package that will be running from Tomcat.
With the current approach your logger will work only within a Docker container with unavailability to test it locally (unless you provide the missing jars manually).
There was a problem hiding this comment.
Regarding the Spring-boot update, I may certainly assist you with that. Feel free to get back to me by email.
There was a problem hiding this comment.
Maybe I was unclear here - These jar files are only for tomcat log output, and not application logs - so unless your local workflow is to spin up a tomcat, build WAR and deploy via tomcat those logs will never be seen locally. Hopefully I am making sense :)
| <layout class="ch.qos.logback.contrib.json.classic.JsonLayout"> | ||
| <timestampFormat>yyyy-MM-dd'T'HH:mm:ss.SSSX</timestampFormat> | ||
| <jsonFormatter class="ch.qos.logback.contrib.jackson.JacksonJsonFormatter"/> | ||
| </layout> |
There was a problem hiding this comment.
Does it store the logs to a separate file or prints them to console? If I remember well, the requirement was to store the logs to a file. I see that "ch.qos.logback.core.ConsoleAppender" is used.
There was a problem hiding this comment.
There is no need to store logs on the file system. This is being run in a containerized environment with ephemeral storage - we are feeding those log to our centralized logging system (data dog) where logs are stored and managed. It is actually preferable NOT to store logs on the file system as it can cause disks to fill up
There was a problem hiding this comment.
Ok, I misunderstood the request then. You may also change the appender to "ch.qos.logback.core.ConsoleAppender" in my branch, to make logs reported within a console.
|
Dear Roberta, dear Kristofer, I have added a few comments that caught my attention. I did not work with ecs logger formatter, so I cannot provide you with more information on the used tool and implementation without testing. If you think it works better, feel free to keep the implementation. Nevertheless, I would highly recommend to avoid modifications within Dockerfile or/and setenv.sh, and include the required configuration directly within pom.xml file. Best regards, |
Same as, and supersedes #20 and #21