Skip to content

Internal logging support #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 27, 2019
Merged

Internal logging support #205

merged 5 commits into from
Sep 27, 2019

Conversation

nicktorwald
Copy link

Add a logging abstraction over foreign logger backends. The internal
logger supports JUL and SLF4J (Logback) loggers in this commit.
Parametrized messages are supported via MessageFormat instead of
relying on external loggers support.

Closes: #194

@coveralls
Copy link

coveralls commented Jul 11, 2019

Coverage Status

Coverage decreased (-1.3%) to 75.58% when pulling 946ed15 on nicktorwald/gh-194-logging into 2b32e80 on master.

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is I understood right that slf4j is the optional dependency?

Are logback users will be obligated to use slf4j?

Are we want to skip logging of reconnects and retried requests for now? Is this the reason to postpone implementing of throttling / filtering of logs? Please, comment it in the issue if so.

I was against of runtime guessing to choose a logger backend. Please, clarify your points if you are disagree (in the issue).

I would rename the 2nd commit: it is not an exampe, it is the implementation of logging of warnings from cluster discovery.

I would add a short example to README: how to attach another logger implementation and set it for the connector.

I would think, whether it worth to test that we emit right warnings. I don't see much value in that for now, to be honest.

@nicktorwald
Copy link
Author

Is I understood right that slf4j is the optional dependency?

Yes, a packaged connector JAR file is dependency-free from slf4j.
Actually, we use slf4j-api module which contains only facade interfaces. This is a provided dependency that means it's used for compiling stage only (we have to compile Slf4jLoggerProvider) and it's skipped on packaging stage.

Are logback users will be obligated to use slf4j?

Looks that always, logback implements slf4j API (i.e. Logger class) This leads logback can be used as-is with slf4j. For other logging libraries, it's required to use appropriate bridges like slf4j-to-XXX and vice-versa.

Are we want to skip logging of reconnects and retried requests for now?

This can be done in scope of this PR but separate commit.

Is this the reason to postpone implementing of throttling / filtering of logs?

This is usually out of our responsibility and user's environment configures those options.
Assume we use logback as a logging backend so we can configure where, how and what we want
to log. For instance, a user's app can contain the following config file:

<configuration>
  <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
    <encoder>
      <pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern>
    </encoder>
  </appender>

  <logger name="org.tarantool.jdbc" level="INFO" />
  <logger name="org.tarantool.cluster" level="DEBUG" />
  <logger name="org.tarantool.cluster.TarantoolClientImpl" level="TRACE" />

  <root level="debug">
    <appender-ref ref="STDOUT" />
  </root>
</configuration>

Here, we chose stdout as a target destination, we picked DEBUG messages level to be logged as a default level and defined the format of a message. We also changed the default level for org.tarantool.jdbc and org.tarantool.cluster packages as well as org.tarantool.cluster.TarantoolClientImpl class.

About throttling, I think this is also should be configured by a user using logger capability. For example, logback provides an out of the box solution DuplicateMessageFilter

<turboFilter class="ch.qos.logback.classic.turbo.DuplicateMessageFilter"/>

As a result, we are only responsible for adding logger write methods to the right places and with the appropriate level. For client requests, it's usually used the lowest TRACE level because of verbosity and it's enabled if this sort of logging is required. If the user chooses TRACE level, he should be ready to receive a lot of messages.

I was against of runtime guessing to choose a logger backend. Please, clarify your points if you are disagree (in the issue).

I based this PR on typical practices used in other java libraries where there're a couple of approaches used in a chain: JVM argument like -Dproperty=value; SPI support (if user provide it's own implementation and registers it as a service provider then it will be used); finally we exploring the CLASSPATH and try to find first appropriate logger backend; if all methods fail it uses the default logging system - VoidLogger of JUL. Of course, we can skip classpath scanning if the logger isn't explicitly configured and pick JUL in the case.

I would rename the 2nd commit: it is not an example, it is the implementation of logging of warnings from cluster discovery.

Ok.

I would add a short example to README: how to attach another logger implementation and set it for the connector.

Ok.

I would think, whether it worth to test that we emit right warnings. I don't see much value in that for now, to be honest.

Hm... Haven't seen that anyone tests this kinda functionality. Logging seems to be not so important for testing and it obviously reflects the current state which we test.

@Totktonada
Copy link
Member

Are we want to skip logging of reconnects and retried requests for now?

This can be done in scope of this PR but separate commit.

Let's log at least reconnects: we need it to understand a cause of, say, such problems. Don't so sure about logging of retried requests: if it is easy, maybe it worth to add too (and log errors we got at failed attempts). In general I think that logging of all transient errors we got (and overcomed) is good.

About throttling, I think this is also should be configured by a user using logger capability.

It throttles the same messages: it does not work for a similar (but not same) messages. But it is okay for now I think.

I was against of runtime guessing to choose a logger backend. Please, clarify your points if you are disagree (in the issue).

I based this PR on typical practices used in other java libraries <...>

If it is the behaviour that is expected by a user based on expirience with other libraries, so let's behave in this way: properties, SPI, CLASSPATH, JUL. It should not lead to a confusion so.

@Totktonada Totktonada mentioned this pull request Sep 5, 2019
@nicktorwald nicktorwald force-pushed the nicktorwald/gh-194-logging branch 3 times, most recently from d7131fe to 73fc1f6 Compare September 9, 2019 15:36
@nicktorwald nicktorwald force-pushed the nicktorwald/gh-194-logging branch 3 times, most recently from a095ee2 to f2e638c Compare September 17, 2019 12:03
@Totktonada
Copy link
Member

I would move BaseLogger changes to the first commit.

Nit: Trailing whitespaces in README.

@nicktorwald nicktorwald force-pushed the nicktorwald/gh-194-logging branch from f2e638c to 91bf365 Compare September 24, 2019 17:38
@nicktorwald
Copy link
Author

I would move BaseLogger changes to the first commit.

Nit: Trailing whitespaces in README.

done

@nicktorwald nicktorwald force-pushed the nicktorwald/gh-194-logging branch from 91bf365 to 946ed15 Compare September 26, 2019 06:56
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Add a logging abstraction over foreign logger backends. The internal
logger supports JUL and SLF4J (Logback) loggers in this commit.
Parametrized messages are supported via MessageFormat instead of
relying on external loggers support.

Closes: #194

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
Add a warning message if some of the obtained cluster addresses are
invalid and skipped from the processing.

Follows on: #194
Add logging waring messages when client tries establishing connection
to Tarantool instance. All the request that cannot be processed because
of some transient errors are also traced.

Disable JUL logging for test classes by default.

Follows on: #194
Add basic info about supported logging system in the connector.

Follows on: #194
Update maven wrapper as well as target maven version to
3.6.2.
@nicktorwald nicktorwald force-pushed the nicktorwald/gh-194-logging branch from 946ed15 to 56ee9ea Compare September 27, 2019 12:25
@nicktorwald nicktorwald merged commit 747c2af into master Sep 27, 2019
@nicktorwald nicktorwald deleted the nicktorwald/gh-194-logging branch September 27, 2019 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support logging
3 participants