Skip to content
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

Add responseCauseSanitizer to LoggingDecoratorBuilder #1594

Merged
merged 1 commit into from Feb 15, 2019

Conversation

trustin
Copy link
Collaborator

@trustin trustin commented Feb 14, 2019

Motivation:

A user wants to sanitize the stack trace of RequestLog.responseCause()
or avoid logging the stack trace completely.

Modifications:

  • Added LoggingDecoratorBuilder.responseCauseSanitizer()and applied the specifiedFunction` to the response cause.
  • Widened the type parameters of sanitizer functions.
  • Miscellaneous:
    • Fixed a bug where a response content sanitizer is applied on request
      content on a certain case.
    • Added contentSanitizer() and headersSanitizer() shortcut methods.
    • Javadoc clean-up

Result:

  • A user can sanitize/drop the stack trace of RequestLog.responseCause().
  • Request content is always sanitized with an intended sanitizer.
  • More shortcut methods in LoggingClientBuilder and LoggingServiceBuilder.

Motivation:

A user wants to sanitize the stack trace of `RequestLog.responseCause()`
or avoid logging the stack trace completely.

Modifications:

- Added LoggingDecoratorBuilder.responseCauseSanitizer()` and applied
  the specified `Function` to the response cause.
- Widened the type parameters of sanitizer functions.
- Miscellaneous:
  - Fixed a bug where a response content sanitizer is applied on request
    content on a certain case.
  - Added `contentSanitizer()` and `headersSanitizer()` shortcut methods.
  - Javadoc clean-up

Result:

- A user can sanitize/drop the stack trace of `RequestLog.responseCause()`.
  - Fixes line#1592
- Request content is always sanitized with an intended sanitizer.
- More shortcut methods in `LoggingClientBuilder` and `LoggingServiceBuilder`.
@trustin
Copy link
Collaborator Author

trustin commented Feb 14, 2019

@anuraaga I found we do not log RequestLog.requestCause(), so I did not add requestCauseSanitizer(). Do you think we need to add requestCauseSanitizer() and start to log a request cause? I'm curious why we did not so far. Any clues?

@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ff46158). Click here to learn what that means.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1594   +/-   ##
=========================================
  Coverage          ?   72.82%           
  Complexity        ?     7574           
=========================================
  Files             ?      699           
  Lines             ?    30459           
  Branches          ?     3712           
=========================================
  Hits              ?    22182           
  Misses            ?     6369           
  Partials          ?     1908
Impacted Files Coverage Δ Complexity Δ
...corp/armeria/common/logging/DefaultRequestLog.java 80.09% <ø> (ø) 144 <0> (?)
...om/linecorp/armeria/common/logging/RequestLog.java 25.71% <ø> (ø) 8 <0> (?)
...p/armeria/client/logging/LoggingClientBuilder.java 100% <100%> (ø) 3 <0> (?)
.../armeria/server/logging/LoggingServiceBuilder.java 100% <100%> (ø) 3 <0> (?)
...rp/armeria/internal/logging/LoggingDecorators.java 100% <100%> (ø) 8 <6> (?)
...inecorp/armeria/server/logging/LoggingService.java 65.62% <66.66%> (ø) 6 <0> (?)
...linecorp/armeria/client/logging/LoggingClient.java 62.5% <66.66%> (ø) 6 <0> (?)
...rmeria/common/logging/LoggingDecoratorBuilder.java 63.63% <73.33%> (ø) 21 <7> (?)

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 ff46158...ee21f27. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 14, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ff46158). Click here to learn what that means.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1594   +/-   ##
=========================================
  Coverage          ?   72.82%           
  Complexity        ?     7574           
=========================================
  Files             ?      699           
  Lines             ?    30459           
  Branches          ?     3712           
=========================================
  Hits              ?    22182           
  Misses            ?     6369           
  Partials          ?     1908
Impacted Files Coverage Δ Complexity Δ
...corp/armeria/common/logging/DefaultRequestLog.java 80.09% <ø> (ø) 144 <0> (?)
...om/linecorp/armeria/common/logging/RequestLog.java 25.71% <ø> (ø) 8 <0> (?)
...p/armeria/client/logging/LoggingClientBuilder.java 100% <100%> (ø) 3 <0> (?)
.../armeria/server/logging/LoggingServiceBuilder.java 100% <100%> (ø) 3 <0> (?)
...rp/armeria/internal/logging/LoggingDecorators.java 100% <100%> (ø) 8 <6> (?)
...inecorp/armeria/server/logging/LoggingService.java 65.62% <66.66%> (ø) 6 <0> (?)
...linecorp/armeria/client/logging/LoggingClient.java 62.5% <66.66%> (ø) 6 <0> (?)
...rmeria/common/logging/LoggingDecoratorBuilder.java 63.63% <73.33%> (ø) 21 <7> (?)

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 ff46158...ee21f27. Read the comment docs.

@anuraaga
Copy link
Collaborator

Hmm - I don't know if I've ever thought of the existence of request cause. Any request exception would probably be logged in the response itself so it seems likely to double-log if the request cause was logged.

@trustin
Copy link
Collaborator Author

trustin commented Feb 15, 2019

Yeah. That's probably why. Let me keep it this way then. Thanks for a reply.

@trustin trustin removed the request for review from hyangtack February 15, 2019 03:19
@trustin trustin merged commit b95b992 into line:master Feb 15, 2019
@trustin trustin deleted the cause_sanitizer branch February 15, 2019 03:19
@trustin
Copy link
Collaborator Author

trustin commented Feb 15, 2019

Thanks for reviewing :-)

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

A user wants to sanitize the stack trace of `RequestLog.responseCause()`
or avoid logging the stack trace completely.

Modifications:

- Added LoggingDecoratorBuilder.responseCauseSanitizer()` and applied
  the specified `Function` to the response cause.
- Widened the type parameters of sanitizer functions.
- Miscellaneous:
  - Fixed a bug where a response content sanitizer is applied on request
    content on a certain case.
  - Added `contentSanitizer()` and `headersSanitizer()` shortcut methods.
  - Javadoc clean-up

Result:

- A user can sanitize/drop the stack trace of `RequestLog.responseCause()`.
- Request content is always sanitized with an intended sanitizer.
- More shortcut methods in `LoggingClientBuilder` and `LoggingServiceBuilder`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants