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

Fix where access log is not written when the method of a reques… #2159

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Fix where access log is not written when the method of a reques… #2159

merged 2 commits into from
Oct 8, 2019

Conversation

minwoox
Copy link
Contributor

@minwoox minwoox commented Oct 6, 2019 •
edited by trustin
Loading

…t is not allowed

Motivation:
There's some cases that we don't call RequestLogBuilder.endRequest().

  • When the method of the request is not allowed
  • When there's no services that match the path of the request
  • and so on

Because the RequestLog in never complete, AccessLogWriter is not called so we should fix it.

Modifications:

  • Call RequestLogBuilder.endRequest() even when service.serve() is not invoked
  • Do not log HttpStatusException as requestCause

Result:

  • You can now see the access logs when the method of a request is not allowed and there are no services that match the path.

@minwoox
Fix where access log is not written when the method of a request is n…
ff22441 
…ot allowed

Motivation:
There's some cases that we don't call `RequestLogBuilder.endRequest()`.
- When the method of the request is not allowed
- When there's no services that match the path of the request
- and so on

Because the `RequestLog` in never complete, `AccessLogWriter` is not called so we should fix it.

Modifications:
- Call `RequestLogBuilder.endRequest()` even when `service.serve()` is not invoked

Result:
- You can now see the access logs when the method of a request is not allowed and there's no services that match the path.
@minwoox minwoox added the defect label Oct 6, 2019
@minwoox minwoox added this to the 0.94.1 milestone Oct 6, 2019
@minwoox minwoox requested review from ikhoon and trustin as code owners October 6, 2019 15:08
minwoox
minwoox commented Oct 6, 2019
View reviewed changes
core/src/test/java/com/linecorp/armeria/server/HttpServerHandlerTest.java Outdated
}

@Test
void httpStatusException() {
Copy link
Contributor Author

@minwoox minwoox Oct 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I found out that we leave the httpStatusException as the cause but not for httpResponseException.
I think we should treat them in the same way and I feel we do not need to log the causes in both cases.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Could you update accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. PTAL 🙇

@codecov
Copy link

codecov bot commented Oct 6, 2019 •
edited
Loading

Codecov Report

Merging #2159 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2159      +/-   ##
============================================
- Coverage     73.59%   73.52%   -0.07%     
+ Complexity     9571     9566       -5     
============================================
  Files           837      837              
  Lines         36836    36856      +20     
  Branches       4543     4547       +4     
============================================
- Hits          27108    27098      -10     
- Misses         7402     7433      +31     
+ Partials       2326     2325       -1
Impacted Files Coverage Δ Complexity Δ
...com/linecorp/armeria/server/HttpServerHandler.java 81.33% <100%> (+0.52%) 81 <0> (ø) ⬇️
...rp/armeria/server/tomcat/ManagedTomcatService.java 50.84% <0%> (-22.04%) 6% <0%> (-1%)
...armeria/server/tomcat/Tomcat90ProtocolHandler.java 48.14% <0%> (-14.82%) 11% <0%> (-4%)
.../linecorp/armeria/internal/Http2ObjectEncoder.java 71.42% <0%> (-8.58%) 12% <0%> (-1%)
...meria/internal/AbstractHttp2ConnectionHandler.java 93.33% <0%> (-3.34%) 13% <0%> (-1%)
.../armeria/client/endpoint/dns/DnsEndpointGroup.java 83.15% <0%> (-3.16%) 13% <0%> (-2%)
...om/linecorp/armeria/server/jetty/JettyService.java 63.05% <0%> (-2.96%) 23% <0%> (-1%)
.../linecorp/armeria/server/tomcat/TomcatService.java 63.82% <0%> (-2.66%) 24% <0%> (-1%)
...a/com/linecorp/armeria/common/util/Exceptions.java 36.84% <0%> (-2.64%) 23% <0%> (-2%)
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 70.87% <0%> (-1.95%) 46% <0%> (-3%)
... and 14 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 326877a...cea6c3e. Read the comment docs.

trustin
trustin reviewed Oct 7, 2019
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks!

core/src/test/java/com/linecorp/armeria/server/HttpServerHandlerTest.java Outdated
}

@Test
void httpStatusException() {
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Could you update accordingly?

ikhoon
ikhoon approved these changes Oct 7, 2019
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

LGTM :-)

@trustin
Copy link
Member

trustin commented Oct 8, 2019

@ikhoon Still LGTY?

ikhoon
ikhoon approved these changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Still LGTM, Sorry for the late review. 😢

@trustin trustin changed the title Fix where access log is not written when the method of a request is n… Fix where access log is not written when the method of a reques… Oct 8, 2019
@trustin trustin merged commit a7fc031 into line:master Oct 8, 2019
@minwoox minwoox deleted the fix_end_request branch October 8, 2019 08:12
@minwoox
Copy link
Contributor Author

minwoox commented Oct 8, 2019

Thanks for reviewing! 😉

eugene70 pushed a commit to eugene70/armeria that referenced this pull request Oct 16, 2019
@minwoox
Fix where access log is not written when the method of a reques… (lin…
265ed97 
…e#2159)

…t is not allowed

Motivation:
There's some cases that we don't call `RequestLogBuilder.endRequest()`.
- When the method of the request is not allowed
- When there's no services that match the path of the request
- and so on

Because the `RequestLog` in never complete, `AccessLogWriter` is not called so we should fix it.

Modifications:
- Call `RequestLogBuilder.endRequest()` even when `service.serve()` is not invoked
- Do not log HttpStatusException as requestCause

Result:
- You can now see the access logs when the method of a request is not allowed and there are no services that match the path.
@minwoox minwoox mentioned this pull request Nov 7, 2019
Merged
7 tasks
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
@minwoox @trustin
Fix where access log is not written when the method of a reques… (lin…
5b24593 
…e#2159)

…t is not allowed

Motivation:
There's some cases that we don't call `RequestLogBuilder.endRequest()`.
- When the method of the request is not allowed
- When there's no services that match the path of the request
- and so on

Because the `RequestLog` in never complete, `AccessLogWriter` is not called so we should fix it.

Modifications:
- Call `RequestLogBuilder.endRequest()` even when `service.serve()` is not invoked
- Do not log HttpStatusException as requestCause

Result:
- You can now see the access logs when the method of a request is not allowed and there are no services that match the path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@trustin trustin trustin approved these changes

Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
0.95.0
Development

Successfully merging this pull request may close these issues.
3 participants
@minwoox @trustin @ikhoon