Add Logback integration which exports context properties to MDC #305

trustin · 2016-10-14T08:33:08Z

Add RequestContextExportingAppender
Deprecate ServiceRequestContext.logger() and its related configuration
properties

trustin · 2016-10-14T08:34:16Z

See testXmlConfig.xml for an example configuration.
See BuiltInProperty for the complete list of built-in properties
You can export any attributes and http headers as well (see testXmlConfig.xml again.)

trustin · 2016-10-14T09:34:43Z

imasahiro · 2016-10-14T10:24:01Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+                    authority = authority.substring(0, m.start());
+                }
+                REQ_AUTHORITY.set(authority);
+                return;


Remove this

oops, never mind.

imasahiro · 2016-10-14T10:24:30Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+            }
+        }
+
+        String authority;


Add final

Thanks. Fixed. :-)

imasahiro · 2016-10-17T01:12:37Z

core/src/main/java/com/linecorp/armeria/common/NonWrappingRequestContext.java

+     */
+    protected abstract Channel channel();
+
+    @Override


Do we need to put @Nullable annotation here?

imasahiro · 2016-10-17T01:12:44Z

core/src/main/java/com/linecorp/armeria/common/NonWrappingRequestContext.java

+        return ch != null ? (A) ch.remoteAddress() : null;
+    }
+
+    @Override


imasahiro · 2016-10-17T01:24:46Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+            return;
+        }
+
+        if (mdcKey.startsWith("req.http_headers.")) {


How about define string constant and use it?
mdcKey.substring(17) is hard to understand for me.

Thanks for suggestion. Fixed.

anuraaga · 2016-10-17T05:23:19Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+
+    public Map<AttributeKey<?>, String> getAttributes() {
+        return Collections.unmodifiableMap(attrNames.entrySet().stream().collect(
+                Collectors.toMap(Entry::getKey, e -> e.getValue().substring(6))));


PREFIX_ATTRS.length() I think

Thanks. Missed that one! :-)

anuraaga · 2016-10-17T06:26:20Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+import io.netty.util.AsciiString;
+import io.netty.util.AttributeKey;
+
+public class RequestContextExportingAppender extends UnsynchronizedAppenderBase<ILoggingEvent>


Since this is Unsynchronized, do we need to make this class threadsafe (e.g. ConcurrentHashMap, computeIfAbsent)?

Will make the export list immutable once start() is called.

Done. Thanks for an advice.

anuraaga · 2016-10-17T06:27:57Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+            if (enabledBuiltIns.contains(SSL_SESSION_ID)) {
+                final byte[] id = s.getId();
+                if (id != null) {
+                    SSL_SESSION_ID.set(BaseEncoding.base16().lowerCase().encode(id));


Can store BaseEncoding.base16().lowerCase() to a static final

codecov-io · 2016-10-17T07:33:49Z

Current coverage is 64.35% (diff: 70.48%)

Merging #305 into master will increase coverage by 0.35%

@@             master       #305   diff @@
==========================================
  Files           310        316     +6   
  Lines         12236      12727   +491   
  Methods           0          0          
  Messages          0          0          
  Branches       1790       1906   +116   
==========================================
+ Hits           7830       8190   +360   
- Misses         3584       3661    +77   
- Partials        822        876    +54

Powered by Codecov. Last update 1e6b034...94c0464

anuraaga · 2016-10-17T08:15:03Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+    private RequestContextExporter exporter;
+
+    private final AppenderAttachableImpl<ILoggingEvent> aai = new AppenderAttachableImpl<>();
+    private final Set<BuiltInProperty> builtIns = EnumSet.noneOf(BuiltInProperty.class);


How about making these part of a RequestContextExporterBuilder which returns RequestContextExporter after calling build()? It's a bit weird having this now-"temporary" state on the appender.

We cannot fix the mutability of appender because XML configuration requires it to be so - it has to call setExport() on each <export/> element. Please let me know if there's a way to build an immutable appender using Joran.

I agree the class won't be completely immutable, but at least wrapping these into a separate, Builder type of object, makes it clearer they're only used during setup.

OK. Extracted intermediary data structure to RequestContextExporterBuilder. Looking better?

trustin · 2016-10-18T16:14:08Z

Found a bug where the exported MDC properties are not included in the Map returned by LoggingEvent.getMDCPropertyMap() when RequestContextExportingAppender is not the first appender in the appender list of a Logger. Will fix with non-trivial changes.

trustin · 2016-10-19T03:11:14Z

Bug fixed at: 3f918fd

taicki

Looks good. A couple of minor comments.

taicki · 2016-10-20T02:02:28Z

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExportingAppender.java

+        return state;
+    }
+
+    private static int flag(RequestLog req, ResponseLog res) {


nit: defining an enum and adding this method would be a bit more readable.

Now using enum :-)

taicki · 2016-10-20T02:49:19Z

site/src/sphinx/advanced-logging.rst

+        <export>remote.port</export>
+        <export>tls.cipher</export>
+        <export>req.http_headers.user-agent</export>
+        <export>attrs.req_id:com.example.RequestId#ATTR</export>


Please add an example in the above pattern for attrs' case.

Updated the documentation.

- Add RequestContextExportingAppender - Deprecate ServiceRequestContext.logger() and its related configuration properties

taicki

LGTM

trustin added new feature do-not-merge labels Oct 14, 2016

trustin added this to the 0.29.0.Final milestone Oct 14, 2016

trustin assigned taicki Oct 14, 2016

trustin assigned imasahiro Oct 14, 2016

imasahiro reviewed Oct 14, 2016

View reviewed changes

trustin force-pushed the logback branch from 5f216ea to 5469c9e Compare October 15, 2016 02:13

imasahiro reviewed Oct 17, 2016

View reviewed changes

anuraaga reviewed Oct 17, 2016

View reviewed changes

imasahiro approved these changes Oct 17, 2016

View reviewed changes

trustin force-pushed the logback branch from dd26efe to 4f93092 Compare October 18, 2016 06:05

anuraaga approved these changes Oct 18, 2016

View reviewed changes

taicki reviewed Oct 20, 2016

View reviewed changes

trustin force-pushed the logback branch 2 times, most recently from f9250fa to 085b9e7 Compare October 20, 2016 08:24

Add Logback integration which exports context properties to MDC

94c0464

- Add RequestContextExportingAppender - Deprecate ServiceRequestContext.logger() and its related configuration properties

trustin force-pushed the logback branch from 085b9e7 to 94c0464 Compare October 20, 2016 08:26

trustin removed the do-not-merge label Oct 20, 2016

taicki approved these changes Oct 21, 2016

View reviewed changes

taicki merged commit 7e988af into line:master Oct 21, 2016

trustin deleted the logback branch October 21, 2016 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Logback integration which exports context properties to MDC #305

Add Logback integration which exports context properties to MDC #305

trustin commented Oct 14, 2016

trustin commented Oct 14, 2016

trustin commented Oct 14, 2016

imasahiro Oct 14, 2016

imasahiro Oct 14, 2016 •

edited by trustin

Loading

imasahiro Oct 14, 2016

trustin Oct 15, 2016

imasahiro Oct 17, 2016

trustin Oct 17, 2016

imasahiro Oct 17, 2016

trustin Oct 17, 2016

imasahiro Oct 17, 2016

trustin Oct 17, 2016

anuraaga Oct 17, 2016

trustin Oct 17, 2016

anuraaga Oct 17, 2016

trustin Oct 17, 2016

trustin Oct 17, 2016

anuraaga Oct 17, 2016

trustin Oct 17, 2016

codecov-io commented Oct 17, 2016 •

edited

Loading

anuraaga Oct 17, 2016

trustin Oct 17, 2016

anuraaga Oct 17, 2016

trustin Oct 18, 2016

trustin commented Oct 18, 2016

trustin commented Oct 19, 2016

taicki left a comment

taicki Oct 20, 2016

trustin Oct 20, 2016

taicki Oct 20, 2016

trustin Oct 20, 2016

taicki left a comment

Add Logback integration which exports context properties to MDC #305

Add Logback integration which exports context properties to MDC #305

Conversation

trustin commented Oct 14, 2016

trustin commented Oct 14, 2016

trustin commented Oct 14, 2016

Choose a reason for hiding this comment

imasahiro Oct 14, 2016 • edited by trustin Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 17, 2016 • edited Loading

Current coverage is 64.35% (diff: 70.48%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented Oct 18, 2016

trustin commented Oct 19, 2016

taicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taicki left a comment

Choose a reason for hiding this comment

imasahiro Oct 14, 2016 •

edited by trustin

Loading

codecov-io commented Oct 17, 2016 •

edited

Loading