Page MenuHomePhabricator

Remove jsonevent-layout library from our Gerrit fork
Open, Needs TriagePublic

Description

In operations/software/gerrit we have a single cherry-pick on top of upstream stable-3.2 branch:

185bdc3a69370c3ab96cc94d20c0746fe6bade56

That is to restore the jsonevent-layout library from Maven. It is used to generate json error logs which are then relayed to logstash:

modules/gerrit/files/homedir/review_site/etc/log4j.xml
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE log4j:configuration SYSTEM "log4j.dtd">
<log4j:configuration xmlns:log4j="http://jakarta.apache.org/log4j/">
...
    <appender name="gerrit.json" class="org.apache.log4j.DailyRollingFileAppender">
        <param name="encoding" value="UTF-8"/>
        <param name="File" value="/var/log/gerrit/gerrit.json"/>
        <!--<param name="fileAppend" value="true"/>-->
        <param name="immediateFlush" value="true"/>
        <param name="threshold" value="INFO"/>
        <layout class="net.logstash.log4j.JSONEventLayoutV1"/>
    </appender>

Eventually I have found out the Gerrit daemon is able to generate the file all by itself by enabling log.jsonLogging (default: false). https://gerrit.wikimedia.org/r/Documentation/config-gerrit.html#log.jsonLogging . That has been possible since Gerrit 2.12.

Theorically we can thus turn on jsonLogging in Gerrit, remove the log4j appender and we would no more depend on jsonevent-layout.

A couple things to check:

  • whether our logstash receiver will properly recognizes it
  • log.jsonLogging also acts on the httpd and sshd logs.

Event Timeline

hashar created this task.Nov 17 2020, 1:50 PM

In /var/log/gerrit we have:

delete_loglog4j.xml
gc_loglog4j.xml
gerrit.jsonlog4j.xml net.logstash.log4j.JSONEventLayoutV1
gerrit.loglog4j.xml org.apache.log4j.PatternLayout
plugin_loglog4j.xml
replication_loglog4j.xml
sshd_loglog4j.xml com.google.gerrit.sshd.SshLogLayout

The systemd journal shows nothing.

Gerrit has built-in support to write:

fileEnable with
httpd_loglog.textLogging (default true)
error_loglog.textLogging (default true)
sshd_logsshd.requestLog (default true)

httpd_log and error_log do not show up, I guess because we overwrite the default log4j configuration via -Dlog4j.configuration.

I briefly tested log.jsonLogging for the 3.2 upgrade. According to my notes, the format for @timestamp is slightly different (Z as timezone vs. +0000. That's more of a pain on the Java side than one wants to), the @version differs, and IIRC exceptions are slightly different.

But one can surely work around all these with a bit of hacking.

What made me stick with jsonevent-layout is that:

  • it was what we had there and the upgrade was big enough already (which is now of course moot :-) )
  • logging in Java is a bit messy aleady and Gerrit was not the best citizen here in the past. So I'd not rely on anything that lives inside Gerrit, and instead would go with massaging log messages outside of Gerrit (like jsonevent-layout did and does).

But there is actually nothing requiring us to have the jar in the gerrit war file. One could put the jsonevent-layout jar $SOMEWHERE on the gerrit hosts and then simply add them to classpath (i.e.: in the systemd service file) and it would get picked up accordingly. That would allow to use the upstream vanilla gerrit.war and still keep the massaging of log messages outside of Gerrit.

(Obviously, none of the above are a blocker for using log.jsonLogging nonetheless)

And surely passing -Dlog4j.configuration disables the Gerrit builtin logs which have support for log.jsonLogging :-\

public static boolean shouldConfigure() {
  return Strings.isNullOrEmpty(System.getProperty(LOG4J_CONFIGURATION));
}

Thank you @QChris for your input, it is very helpful to have all the context. My intent for this task is to drop the cherry pick. I will look at shipping the jsonevent-layout jar outside of the gerrit.war :]

Thank you @QChris for your input, it is very helpful to have all the context. My intent for this task is to drop the cherry pick. I will look at shipping the jsonevent-layout jar outside of the gerrit.war :]

jsonevent-layout has its own dependencies, most of those dependencies are inside gerrit already with the exception of json-smart, so we'll need both of those in the class path.

Also, if we start using upstream packages (which I think is probably the purpose of this task) we should move to openjdk-11 per: https://www.gerritcodereview.com/3.2.html#native-packaging

From Luca, we can put the extra .jar in $GERRIT_SITE/lib like we did previously for the mysql connector. Gerrit adds that path to CLASSPATH and it thus discovers jar in there automagically. Or in short: no need to mess with systemd configuration / environment variables.

This comment was removed by QChris.
QChris added a comment.EditedNov 18 2020, 9:07 PM

From Luca, we can put the extra .jar in $GERRIT_SITE/lib like we did previously for the mysql connector. Gerrit adds that path to CLASSPATH and it thus discovers jar in there automagically. Or in short: no need to mess with systemd configuration / environment variables.

I did not test this, so I might be totally off-track here. And maybe that also changed with one of Gerrit's logging backend switches, but loading from $GERRIT_SITE/lib used to happen only after the logging got set up. As most relevant classes still have their logger in a static field, logging would get initialized very early on, before the jar from $GERRIT_SITE/lib got loaded. But then again Flogger is trying to do a few things lazily these days, so that might help or mitigate.

If $GERRIT_SITE/lib works, it's certainly nice. But if it doesn't, I'd not bother much and go for adding the class path in the systemd service file, as WMF has been sunk quite some time with $GERRIT_SITE/lib issues in the past already.

Thank you for mentioning load order might be an issue and actually has hit us in the past. I will give it a try at least for educational purpose.

I have looked at the systemd unit, it sources an environment file which is generated by Puppet. Adding CLASSPATH=/var/lib/gerrit2/review_site/lib/* is thus straightforward.

I will look at fetching the .jar we need and have them included in our deploy/wmf/stable-3.2 branch under a lib directory. Then have Puppet to symlink /var/lib/gerrit2/review_site/lib at /srv/deployment/gerrit/gerrit/lib and I guess we will be set :]