Use the default settings for `Logging{Client,Service}`'s default fact… #2855

trustin · 2020-06-30T05:38:32Z

…ory methods

Motivation:

So far, Logging{Client,Service}.newDecorator() had different settings
than Logging{Client,Service}.builder().newDecorator() due to backward
compatibility. It's time to change this in favor of consistency.

Modifications:

Change the default logging level for successful requests and responses
from TRACE to DEBUG.
Change Logging{Client,Service}.newDecorator() to use its builder's
default settings.
- DEBUG level logging for successful requests and responses.
- WARN level logging for failed requests and responses`.

Result:

Consistency
Fixes Logging decorator factories should match semantics of empty builder #2696
(Breaking) The default logging level for successful requests and response
has been changed:
- INFO to DEBUG for Logging{Client,Service}.newDecorator()
- TRACE to DEBUG for Logging{Client,Service}Builder

…ory methods Motivation: So far, `Logging{Client,Service}.newDecorator()` had different settings than `Logging{Client,Service}.builder().newDecorator()` due to backward compatibility. It's time to change this in favor of consistency. Modifications: - Change `Logging{Client,Service}.newDecorator()` to use its builder's default settings. - `TRACE` level logging for successful requests and responses. - `WARN` level logging for failed requests and responses`. Result: - Consistency - Fixes line#2696

minwoox

Thanks!

ikhoon

Nice!

trustin · 2020-06-30T05:55:15Z

By the way, is TRACE level the right default for successful requests and responses? I was rather expecting DEBUG or INFO. Do you remember why we lowered to TRACE? /cc @imasahiro @kojilin @anuraaga

minwoox · 2020-06-30T06:17:52Z

I don't remember but DEBUG is fine with me.

ikhoon · 2020-06-30T06:36:06Z

Actually, I'm leaning towards INFO.😆 Because the request logs are generated from Logging{Client,Service} and users can easily turn it off using logging frameworks XML configuration.

ikhoon · 2020-06-30T06:43:53Z

Related work: The default log level of Spring's CommonsRequestLoggingFilter is DEBUG.
https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/CommonsRequestLoggingFilter.java#L39

trustin · 2020-06-30T09:04:57Z

Let me change the default level from TRACE to DEBUG tomorrow. Please feel free to let me know if you have any concern.

codecov · 2020-06-30T09:13:11Z

Codecov Report

Merging #2855 into master will decrease coverage by 0.10%.
The diff coverage is 65.78%.

@@             Coverage Diff              @@
##             master    #2855      +/-   ##
============================================
- Coverage     72.83%   72.72%   -0.11%     
- Complexity    12167    12168       +1     
============================================
  Files          1079     1080       +1     
  Lines         47283    47321      +38     
  Branches       5896     5896              
============================================
- Hits          34438    34415      -23     
- Misses         9817     9887      +70     
+ Partials       3028     3019       -9

Impacted Files	Coverage Δ	Complexity Δ
...eria/client/circuitbreaker/CircuitBreakerRule.java	`42.85% <16.66%> (-7.15%)`	`11.00 <1.00> (ø)`
...a/com/linecorp/armeria/client/retry/RetryRule.java	`66.66% <33.33%> (-3.04%)`	`17.00 <1.00> (+1.00)`	⬇️
.../circuitbreaker/CircuitBreakerRuleWithContent.java	`35.71% <50.00%> (-4.29%)`	`7.00 <2.00> (ø)`
...armeria/client/AbstractRuleWithContentBuilder.java	`53.84% <60.00%> (-9.80%)`	`7.00 <1.00> (ø)`
...m/linecorp/armeria/client/AbstractRuleBuilder.java	`74.54% <61.11%> (-12.13%)`	`23.00 <5.00> (+2.00)`	⬇️
...orp/armeria/client/retry/RetryRuleWithContent.java	`71.42% <66.66%> (-0.58%)`	`12.00 <1.00> (+1.00)`	⬇️
...tbreaker/CircuitBreakerRuleWithContentBuilder.java	`63.41% <71.42%> (-6.03%)`	`18.00 <6.00> (+1.00)`	⬇️
...eria/client/retry/RetryRuleWithContentBuilder.java	`75.00% <71.42%> (-7.06%)`	`24.00 <10.00> (+1.00)`	⬇️
...ient/circuitbreaker/CircuitBreakerRuleBuilder.java	`80.00% <75.00%> (-7.10%)`	`20.00 <3.00> (+1.00)`	⬇️
...linecorp/armeria/client/logging/LoggingClient.java	`100.00% <100.00%> (ø)`	`3.00 <1.00> (ø)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de25688...e30f442. Read the comment docs.

ikhoon

Still LGTM

line#2855) …ory methods Motivation: So far, `Logging{Client,Service}.newDecorator()` had different settings than `Logging{Client,Service}.builder().newDecorator()` due to backward compatibility. It's time to change this in favor of consistency. Modifications: - Change the default logging level for successful requests and responses from `TRACE` to `DEBUG`. - Change `Logging{Client,Service}.newDecorator()` to use its builder's default settings. - `DEBUG` level logging for successful requests and responses. - `WARN` level logging for failed requests and responses`. Result: - Consistency - Fixes line#2696 - (Breaking) The default logging level for successful requests and response has been changed: - `INFO` to `DEBUG` for `Logging{Client,Service}.newDecorator()` - `TRACE` to `DEBUG` for `Logging{Client,Service}Builder`

trustin added cleanup breaking change labels Jun 30, 2020

trustin added this to the 0.99.8 milestone Jun 30, 2020

trustin requested review from ikhoon and minwoox as code owners June 30, 2020 05:38

minwoox approved these changes Jun 30, 2020

View reviewed changes

ikhoon approved these changes Jun 30, 2020

View reviewed changes

trustin added 3 commits June 30, 2020 22:55

Merge branch 'master' into logging_default_behavior

114be14

TRACE -> DEBUG

91f893d

Fix

e30f442

ikhoon reviewed Jul 1, 2020

View reviewed changes

trustin merged commit 1e131c6 into line:master Jul 1, 2020

trustin deleted the logging_default_behavior branch July 1, 2020 06:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the default settings for `Logging{Client,Service}`'s default fact… #2855

Use the default settings for `Logging{Client,Service}`'s default fact… #2855

trustin commented Jun 30, 2020 •

edited

minwoox left a comment

ikhoon left a comment

trustin commented Jun 30, 2020 •

edited

minwoox commented Jun 30, 2020

ikhoon commented Jun 30, 2020 •

edited

ikhoon commented Jun 30, 2020

trustin commented Jun 30, 2020

codecov bot commented Jun 30, 2020 •

edited

ikhoon left a comment

Use the default settings for Logging{Client,Service}'s default fact… #2855

Use the default settings for Logging{Client,Service}'s default fact… #2855

Conversation

trustin commented Jun 30, 2020 • edited

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented Jun 30, 2020 • edited

minwoox commented Jun 30, 2020

ikhoon commented Jun 30, 2020 • edited

ikhoon commented Jun 30, 2020

trustin commented Jun 30, 2020

codecov bot commented Jun 30, 2020 • edited

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

Use the default settings for `Logging{Client,Service}`'s default fact… #2855

Use the default settings for `Logging{Client,Service}`'s default fact… #2855

trustin commented Jun 30, 2020 •

edited

trustin commented Jun 30, 2020 •

edited

ikhoon commented Jun 30, 2020 •

edited

codecov bot commented Jun 30, 2020 •

edited