control-service: add custom logback-spring.xml config

[mrMoZ1]

control-service: Enable sending logs to log insight.

Why: Recently we have had problems with deployment of data jobs.
Currently the control service only sends logs to the container's
stdout stream. This meant that deployment errors could be lost
due to log rolling. We have decided as a team to start sending
logs to log insight. This introduces a change that enables us
to do so.

What:Added a logback-spring.xml file which contains the properties
for logging. By default we will send logs to stdout still. However,
we now check if a particular environment variable is present and if
it is we also send logs to the host provided in the environment var.

Testing: Tested starting the service locally with and without the
specified env variable. Logs appear in loginsight when variable is
present. In both cases logs appear in stdout.

Signed-off-by: Momchil Zhivkov [email protected]

[antoniivanov]

Please Describe the aim of the change and why it is needed (as per the template recommendations 😄 )

I have some concerns about the solution but I'd like to understand first what is the aim of the change and then it may be clearer why this was the chosen approach.

[antoniivanov]

We need to be able to send logs from control plane to a remote host.

The aim of the change is described in this sentence only. The rest is very useful information describing the alternative approaches. It's great that it's here.
But we need to start from the use-cases before we go into options.

Can you expand it a bit. What do you mean "need to be able to send logs from control plane to a remote host" What logs, what remote host, why do we need to do that?

[antoniivanov]

Thanks for expanding the PR/commit message.

My concerns are is one we are making the helm chart configuration more complex. Is this a common enough use case that it needs to be visible at helm chart level ?

Any configuration exposed in values.yaml of the helm chart can be thought of as a contract between us and our users that we need to support.
Internal or dev only configuration is documented only at application.properties and not exposed directly. If it needs to be set it's set through extraEnvVars for example but it is not supported (e.i. users take the risk that it may break during an upgrade).

The second thing is. Is logback configuration the right interface? What happens if we change the logging backend to Log4J or some other logging implementation?

Beyond those two concerns, I have some notes about the current implementation that I'd make in the review but we can ignore them for now until we clear those 2 things

[mrMoZ1]

Thanks for expanding the PR/commit message.

My concerns are is one we are making the helm chart configuration more complex. Is this a common enough use case that it needs to be visible at helm chart level ?

Any configuration exposed in values.yaml of the helm chart can be thought of as a contract between us and our users that we need to support.
Internal or dev only configuration is documented only at application.properties and not exposed directly. If it needs to be set it's set through extraEnvVars for example but it is not supported (e.i. users take the risk that it may break during an upgrade).

The second thing is. Is logback configuration the right interface? What happens if we change the logging backend to Log4J or some other logging implementation?

Beyond those two concerns, I have some notes about the current implementation that I'd make in the review but we can ignore them for now until we clear those 2 things

Thanks for the feedback. I will try to address both concerns and maybe we can think of what to do next. One of the options would be to stick with this variant, the other option is to add the xml file directly in our super collider repo and add a reference to it in the shell script that builds the control plane container. My main concern with that one is passing an xml file to the EXTRA_JAVA_OPTS variable. Since we will need to reference a file in the EXTRA_JAVA_OPTS. Is that possible, do we need to pass the file to the container somehow? This is why IMO the easiest option was to include it in the helm charts. Another variant is to add a logback xml configuration file in the control-service project which will have to have the appender which sends data to a remote host. However this will change current logging behavior for all users.

To address the feedback:

1. Yes this change will make the helm configuration slightly more complex. However, this will be disabled by default and not so technically savvy users needn't worry about it if they don't understand what the extra configuration properties do. They can choose to leave the flag to false which is the default setting. This way default spring logging will be used. I would argue that having the ability to change the logging configuration is a common enough use case to warrant changes to the helm chart. Why? Different users might want to send logging data to a file, or to a remote log aggregator service, or they might want to change which packages get logged or fine tweak the log levels etc. However, this is what I think.
2. If, at some point in the future, we decide to change the logging backend, on top of everything else needed to change (if anything), we will also need to update the helm chart accordingly. This will consist of changing the default provided .xml configuration example to be compatible with the new logging framework. And maybe change the name of the configuration file passed to the spring property to something more appropriate.

Let me know how to proceed with this change. Should we pursue changing the helm chart, or choose one of the different options outlined above

[antoniivanov]

send logging data to a file, or to a remote log aggregator service

Currently in Kuberenetes environment recommendations are that application log to stdout/stderr and separate log collection service (like fluentd ) which would read the logs and forward them toward remote host.
Reference: fluentd https://github.com/vmware/kube-fluentd-operator and https://kubernetes.io/docs/concepts/cluster-administration/logging/

But this can be expensive as fluentd operator need to be installed .
So we need a cheaper alternative solution. So the benefit of this solution is that it's cheaper. But I think we should make clear that is not the recommended one though.

they might want to change which packages get logged or fine tweak the log levels

That's pretty valid case - but it's solved easily though environment variables. (--set extraEnvVars.LOGGING_LEVEL_COM_VMWARE_TAURUS=DEBUG as we do in our demo installation) I think this is spring-based variable, not logback (hence it would work even if we change the backend.

If, at some point in the future, we decide to change the logging backend on top of everything else needed to change ...

The problem here is not what we need to change. Changing our implementation is easy. The issue is that all of the customers that have custom logback based configuration - now would be broken.

First is that a big problem? How likely is logback to change?

Can we avoid that potential issue? slf4j (http://www.slf4j.org/) is meant to be a facade over the actual logging implementation. Is it possible to use it in some way here?
Do spring provide some interface ?

Also how does spring handle this problem ? They enable users to pick their own logging backend, right?

Let me know how to proceed with this change. Should we pursue changing the helm chart, or choose one of the different options outlined above

I am not sure yet, The helm chart seems the only practical option. But let's research if there's some alternative that doesn't make us depend on logback.

[antoniivanov]

The problem we need to solve is how to enable syslog appender in some installations, right ?

Alternative I think it is to reduce the scope of the change.

Just as you mentioned we can add syslog appender in our logback.xml
But we can make it configurable - e.g syslog host and port as env variable.
I'd argue it's not common enough that we do not need to expose them to the helm chart.

If they are not set (which would be default) , syslog appender would be disabled. If they are set , it would be enabled.

[ivakoleva]

Logging implementations
We are presently using Sprint Boot Starter 2.5.3:

 +--- org.springframework.boot:spring-boot-starter-logging:2.5.3
|    |    |    +--- ch.qos.logback:logback-classic:1.2.4
|    |    |    |    +--- ch.qos.logback:logback-core:1.2.4
|    |    |    |    \--- org.slf4j:slf4j-api:1.7.31 -> 1.7.32
|    |    |    +--- org.apache.logging.log4j:log4j-to-slf4j:2.14.1
|    |    |    |    +--- org.slf4j:slf4j-api:1.7.25 -> 1.7.32
|    |    |    |    \--- org.apache.logging.log4j:log4j-api:2.14.1
|    |    |    \--- org.slf4j:jul-to-slf4j:1.7.32
|    |    |         \--- org.slf4j:slf4j-api:1.7.32

Logback is the default starter implementation https://docs.spring.io/spring-boot/docs/2.5.3/reference/htmlsingle/#features.logging . Also:
Appropriate Logback routing is also included to ensure that dependent libraries that use Java Util Logging, Commons Logging, Log4J, or SLF4J all work correctly.
You may notice we use logback-spring.xml in base module, at the same time SLF4J Logger.

Using ENV variable
Passing an additional config via environment variable to application, definitely seems simplest to me, and would have probably approached it that way.

Using K8s volume to mount
This is a bit more complex, however you change logging configurations in runtime, setting the application to scan the config regularly e.g. each 5min. This is an advantage when troubleshooting, especially for applications that in a specific state reproduced (and restart would reset it).
Also, conceptually, Kubernetes is designed to handle application deployment concerns.