[JAV-589] provide vertx access log handler component #441

yhs0092 · 2017-12-15T10:27:31Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [JAV-XXX] Fixes bug in ApproximateQuantiles, where you replace JAV-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

see details in https://servicecomb.atlassian.net/browse/JAV-589.
this PR provides a vertx access log printer.

coveralls · 2017-12-16T05:09:16Z

Coverage increased (+0.2%) to 86.957% when pulling 52e20e5 on yhs0092:JAV-561_provide_vertx_access_log_component into 9f50bc0 on ServiceComb:master.

WillemJiang · 2017-12-18T03:26:04Z

...ertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/AccessLogConfiguration.java

@@ -0,0 +1,38 @@
+package io.servicecomb.transport.rest.vertx.accesslog;


Please add the license header.

License header has been added.

Please use the new add Apache license, you can get it from the master code.

WillemJiang · 2017-12-18T03:26:32Z

...ertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/AccessLogConfiguration.java

+import io.servicecomb.core.BootListener.BootEvent;
+
+public final class AccessLogConfiguration {
+  private static final String BASE = "cse.accesslog.";


This should be configurable.

This is the name prefix of configuration item. It has been renamed to "servicecomb.accesslog."

WillemJiang · 2017-12-18T03:27:04Z

...rest-vertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/AccessLogHandler.java

@@ -0,0 +1,8 @@
+package io.servicecomb.transport.rest.vertx.accesslog;


license header.

License header has been added.

WillemJiang · 2017-12-18T03:30:05Z

...t/transport-rest-vertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/README.md

+
+To enable access log printing, you can config access log in microservice.yaml like below:
+```yaml
+cse:


it's better to use servicecomb prefix instead of cse.

The prefix "cse" has been replaced with "servicecomb".

WillemJiang · 2017-12-18T03:31:00Z

...tx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/element/AccessLogElement.java

@@ -0,0 +1,16 @@
+package io.servicecomb.transport.rest.vertx.accesslog.element;


License header.

License header has been added.

WillemJiang · 2017-12-18T03:32:30Z

...n/java/io/servicecomb/transport/rest/vertx/accesslog/element/impl/BytesWrittenV2Element.java

+import io.servicecomb.transport.rest.vertx.accesslog.element.AccessLogElement;
+import io.vertx.core.http.HttpServerResponse;
+
+public class BytesWrittenV2Element implements AccessLogElement {


why do we have two versions of bytes written element?

They will return different result when bytes written is zero. v1 is "0" and v2 is "-".

WillemJiang · 2017-12-18T03:33:58Z

.../src/main/java/io/servicecomb/transport/rest/vertx/accesslog/element/impl/CookieElement.java

+
+import java.util.Set;
+
+import com.sun.org.apache.regexp.internal.RE;


Please don't use the internal API which may not be compatible with other JDK.

This is added by mistake. Has been removed.

WillemJiang · 2017-12-18T03:34:48Z

.../io/servicecomb/transport/rest/vertx/accesslog/element/impl/DatetimeConfigurableElement.java

@@ -0,0 +1,97 @@
+package io.servicecomb.transport.rest.vertx.accesslog.element.impl;


License header.

License header has been added.

WillemJiang · 2017-12-18T03:42:13Z

transports/transport-rest/transport-rest-vertx/src/main/resources/config/base/log4j.properties

@@ -0,0 +1,13 @@
+# access log default configuration


The log configure should be provide from the Application level

User can override the access log configuration in their log4j.properties. Or totally replace Log4j with other log printer framework, since this AccessLogHandlerImpl just depends on slf4j.

coveralls · 2017-12-18T11:40:06Z

Coverage increased (+0.2%) to 86.956% when pulling c410a1e on yhs0092:JAV-561_provide_vertx_access_log_component into 9f50bc0 on ServiceComb:master.

yhs0092 · 2017-12-18T11:42:20Z

@WillemJiang New commit has passed ci, please review it again.

coveralls · 2017-12-19T05:17:28Z

Coverage increased (+0.2%) to 86.971% when pulling 98071dc on yhs0092:JAV-561_provide_vertx_access_log_component into 9f50bc0 on ServiceComb:master.

coveralls · 2017-12-19T12:42:48Z

Coverage increased (+0.3%) to 87.07% when pulling 394a199 on yhs0092:JAV-561_provide_vertx_access_log_component into 9f50bc0 on ServiceComb:master.

wujimin · 2017-12-21T07:45:10Z

...ertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/AccessLogConfiguration.java

+
+  private String getProperty(String defaultValue, String key) {
+    String property = DynamicPropertyFactory.getInstance().getStringProperty(key, defaultValue).get();
+    if (null == property) {


how can we got null in this place?

this null check has been removed.

wujimin · 2017-12-21T07:45:41Z

...ertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/AccessLogConfiguration.java

+
+  public boolean getAccessLogEnabled() {
+    String enabled = getProperty("false", ACCESSLOG_ENABLED);
+    return Boolean.parseBoolean(enabled);


while not just DynamicPropertyFactory.getInstance().getBooleanProperty?

This has been changed to use DynamicPropertyFactory.getInstance().getBooleanProperty()

wujimin · 2017-12-21T07:50:13Z

...rest-vertx/src/main/java/io/servicecomb/transport/rest/vertx/accesslog/AccessLogHandler.java

+import io.vertx.core.Handler;
+import io.vertx.ext.web.RoutingContext;
+
+public interface AccessLogHandler extends Handler<RoutingContext> {


what's the purpose of this interface?

This interface has been removed. Now, it's implementation AccessLogHandlerImpl implements the interface Handler directly.

The class AccessLogHandlerImpl is renamed to AccessLogHandler since this interface no longer exists.

wujimin · 2017-12-21T07:52:51Z

...n/java/io/servicecomb/transport/rest/vertx/accesslog/element/impl/BytesWrittenV2Element.java

+    long bytesWritten = response.bytesWritten();
+    if (0 == bytesWritten) {
+      return ZERO_BYTES;
+    } else {


already return, no need else

If bytesWritten equals to 0, "-" will be returned instead of "0". So there should be this check logic.

check is no problem
"else" is unnecessary

wujimin · 2017-12-21T07:53:46Z

.../src/main/java/io/servicecomb/transport/rest/vertx/accesslog/element/impl/CookieElement.java

+  @Override
+  public String getFormattedElement(AccessLogParam accessLogParam) {
+    RoutingContext context = accessLogParam.getRoutingContext();
+    if (null == context) {


how can we got a null context?

This null check has been removed.

wujimin · 2017-12-21T08:35:24Z

...ansport-rest-vertx/src/main/java/io/servicecomb/transport/rest/vertx/RestServerVerticle.java

 import io.vertx.core.AbstractVerticle;
 import io.vertx.core.Context;
 import io.vertx.core.Future;
 import io.vertx.core.Vertx;
 import io.vertx.core.http.HttpServer;
 import io.vertx.core.http.HttpServerOptions;
 import io.vertx.ext.web.Router;
+import io.vertx.ext.web.handler.LoggerFormat;
+import io.vertx.ext.web.handler.impl.LoggerHandlerImpl;


import but not used
so you did not resolve compile warnings?

These unused imports have been removed, I will take care in the next time.

wujimin · 2017-12-21T08:58:04Z

where is traceId?
where is accessLog for highway?

do we have issues to trace this?

yhs0092 · 2017-12-21T11:55:44Z

@wujimin The issue JAV-561 is still in progress. Other features you mentioned are still in developing. I create this PR to commit the first part of this issue(access log of vertx) because it's too large to commit all content in one PR.

wujimin · 2017-12-22T00:18:33Z

ok
561 is a story
i think it's better to create subTasks in it. this PR is the first subTask

we need these subTasks at least:
1.highway support accesslog
2.generate traceId not depend on trace/access mechanism
3.support log traceId in accesslog

coveralls · 2017-12-22T03:56:30Z

Changes Unknown when pulling d7ae8da on yhs0092:JAV-561_provide_vertx_access_log_component into ** on apache:master**.

… setting variety.

…to unit test file.

coveralls · 2017-12-22T07:02:55Z

Coverage increased (+0.3%) to 87.201% when pulling 3546b3b on yhs0092:JAV-561_provide_vertx_access_log_component into bdc2fd3 on apache:master.

coveralls · 2017-12-22T10:23:18Z

Coverage increased (+0.3%) to 87.201% when pulling cc98df1 on yhs0092:JAV-561_provide_vertx_access_log_component into bdc2fd3 on apache:master.

WillemJiang reviewed Dec 18, 2017

View reviewed changes

yhs0092 closed this Dec 19, 2017

yhs0092 reopened this Dec 19, 2017

wujimin reviewed Dec 21, 2017

View reviewed changes

WillemJiang closed this Dec 22, 2017

WillemJiang reopened this Dec 22, 2017

yhs0092 added 7 commits December 22, 2017 14:07

[JAV-589] provide access log handler component

de4df03

[JAV-589] fix unit test failure caused by default timezone and locale…

1c0d186

… setting variety.

[JAV-589] fix unit test failure, add README.md

e8e134f

[JAV-589] add license header, organize import.

9d5ba47

[JAV-589] modify some configurations and comments.

dc2f0c4

[JAV-589] modify README.

1479a74

[JAV-589] update License header.

903b5ae

[JAV-589] modify code according to code review, add license header in…

3546b3b

…to unit test file.

yhs0092 changed the title ~~[JAV-561] provide access log handler component~~ [JAV-589] provide access log handler component Dec 22, 2017

yhs0092 changed the title ~~[JAV-589] provide access log handler component~~ [JAV-589] provide vertx access log handler component Dec 22, 2017

yhs0092 force-pushed the JAV-561_provide_vertx_access_log_component branch from d7ae8da to 3546b3b Compare December 22, 2017 06:44

wujimin approved these changes Dec 22, 2017

View reviewed changes

[JAV-589] remove redundant else expression

cc98df1

yhs0092 closed this Dec 22, 2017

yhs0092 reopened this Dec 22, 2017

liubao68 approved these changes Dec 22, 2017

View reviewed changes

WillemJiang approved these changes Dec 22, 2017

View reviewed changes

WillemJiang merged commit e4611cd into apache:master Dec 22, 2017

yhs0092 deleted the JAV-561_provide_vertx_access_log_component branch July 23, 2018 12:17

		@@ -0,0 +1,38 @@
		package io.servicecomb.transport.rest.vertx.accesslog;

		@@ -0,0 +1,8 @@
		package io.servicecomb.transport.rest.vertx.accesslog;

		@@ -0,0 +1,16 @@
		package io.servicecomb.transport.rest.vertx.accesslog.element;


		import java.util.Set;

		import com.sun.org.apache.regexp.internal.RE;

		@@ -0,0 +1,97 @@
		package io.servicecomb.transport.rest.vertx.accesslog.element.impl;

[JAV-589] provide vertx access log handler component #441

[JAV-589] provide vertx access log handler component #441

Conversation

yhs0092 commented Dec 15, 2017 • edited by WillemJiang Loading

coveralls commented Dec 16, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2017

yhs0092 commented Dec 18, 2017

coveralls commented Dec 19, 2017

coveralls commented Dec 19, 2017

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

wujimin commented Dec 21, 2017

yhs0092 commented Dec 21, 2017

wujimin commented Dec 22, 2017

coveralls commented Dec 22, 2017

coveralls commented Dec 22, 2017

coveralls commented Dec 22, 2017

yhs0092 commented Dec 15, 2017 •

edited by WillemJiang

Loading