[GitHub] [couchdb] iilyak commented on a change in pull request #1963: Add stats in fabric for partition and normal views

[GitBox <gi...@apache.org>]

iilyak commented on a change in pull request #1963: Add stats in fabric for partition and normal views
URL: https://github.com/apache/couchdb/pull/1963#discussion_r262652964
 
 

 ##########
 File path: src/chttpd/src/chttpd.erl
 ##########
 @@ -1045,6 +1051,39 @@ send_error(Req, Code, Headers, ErrorStr, ReasonStr, Stack) ->
         case Stack of [] -> []; _ -> [{<<"ref">>, stack_hash(Stack)}] end
     ]}).
 
+update_timeout_stats(Error, PathParts)
 
 Review comment:
   General rambling...
   
   I don't like that we are exposing endpoint paths in `chttpd.erl`. I know we already do request specific handling in `chttpd:error_info`. This makes implementation of an endpoint coupled with correct update in `chttpd`. I would rather prefer not to leak this detail. However I don't see an easy way to achieve that without changing the way how http handlers are plugged. Changing that aspect is clearly out of scope of the PR. 
   
   Back to this PR...
   I think we should convert this function to use function clauses. It might look something like:
   ```
   update_timeout_stats(<<"timeout">>, #httpd{requested_path_parts = RequestedPathParts} = Req) ->
      update_timeout_stats(RequestedPathParts);
   update_timeout_stats(timeout, #httpd{requested_path_parts = RequestedPathParts} = Req) ->
      update_timeout_stats(RequestedPathParts).
   
   update_timeout_stats([_, <<"_partition">>, _, <<"_design">>, _, <<"_view">> | _]) ->
        couch_stats:increment_counter([couchdb, httpd, partition_view_timeouts]);
   ...
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services