Armeria: Support prefix option for Logback integration

[okue]

Support prefix option <exportPrefix> for Logback integration to specify the prefix of exported MDC properties.

For example, if this xml is loaded

  <appender name="RCEA" class="com.linecorp.armeria.common.logback.RequestContextExportingAppender">
    <!-- specify the prefix of exported MDC properties -->
    <exportPrefix>armeria</exportPrefix>
    <export>req.*</export>
    <export>res.*</export>
    ...
  </appender>

then RequestContextExportingAppender exports armeria.req.path, armeria.req.service_name, armeria.res.status_code, etc.

[okue]

Or support prefix attribute like <exports prefix="armeria"> ... </exports>.

In this case, we can write the following,

  <appender name="RCEA" class="com.linecorp.armeria.common.logback.RequestContextExportingAppender">
    <!-- append `armeria.` as prefix -->
    <exports prefix="armeria">req.*, res.*</exports>
    <!-- no prefix -->
    <export>some_value=attr:com.example.AttrKeys#SOME_KEY<export>
  </appender>

and RequestContextExportingAppender exports armeria.req.*, armeria.req.*, and some_value.
Note that no prefix is appended to some_value property.

[okue]

I cannot find how to give an attribute to custom Logback appender in the document and baeldung blog.
If it may be impossible to support <exports prefix="armeria">...</exports>, supporting <exportsNoPrefix> may be the solution.

On the other hand, it seems no problem to support <exportPrefix> tag.
(at my fork https://github.com/okue/armeria/pull/4)

[trustin]

How about allowing an empty value for exportPrefix? e.g.

<exportPrefix>armeria</exportPrefix>
<exports prefix="armeria">req.*, res.*</exports>
<exportPrefix />
<export>some_value=attr:com.example.AttrKeys#SOME_KEY<export>

This basically assumes that the tags are translated into sequential calls to RequestContextExporterBuilder, but I think this assumption will not fail.

[okue]

If we can assume that the tags are translated sequentially from top to bottom,
supporting only <exportPrefix> (allowing an empty string) is sufficient!

[okue]

If supporting <exportPrefix> tag is accepted, I'll send a PR ^ ^

[trustin]

Awesome, @okue. +1 for adding <exportPrefix />. @minwoox @ikhoon WDYT?

[ikhoon]

<exportPrefix /> looks nice!

If supporting <exportPrefix> tag is accepted, I'll send a PR ^ ^

Thanks in advance. 😉

[minwoox]

How about something like:

  <appender name="RCEA" class="com.linecorp.armeria.common.logback.RequestContextExportingAppender">
    <!-- specify the prefix of exported MDC properties -->
    <exports prefix="armeria">
      <export>req.*</export>
      <export>res.*</export>
    </exports>
    <export>some_value=attr:com.example.AttrKeys#SOME_KEY<export>
    ...
  </appender>

Not sure if it's easy though.

[okue]

I don't know if we can give this xml to custom logback appender.

    <exports prefix="armeria">
      <export>req.*</export>
      ...
    </exports>

[minwoox]

Logback uses Javabeans introspection so I thought it's possible but I'm not sure because I never tried that. 😄
Let me try first and then I will tell you if we can do that or not.

[okue]

Let me try first and then I will tell you if we can do that or not.

Thanks 😭

[minwoox]

What I found out so far is that we can do:

  <appender name="RCEA" class="com.linecorp.armeria.common.logback.RequestContextExportingAppender">
    <exportPrefix>
      <prefix>armeria</prefix>
      <export>req.*</export>
      <export>res.*</export>
    </exportPrefix>
    <export>some_value=attr:com.example.AttrKeys#SOME_KEY<export>
    ...
  </appender>

I couldn't find a way to do this:

<exports prefix="armeria">

because it seems like Logback does not support adding the attributes of the nested property.
The exportPrefix class is created here:
https://github.com/qos-ch/logback/blob/v_1.2.3/logback-core/src/main/java/ch/qos/logback/core/joran/action/NestedComplexPropertyIA.java#L121
and just uses the methods to set the values (setter and adder):
https://github.com/qos-ch/logback/blob/v_1.2.3/logback-core/src/main/java/ch/qos/logback/core/joran/util/beans/BeanDescription.java#L16
So there's no inherent function to add the attribute. 🤔

[trustin]

How about <exportGroup> ?

<!-- If not wrapped with exportGroup, all directives go to the 'default export group'. -->
<prefix>...</prefix> <!-- This sets the prefix for all exports in the default export group. -->
<export>...</export>
<exports>...</exports>

<!-- exportGroup creates a new export group. -->
<exportGroup>
  <prefix>...</prefix> <!-- This sets the prefix for the exports in its group only. -->
  <export>...</export>
  <exports>...</exports>
</exportGroup>

<exportGroup>
  <export>...</export>
  <prefix>...</prefix> <!-- A user should be able to specify a prefix later -->
</exportGroup>

<exportGroup>
  <!-- No prefix tag means 'inherit from the default export group. -->
  <export>...</export>
</exportGroup>

One thing to think about is whether we need to allow an empty prefix, i.e. <prefix />. It looks somewhat confusing, so it's probably better throwing an exception? Users can still achieve what they want with proper grouping of exports:

<exportGroup>
  <export>...</export>
</exportGroup>
<exportGroup>
  <prefix>...</prefix>
  <export>...</export>
</exportGroup>

We might also want to an additional tag that excludes some exports:

<!-- Add armeria prefix for all request properties except req.foo. -->
<export>req.foo</export>
<exportGroup>
  <prefix>armeria</prefix>
  <exports>req.*</exports>
  <exclusion>^req\.foo$</exclusion> <!-- regex -->
</exportGroup>

Makes sense?

[trustin]

I'm also curious if a user will ever want to export one property into multiple MDC properties, e.g. req.foo to a.req.foo and b.req.foo:

<exportGroup>
  <prefix>a</prefix>
  <export>req.foo</export>
</exportGroup>
<exportGroup>
  <prefix>b</prefix>
  <export>req.foo</export>
</exportGroup>

IIRC we do not support this use case, or do we?

[minwoox]

How about ?

That looks good. 😄

i.e. . It looks somewhat confusing, so it's probably better throwing an exception?

+1

No prefix tag means 'inherit from the default export group

Shouldn't we also raise an exception because it doesn't mean anything?

[trustin]

Shouldn't we also raise an exception because it doesn't mean anything?

How about just warning?

[minwoox]

How about just warning?

That also sounds good.
@okue So we are going to need the class for nesting:

public final class ExportGroupConfiguration { // Named after SslConfiguration
    ...
    public void setPrefix(String prefix) {...}
    public void setExport(String mdcKey) {...}
    public void setExports(String mdcKeys) {...}
    ...
}

And add the setter:

public final class RequestContextExportingAppender {
    ...
    public void setExportGroup(ExportGroupConfiguration exportGroup) {...}
    ...
}

[okue]

Thank you very mush for surveying how to implement, @minwoox!

<exportGroup> looks good 😄

<exportGroup>
  <prefix>...</prefix>
  <export>...</export>
  <exports>...</exports>
</exportGroup>

BTW, Default export group (<prefix> which isn't surrounded by <exportGroup>) is needed?

[trustin]

I think we need it for backward compatibility. 😅

[okue]

Aa, let me rephrase that.
Is it needed to support <prefix> for default export group?
Is it bad if the prefix is always empty?

[trustin]

Is it needed to support <prefix> for default export group?

Yes, I guess, because a user may want to add a prefix to all exported properties? It would be somewhat inconvenient to use <exportGroup> just because of that.

[ikhoon]

Logback uses Javabeans introspection so I thought it's possible but I'm not sure because I never tried that. 😄
Let me try first and then I will tell you if we can do that or not.

Just out of courisity, at first I thought JoranConfigurator seemed like to work for getting attributes. Spring Boot extends it for <springProfile name="..." />. https://howtodoinjava.com/spring-boot2/logging/profile-specific-logging/

I've tried to make a prototype and I realized that it was overkill for the this issue.😅
Because the JoranConfigurator should be initialized before RequestContextExportingAppender that means we may need to make custom initializer...

[minwoox]

Yes, if the attributes are defined at the top level like Spring does,
Logback stores it here:
https://github.com/qos-ch/logback/blob/master/logback-core/src/main/java/ch/qos/logback/core/joran/spi/InterpretationContext.java#L46

But for the nested property I couldn't find how to get it easily, and we might need a hack or request the feature.

[ikhoon]

But for the nested property I couldn't find how to get it easily, and we might need a hack or request the feature.

JFYI. When I tested, nested properties were selected by "*/foobar".

[minwoox]

Oh, I never knew that. 😄
If we have more options, then shouldn't we reconsider the XML file?

[trustin]

Another thing to consider is that we might add support for other logging frameworks in the future, e.g. log4j2. We might want to keep the same set of XML tags if possible for consistent user experience.

[minwoox]

Had a chat with @trustin and @ikhoon, and we want to go with https://github.com/line/armeria/issues/3086#issuecomment-703116476
because it's simple and easy to implement. 😄

[okue]

Looks good 😄

[trustin]

Closed via #3112