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

Improve access log handler to handle HEAD method. #791

Merged

Conversation

ksauzz
Copy link
Contributor

@ksauzz ksauzz commented Feb 5, 2014

This fix prevents to include BytesOut element on Head Object(KeyStat) request the same as Head Bucket(BucketStat).

Requires the latest erlcloud commit: https://github.com/basho/erlcloud/pull/12/commits
Fix #789

@ksauzz ksauzz self-assigned this Feb 5, 2014
@ksauzz ksauzz added this to the 1.4.5 milestone Feb 5, 2014
0;
_ ->
Length
end,
Copy link
Contributor

Choose a reason for hiding this comment

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

 BytesOut = case Method of
                  'HEAD' -> 0;
                  _ ->      Length
            end,

I prefer this style.

@kuenishi
Copy link
Contributor

kuenishi commented Feb 5, 2014

Would you mind tagging erlcloud as 0.4.3 point to it from rebar.confg ?

Then I'll give my small +1.

make test and make pulse was confirmed eventually.

@shino shino mentioned this pull request Feb 5, 2014
@shino
Copy link
Contributor

shino commented Feb 5, 2014

Sorry for stepping in, but I have a question/not-so-clear-point-for-me.

I don't what #wm_log_data.response_length represents, the name suggests
it's a length of response body, not Content-Length header value.
Does someone have information on this point?

If my above understanding is correct, #wm_log_data.response_length should be set
by Webmachine to zero for every HEAD request.

Quick fix by Webmachine side is:

diff --git a/src/webmachine_request.erl b/src/webmachine_request.erl
index 2a5ff7a..cc25d0c 100644
--- a/src/webmachine_request.erl
+++ b/src/webmachine_request.erl
@@ -389,7 +389,7 @@ send_response(Code, PassedState=#wm_reqstate{reqdata=RD}, _Req) ->
           make_code(Code), <<"\r\n">> |
          make_headers(Code, Length, RD)]),
     FinalLength = case wrq:method(RD) of
-         'HEAD' -> Length;
+         'HEAD' -> 0;
          _ ->
             case Body of
                 {stream, Body2} ->

With this fix,

  • Content-Length of response for HEAD request is correct, same as GET,
  • Access stats for KeyStat has only Count, no BytesOut.

@shino
Copy link
Contributor

shino commented Feb 5, 2014

Oh, the above quick fix has a side effect to access log [1].

BEFORE fix:

127.0.0.1 - - [06/Feb/2014:00:34:14 +0900] "HEAD /buckets/test5/objects/A.txt HTTP/1.1" 200 2 "" "curl/7.27.0"
                                                                                            ^

AFTER fix:

127.0.0.1 - - [06/Feb/2014:00:47:54 +0900] "HEAD /buckets/test5/objects/A.txt HTTP/1.1" 200 0 "" "curl/7.27.0"
                                                                                            ^

The number of "The response size in bytes" differs.
Zero is good for HEAD or not?

[1] Logging · basho/webmachine Wiki > https://github.com/basho/webmachine/wiki/Logging

@kuenishi
Copy link
Contributor

kuenishi commented Feb 6, 2014

Looks like a wm bug. Fixing webmachine won't conflict with this fix and r_t added here , which is very useful. I also prefer faster 1.4.5 roll out for customers. Let's fix webmachine eventually. Maybe it's busy but not too busy. Thoughts?

@shino
Copy link
Contributor

shino commented Feb 6, 2014

Bug is fixed by this PR. +1 for defering WM fix.
@ksauzz Would you file a issue about access log?

@ksauzz
Copy link
Contributor Author

ksauzz commented Feb 6, 2014

@kota I'll bump the version of erlcloud to 0.4.4 since the latest version is 0.4.3.
@shino OK. I'll do that later.

@kota
Copy link

kota commented Feb 6, 2014

@ksauzz @shino you guys mentioning a wrong person again.

@shino
Copy link
Contributor

shino commented Feb 6, 2014

@kota I'm very sorry for bothering again (and again)... Internal ID to mention @kuenishi is @kota so my fingers type the spell ....

@ksauzz
Copy link
Contributor Author

ksauzz commented Feb 6, 2014

@kota So sorry. Please accept my apologies....

@kota
Copy link

kota commented Feb 6, 2014

@shino @ksauzz No problem. :)

- Fix access_log_handler to filter out ByteOut on Head request.
@ksauzz
Copy link
Contributor Author

ksauzz commented Feb 6, 2014

Updated the code style and bumped erlcloud version.

ksauzz added a commit that referenced this pull request Feb 6, 2014
…cess-log-handler

Improve access log handler to handle HEAD method.
@ksauzz ksauzz merged commit d1ee70b into release/1.4 Feb 6, 2014
@ksauzz ksauzz deleted the bugfix/handling-head-request-on-access-log-handler branch February 6, 2014 06:18
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.

None yet

4 participants