Add configurable envars under mdc logging

[yma955]

This envars worked for Datahub kafka consumer(local) test, message output is like: http://pastebin.test.redhat.com/741859

[yma955]

I'm not sure what's the appropriate default values here, correct it if needed pls.

[ruhan1]

I would suggest we don't config those. It will be a burden before deploying a new instance. And, I wonder whether we can get it before building the image? i.e., how to get openshift buildname/commit before we make the build? I guess the config doesn't know the info. I might miss something but this sounds strange to me.

[yma955]

I will also confirm under NOS-1658, thanks.

[jdcasey]

I'd agree with this, but just to the extent that we shouldn't have named methods for those envars at all. In fact, we shouldn't have getter methods for any of these envars...that will limit how much flexibility we have to tailor the list of envars in the future. See my comment above for how I'd like this configuration to work...it will even accommodate aliasing, so we can respond to future OpenShift changes.

[ruhan1]

might just use the above injected one?

[yma955]

yeah, I will remove in new patch

[jdcasey]

My main concern is that LoggingConfig should focus on configuring which environment variable keys to extract, and what names to use when injecting them into the MDC.

We might also rename LoggingConfig (maybe I got the name wrong there) to something like MDCConfig, since we're really tuning what the MDC contains.

Looking more closely at this, I'm concerned that we won't get context information on things that happen as a result of cache listeners, expired content, etc. since they don't start with ResourceManagementFilter. I think we may need to adjust this to work through the layout after all, as was the original plan. If we did that, we could have context info regardless of where the work started. To do this, we could let the logging system set itself up using the class name, then use CDI.getCurrent() to get the injection context for looking up the MDCConfig...or we could push the configuration information directly into the layout config itself, in the XML, using a property that passed an inlined version of these mappings when logback initialized. The format for an XML configuration for this might look like:

<environment-variables>OPENSHIFT_BUILD_NAMESPACE=BUILD_NAMESPACE, HOSTNAME=HOSTNAME</environment-variables>

But I think we have to find a way to make all log messages include this information, so we can tell where they're coming from. If we don't, it will actually increase confusion in the aggregated log, which will be worse than just not having those log messages at all.

[jdcasey]

I'd agree with this, but just to the extent that we shouldn't have named methods for those envars at all. In fact, we shouldn't have getter methods for any of these envars...that will limit how much flexibility we have to tailor the list of envars in the future. See my comment above for how I'd like this configuration to work...it will even accommodate aliasing, so we can respond to future OpenShift changes.

[jdcasey]

See my comment above about format. We should never need to fill in actual values here, the point we're going for is to pull them out of the system environment variables, allowing OpenShift to set them when it spins up a pod.

[jdcasey]

Could we iterate through the keys in the LoggingConfig and put them directly into the MDC instead of putting them into a new map under it?

BTW, any environment we can't read (isn't declared on the system) should have the value UNKNOWN added, so we can detect them in the log output and use that to fix the problem in the pod / deployment.

[ruhan1]

If we are going to send them as a JSON to common logging, to group them in a map would be better. We have other MDC entries like the request-id, etc. If we set the env vars directly, we need to be sure not to override existing keys. We may add new keys in the future and slowly this could be a burden to maintain. But I am not sure how to refer it in logback.xml, like this? %X{evn.hostname} . If we don't need to print it directly to log file, that is no question though.

[ruhan1]

+1 for renaming it to something else. The LoggingConfig makes me think about to control sth about logback.xml. Now I know it is about MDC-to-env key-to-key mapping. how about EnvironmentConfig?

@yma88 for the question about the configuration, if you find you will need the parameter() (which is flexible), just remove those annotations on setter methods, or remove setters themselves. Because in this case, the setting is done in the parameter(), your bean only needs getters.

[yma955]

Hi, @jdcasey @ruhan1 thanks very much for your detailed comments, I understand your points and on the way of turning code.
For IndyLoggingConfig rename, I may aggree with @ruhan1 naming it as 'EnvironmentConfig' and put the env as a map under mdc, since EnvironmentConfig is more obvious and will not make others confused on the mdc entries assembled, because we have other fixed mdc entires defined in RequestContextConstants. @jdcasey Do you have other further consideration on this I may miss?

[ruhan1]

Looks good to me.

[jdcasey]

Looks good. I like the idea of handling the envar map setup during configuration. At first that was confusing to me, but looking more closely it's clear that it will perform better than pulling those envars each time, and those values should not change during the pod lifecycle.