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.

Sign up for GitHub

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

Jump to bottom

Enable request buffering when request body tracing #3440

Open
wants to merge 1 commit into
base: 2.0.x
Choose a base branch
from
Open

Enable request buffering when request body tracing #3440

wants to merge 1 commit into from

Conversation

candrews
Copy link
Contributor

Request body tracing reads the body then it will be read again later for actual handling.
Therefore, request buffering is required when request body tracing.

Fixes #3418

@candrews
Enable request buffering when request body tracing
d98a0e5 
Request body tracing reads the body then it will be read again later for actual handling.
Therefore, request buffering is required when request body tracing.

Fixes spring-cloud#3418
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #3440 into 2.0.x will decrease coverage by 0.51%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.0.x    #3440      +/-   ##
============================================
- Coverage     66.65%   66.14%   -0.52%     
+ Complexity     1484     1482       -2     
============================================
  Files           183      183              
  Lines          6922     6924       +2     
  Branches        845      846       +1     
============================================
- Hits           4614     4580      -34     
- Misses         1994     2028      +34     
- Partials        314      316       +2
Impacted Files Coverage Δ Complexity Δ
...tflix/zuul/filters/pre/Servlet30WrapperFilter.java 75% <50%> (-2.78%) 6 <0> (ø)
...cloud/netflix/eureka/EurekaInstanceConfigBean.java 52.28% <0%> (-16.76%) 53% <0%> (-1%)
...etflix/turbine/stream/HystrixStreamAggregator.java 73.68% <0%> (-5.27%) 8% <0%> (-1%)

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 21e7cca...d98a0e5. Read the comment docs.

3 similar comments
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #3440 into 2.0.x will decrease coverage by 0.51%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.0.x    #3440      +/-   ##
============================================
- Coverage     66.65%   66.14%   -0.52%     
+ Complexity     1484     1482       -2     
============================================
  Files           183      183              
  Lines          6922     6924       +2     
  Branches        845      846       +1     
============================================
- Hits           4614     4580      -34     
- Misses         1994     2028      +34     
- Partials        314      316       +2
Impacted Files Coverage Δ Complexity Δ
...tflix/zuul/filters/pre/Servlet30WrapperFilter.java 75% <50%> (-2.78%) 6 <0> (ø)
...cloud/netflix/eureka/EurekaInstanceConfigBean.java 52.28% <0%> (-16.76%) 53% <0%> (-1%)
...etflix/turbine/stream/HystrixStreamAggregator.java 73.68% <0%> (-5.27%) 8% <0%> (-1%)

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 21e7cca...d98a0e5. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #3440 into 2.0.x will decrease coverage by 0.51%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.0.x    #3440      +/-   ##
============================================
- Coverage     66.65%   66.14%   -0.52%     
+ Complexity     1484     1482       -2     
============================================
  Files           183      183              
  Lines          6922     6924       +2     
  Branches        845      846       +1     
============================================
- Hits           4614     4580      -34     
- Misses         1994     2028      +34     
- Partials        314      316       +2
Impacted Files Coverage Δ Complexity Δ
...tflix/zuul/filters/pre/Servlet30WrapperFilter.java 75% <50%> (-2.78%) 6 <0> (ø)
...cloud/netflix/eureka/EurekaInstanceConfigBean.java 52.28% <0%> (-16.76%) 53% <0%> (-1%)
...etflix/turbine/stream/HystrixStreamAggregator.java 73.68% <0%> (-5.27%) 8% <0%> (-1%)

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 21e7cca...d98a0e5. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 19, 2019 •
edited
Loading

Codecov Report

Merging #3440 into 2.0.x will decrease coverage by 0.51%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##              2.0.x    #3440      +/-   ##
============================================
- Coverage     66.65%   66.14%   -0.52%     
+ Complexity     1484     1482       -2     
============================================
  Files           183      183              
  Lines          6922     6924       +2     
  Branches        845      846       +1     
============================================
- Hits           4614     4580      -34     
- Misses         1994     2028      +34     
- Partials        314      316       +2
Impacted Files Coverage Δ Complexity Δ
...tflix/zuul/filters/pre/Servlet30WrapperFilter.java 75% <50%> (-2.78%) 6 <0> (ø)
...cloud/netflix/eureka/EurekaInstanceConfigBean.java 52.28% <0%> (-16.76%) 53% <0%> (-1%)
...etflix/turbine/stream/HystrixStreamAggregator.java 73.68% <0%> (-5.27%) 8% <0%> (-1%)

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 21e7cca...d98a0e5. Read the comment docs.

spencergibb
spencergibb requested changes Mar 19, 2019
View reviewed changes
...src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/Servlet30WrapperFilter.java
@@ -80,6 +84,10 @@ else if (RequestUtils.isDispatcherServletRequest()) {
// If it's going through the dispatcher we need to buffer the body
ctx.setRequest(new Servlet30RequestWrapper(request));
}
else if (zuulProperties.isTraceRequestBody()) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is exactly what I described.

turn off trace request body if zuul.use-filter=true.

This enables buffering if isTraceRequestBody() which isn't the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turn off trace request body if zuul.use-filter=true.

What you suggest seems like it'll be really awkward to program and unexpected for users:
if zuul.use-filter=true, ignore zuul.traceRequestBody and treat it as if it were always false. This implementation would confuse users who expect zuul.traceRequestBody to do something, and it's weird for traceRequestBody to be a variable that's sometimes ignored. IMHO, making zuul.traceRequestBody always work as expected is a better solution for maintainability and users alike, which is the approach I took in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

It also unexpectedly introduces buffering because trace request body is true by default. I won't apply this PR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you provide any guidance on what I can do?
Things I can think of include making some other change that you suggest or rebasing against another branch.

Copy link
Member

Choose a reason for hiding this comment

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

Add another case here

spring-cloud-netflix/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/filters/ProxyRequestHelper.java

Line 266 in eff47e0

if (!this.traceRequestBody || ctx.isChunkedRequestBody()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for my density... what case are suggesting be added?

Copy link
Member

Choose a reason for hiding this comment

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

if zuul.use-filter=true: return false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spencergibb spencergibb spencergibb requested changes

Assignees
No one assigned
Labels
waiting for feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@candrews @spencergibb @spring-projects-issues