You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by eiri <gi...@git.apache.org> on 2016/03/16 20:49:56 UTC
[GitHub] couchdb-couch pull request: Fix log_response/2
GitHub user eiri opened a pull request:
https://github.com/apache/couchdb-couch/pull/153
Fix log_response/2
After recent refactoring `log_response/2` can also receive an unencoded JSON as part of the error message.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-couch fix-log_response
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch/pull/153.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #153
----
commit 94afbffb05584607331e41c0eb1fb7d7e66d0156
Author: Eric Avdey <ei...@eiri.ca>
Date: 2016-03-16T19:45:00Z
Fix log_response
After recent refactoring `log_response/2` can also receive
an unencoded JSON as part of the error message.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-couch/pull/153#issuecomment-197949086
@iilyak I once had a production system that was doing jiffy encoding of ~1Mb messages at rate about 80 msg/s without breaking a sweet, so I have my doubts about optimizing without measuring. On the other hand if anything couch core code taught me is to treasure functions, that I can grasp propose of just glancing over, instead of stopping and reading attentively.
Anyway, everyone's entitled to their own opinions, so that's a moot point. The PR squashed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-couch/pull/153
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/153#issuecomment-197550702
@eiri: Thank you for noticing the problem. Wouldn't it be better to call `couch_util:json_encode(JsonObj)` after we check `dont_log_response`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by eiri <gi...@git.apache.org>.
Github user eiri commented on the pull request:
https://github.com/apache/couchdb-couch/pull/153#issuecomment-197632013
@iilyak this sounds like micro-optimization, tbh, and leads to not the prettiest code, but if you insist - sure, here we go.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/153#issuecomment-197727690
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/153#issuecomment-197940238
+1 after cleaning up the history
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/153#issuecomment-197936629
@eiri: I would disagree that it is a micro optimization. We would be doing `json -> binary` conversion for every request (which involves `{json, Obj}`) twice if it is coming from clustered interface.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/153#discussion_r56526095
--- Diff: src/couch_httpd.erl ---
@@ -649,16 +649,17 @@ log_request(#httpd{mochi_req=MochiReq,peer=Peer}=Req, Code) ->
gen_event:notify(couch_plugin, {log_request, Req, Code})
end.
+log_response(Code, _) when Code < 400 ->
+ ok;
log_response(Code, Body) ->
- case erlang:get(dont_log_response) of
- true ->
+ case {erlang:get(dont_log_response), Body} of
+ {true, _} ->
ok;
- _ when Code >= 500 ->
- couch_log:error("httpd ~p error response:~n ~s", [Code, Body]);
- _ when Code >= 400 ->
- couch_log:error("httpd ~p error response:~n ~s", [Code, Body]);
+ {_, {json, JsonObj}} ->
--- End diff --
Never mind
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/153#discussion_r56525959
--- Diff: src/couch_httpd.erl ---
@@ -649,16 +649,17 @@ log_request(#httpd{mochi_req=MochiReq,peer=Peer}=Req, Code) ->
gen_event:notify(couch_plugin, {log_request, Req, Code})
end.
+log_response(Code, _) when Code < 400 ->
+ ok;
log_response(Code, Body) ->
- case erlang:get(dont_log_response) of
- true ->
+ case {erlang:get(dont_log_response), Body} of
+ {true, _} ->
ok;
- _ when Code >= 500 ->
- couch_log:error("httpd ~p error response:~n ~s", [Code, Body]);
- _ when Code >= 400 ->
- couch_log:error("httpd ~p error response:~n ~s", [Code, Body]);
+ {_, {json, JsonObj}} ->
--- End diff --
Never mind.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-couch pull request: Fix log_response/2
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-couch/pull/153#discussion_r56525561
--- Diff: src/couch_httpd.erl ---
@@ -649,16 +649,17 @@ log_request(#httpd{mochi_req=MochiReq,peer=Peer}=Req, Code) ->
gen_event:notify(couch_plugin, {log_request, Req, Code})
end.
+log_response(Code, _) when Code < 400 ->
+ ok;
log_response(Code, Body) ->
- case erlang:get(dont_log_response) of
- true ->
+ case {erlang:get(dont_log_response), Body} of
+ {true, _} ->
ok;
- _ when Code >= 500 ->
- couch_log:error("httpd ~p error response:~n ~s", [Code, Body]);
- _ when Code >= 400 ->
- couch_log:error("httpd ~p error response:~n ~s", [Code, Body]);
+ {_, {json, JsonObj}} ->
--- End diff --
It looks like you dropped `400 <= Code >= 500` condition which would mean we would be logging more than before.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---