You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/02/23 16:34:00 UTC
[GitHub] [couchdb] iilyak opened a new pull request #3380: Clean up logs 3.x.v2
iilyak opened a new pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380
## Overview
Port https://github.com/apache/couchdb/pull/3031 (Clean up logs) into 3.x branch.
This PR deals with cleaning up logs from user's data (potentially sensitive) in the cases when OTP processes crash. All commits except "Add format_status/2 callback in gen_server implementations" were cherry picked from https://github.com/apache/couchdb/pull/3031. The "Add format_status/2 callback in gen_server implementations" was extended to include applications relevant for 3.x.
The approach to `format_status/2` is to return term as is when first argument is `normal` in order for `sys:get_status` to return original term. The only exception from this rule is stripping of passwords from replicator state. The status returned from `terminate` clause removes credentials and omit big terms.
## Testing recommendations
`make eunit`
## Related Issues or Pull Requests
- [ ] https://github.com/apache/couchdb/pull/3372 - previous attempt
- [ ] https://github.com/apache/couchdb/pull/3031 - implementation on main
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794339725
I think I would need to create a new repo for the parse_transform code. Because it need to be used as dependency for other applications which are not part of the monolithic repository.
I am trying to choose between
1. a repository specifically for this transform
2. a repository which we would use to hold all transforms we ever implement
3. a repository with utility functions which would also have parse transforms in there
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794229537
The alternative to `map()` for the `Ctx` is a KV list. However map is superior because it can be matched in the function clause. The KV list preserve the order of elements so cannot be matched against. Which simplify dispatch in couch_log_formatter and sanitizer module.
```
format_status(terminate, [PDict, State]) ->
{State, #{module => ?MODULE, dictionary => PDict}}.
sanitizer:sanitize_state(State, #{module := couch_server}) ->
....;
sanitizer:sanitize_state(State, #{module := smoosh_server}) ->
....;
```
compared to
```
format_status(terminate, [PDict, State]) ->
{State, [{module, ?MODULE}, {dictionary, PDict}]}.
sanitizer:sanitize_state(State, Ctx) ->
Module = case lists:keyfind(module, 1, Ctx) -> of
{module, M} -> M;
false -> undefined
end,
PDict = case lists:keyfind(dictionary, 1, Ctx) -> of
{dictionary, D} -> D;
false -> []
end,
sanitize_state(State, Module, PDict).
sanitizer:sanitize_state(State, smoosh_server, PDict) ->
....;
sanitizer:sanitize_state(State, couch_server, PDict) ->
....;
```
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794200227
I don't understand these;
```
format_status(normal, [_PDict, State]) ->
[{data, [{"State", State}]}];
format_status(terminate, [PDict, State]) ->
{State, #{module => ?MODULE, dictionary => PDict}}.
```
Why so many conversions to map?
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-784417076
> I think this might have a better chance if we focus on redacting sensitive data first, and tackle garbage cleanup later.
Agree. I'll update the PR to decouple garbage cleanup out of this PR.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794227209
we also can't use the ableist slur "sanitize" but that's a mechanical change ("scrub" perhaps?). will ponder if the boilerplating can be reduced/removed (my concern is not just the duplication but also the deviation from OTP and the need to remember to do this for every new gen_server).
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794224328
I am very uncomfortable to see that pattern cut and paste so many times, it strongly implies a bad design. I think I see how you got here though.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-784372133
Ah, sure, I get that point, thank you.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-800426693
I rolled back the controversial changes and included only the changes needed to prevent leaking of passwords. I think we can have separate PR if we would want to prevent leaking of users' data or minimize state terms.
----------------------------------------------------------------
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
[GitHub] [couchdb] janl commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-784416097
Hey hey, just casually following this, but the diff is a bit complex. I think this might have a better chance if we focus on redacting sensitive data first, and tackle garbage cleanup later.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-784367867
> I don't think redaction beyond obvious things like passwords can be the default behaviour of CouchDB. I'm less sure if there should be an option for the kind of deep "cleanup" here. With my couchdb support hat on, would I be happy to diagnose a problem from a CouchDB log this heavily modified?
Personally I hit the same problem over and over. I have a bug and go to check logs. The log contains huge term which we truncate and all important information about other fields of a record omitted.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794235753
> I am very uncomfortable to see that pattern cut and paste so many times, it strongly implies a bad design. I think I see how you got here though.
The alternative is a common module somewhere.
```
format_status(terminate, [PDict, State]) ->
Scrubed = ...
some_module:format_status(Scrubbed, PDict).
```
However the gen_servers we update are in different applications. We don't have an application which is included in all the others. I could create a new app which would hold the module. This app would be only for utility functions and couldn't depend on other apps to avoid circular dependencies.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794231957
now you've explained I'm fine with the "context" item being a map. What's really awkward is having to alter every gen_server in an identical way in order to pass it.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794215510
> I don't understand these;
>
> ```
> format_status(normal, [_PDict, State]) ->
> [{data, [{"State", State}]}];
>
> format_status(terminate, [PDict, State]) ->
> {State, #{module => ?MODULE, dictionary => PDict}}.
> ```
>
> Why so many conversions to map?
There is no conversion to map for the state of the `gen_server`. The state is returned as is. The only thing I do here is return additional context. I.e. return `{State, Ctx}` instead of `State`. Then the `couch_log_formatter` passes the Ctx to plug-able sanitizer module. If `sanitizer` is not specified `Ctx` is not used or logged.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#discussion_r581200542
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
Review comment:
I do like that we don't interfere with a sys:get_status call from a remsh (etc).
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
+ reduce_record(St, undefined).
+
+reduce_record(#st{} = St, _) ->
+ #st{
+ header = Header,
+ id_tree = IdTree,
+ seq_tree = SeqTree,
+ local_tree = LocalTree,
+ purge_tree = PurgeTree,
+ purge_seq_tree = PurgeSeqTree
+ } = St,
+ St#st{
+ header = reduce_record(Header, undefined),
+ id_tree = reduce_record(IdTree, undefined),
+ seq_tree = reduce_record(SeqTree, undefined),
+ local_tree = reduce_record(LocalTree, undefined),
+ purge_tree = reduce_record(PurgeTree, undefined),
+ purge_seq_tree = reduce_record(PurgeSeqTree, undefined)
+ };
+
+reduce_record(Header, _) when element(1, Header) =:= db_header ->
+ couch_bt_engine_header:reduce_record(Header);
+
+reduce_record(Tree, _) when element(1, Tree) =:= btree ->
+ {tree_size, couch_btree:size(Tree)};
Review comment:
this redacts every part of the btree's and replaces it with just the tree size (which isn't part of the state). I'm not seeing anything sensitive in the btree attributes to warrant this.
##########
File path: src/couch/src/couch_native_process.erl
##########
@@ -125,6 +125,20 @@ handle_info({'EXIT',_,Reason}, State) ->
terminate(_Reason, _State) -> ok.
code_change(_OldVersion, State, _Extra) -> {ok, State}.
+format_status(normal, [_PDict, State]) ->
+ [{data, [{"State", State}]}];
+
+format_status(terminate, [_PDict, State]) ->
+ #evstate{
+ ddocs = DDocs,
+ funs = Funs
+ } = State,
+ Scrubbed = State#evstate{
+ ddocs = {dict_size, dict:size(DDocs)},
Review comment:
arguably not sensitive either
##########
File path: src/chttpd/src/chttpd_node.erl
##########
@@ -71,7 +71,9 @@ handle_node_req(#httpd{method='PUT', path_parts=[_, Node, <<"_config">>, Section
Value = couch_util:trim(chttpd:json_body(Req)),
Persist = chttpd:header_value(Req, "X-Couch-Persist") /= "false",
OldValue = call_node(Node, config, get, [Section, Key, ""]),
- case call_node(Node, config, set, [Section, Key, ?b2l(Value), Persist]) of
+ IsSensitive = Section == <<"admins">>,
+ Opts = #{persisit => Persist, sensitive => IsSensitive},
Review comment:
typo
##########
File path: src/couch/src/couch_bt_engine_header.erl
##########
@@ -327,6 +328,27 @@ latest(N) when is_integer(N), N < ?LATEST_DISK_VERSION ->
latest(_Else) ->
undefined.
+
+reduce_record(#db_header{} = Header) ->
+ #db_header{
+ id_tree_state = IdTreeState,
+ seq_tree_state = SeqTreeState,
+ local_tree_state = LocalTreeState
+ } = Header,
+ Header#db_header{
+ id_tree_state = {tree_size, tree_state_size(IdTreeState)},
+ seq_tree_state = {tree_size, tree_state_size(SeqTreeState)},
+ local_tree_state = {tree_size, tree_state_size(LocalTreeState)}
+ }.
+
+
+tree_state_size(TreeState) ->
+ %% We just need a correctly shaped btree record,
+ %% so we can pass Fd = nil
+ {ok, Tree} = couch_btree:open(TreeState, nil),
+ couch_btree:size(Tree).
Review comment:
same comment as above
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
Review comment:
I think `redact` is a better word as we have a quite different meaning for `reduce` in couchdb.
##########
File path: src/couch/src/couch_multidb_changes.erl
##########
@@ -174,6 +175,18 @@ code_change(_OldVsn, State, _Extra) ->
{ok, State}.
+format_status(normal, [_PDict, State]) ->
+ [{data, [{"State", State}]}];
+
+format_status(terminate, [_PDict, State]) ->
+ #state{
+ pids=Pids
+ } = State,
+ Scrubbed = State#state{
+ pids={length, length(Pids)}
Review comment:
pids aren't sensitive.
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
+ reduce_record(St, undefined).
+
+reduce_record(#st{} = St, _) ->
+ #st{
+ header = Header,
+ id_tree = IdTree,
+ seq_tree = SeqTree,
+ local_tree = LocalTree,
+ purge_tree = PurgeTree,
+ purge_seq_tree = PurgeSeqTree
+ } = St,
+ St#st{
+ header = reduce_record(Header, undefined),
+ id_tree = reduce_record(IdTree, undefined),
+ seq_tree = reduce_record(SeqTree, undefined),
+ local_tree = reduce_record(LocalTree, undefined),
+ purge_tree = reduce_record(PurgeTree, undefined),
+ purge_seq_tree = reduce_record(PurgeSeqTree, undefined)
+ };
+
+reduce_record(Header, _) when element(1, Header) =:= db_header ->
Review comment:
should be `when is_record(Header, db_header)`.
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
+ reduce_record(St, undefined).
+
+reduce_record(#st{} = St, _) ->
+ #st{
+ header = Header,
+ id_tree = IdTree,
+ seq_tree = SeqTree,
+ local_tree = LocalTree,
+ purge_tree = PurgeTree,
+ purge_seq_tree = PurgeSeqTree
+ } = St,
+ St#st{
+ header = reduce_record(Header, undefined),
+ id_tree = reduce_record(IdTree, undefined),
+ seq_tree = reduce_record(SeqTree, undefined),
+ local_tree = reduce_record(LocalTree, undefined),
+ purge_tree = reduce_record(PurgeTree, undefined),
+ purge_seq_tree = reduce_record(PurgeSeqTree, undefined)
+ };
+
+reduce_record(Header, _) when element(1, Header) =:= db_header ->
+ couch_bt_engine_header:reduce_record(Header);
+
+reduce_record(Tree, _) when element(1, Tree) =:= btree ->
Review comment:
should be `when is_record(Tree, btree)`.
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -173,3 +173,8 @@ get_body(Req) ->
couch_log:notice("Body Fail: ~p~n", [Else]),
couch_httpd:send_error(Req, 400, <<"bad_request">>, <<"Missing JSON body'">>)
end.
+
+remove_sensitive(KVList0) ->
+ KVList1 = lists:keyreplace(<<"username">>, 1, KVList0, {<<"username">>, <<"****">>}),
Review comment:
username isn't sensitive (or shouldn't be!)
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -173,3 +173,8 @@ get_body(Req) ->
couch_log:notice("Body Fail: ~p~n", [Else]),
couch_httpd:send_error(Req, 400, <<"bad_request">>, <<"Missing JSON body'">>)
end.
+
+remove_sensitive(KVList0) ->
+ KVList1 = lists:keyreplace(<<"username">>, 1, KVList0, {<<"username">>, <<"****">>}),
+ KVList2 = lists:keyreplace(<<"password">>, 1, KVList1, {<<"password">>, <<"****">>}),
+ KVList2.
Review comment:
a style point (and I guess you're anticipating other fields in future) but I would list return the lists:keyreplace result
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794243457
changing all the rebar.configs seems ok, and would have the advantage that any new gen_servers are automatically covered. Did you show the parse_transform version? I've lost track.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#discussion_r581198379
##########
File path: src/setup/src/setup_httpd.erl
##########
@@ -173,3 +173,8 @@ get_body(Req) ->
couch_log:notice("Body Fail: ~p~n", [Else]),
couch_httpd:send_error(Req, 400, <<"bad_request">>, <<"Missing JSON body'">>)
end.
+
+remove_sensitive(KVList0) ->
+ KVList1 = lists:keyreplace(<<"username">>, 1, KVList0, {<<"username">>, <<"****">>}),
+ KVList2 = lists:keyreplace(<<"password">>, 1, KVList1, {<<"password">>, <<"****">>}),
+ KVList2.
Review comment:
a style point (and I guess you're anticipating other fields in future) but I would just return the lists:keyreplace result
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on a change in pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#discussion_r581261272
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
Review comment:
The `format_status/`2 was added to prevent huge btree structures hitting logs. Since the reduction of terms is not longer a goal I am thinking about removing this function.
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
Review comment:
The `format_status/2` was added to prevent huge btree structures hitting logs. Since the reduction of terms is not longer a goal I am thinking about removing this function.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on a change in pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#discussion_r581256199
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
+ reduce_record(St, undefined).
+
+reduce_record(#st{} = St, _) ->
+ #st{
+ header = Header,
+ id_tree = IdTree,
+ seq_tree = SeqTree,
+ local_tree = LocalTree,
+ purge_tree = PurgeTree,
+ purge_seq_tree = PurgeSeqTree
+ } = St,
+ St#st{
+ header = reduce_record(Header, undefined),
+ id_tree = reduce_record(IdTree, undefined),
+ seq_tree = reduce_record(SeqTree, undefined),
+ local_tree = reduce_record(LocalTree, undefined),
+ purge_tree = reduce_record(PurgeTree, undefined),
+ purge_seq_tree = reduce_record(PurgeSeqTree, undefined)
+ };
+
+reduce_record(Header, _) when element(1, Header) =:= db_header ->
Review comment:
The `is_record/2` works only for records in scope of a module (defined locally or included). The `db_header` is a private record defined in `couch_bt_engine_header.erl`.
```
couchdb/src/couch/src/couch_bt_engine.erl:224: record db_header undefined
Compiling couchdb/src/couch/src/couch_bt_engine.erl failed:
ERROR: compile failed while processing couchdb/src/couch: rebar_abort
```
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on a change in pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#discussion_r581258440
##########
File path: src/couch/src/couch_bt_engine.erl
##########
@@ -192,6 +193,44 @@ handle_db_updater_info({'DOWN', Ref, _, _, _}, #st{fd_monitor=Ref} = St) ->
{stop, normal, St#st{fd=undefined, fd_monitor=closed}}.
+format_status(normal, [_PDict, #st{} = St]) ->
+ [{data, [{"State", St}]}];
+
+format_status(terminate, [_PDict, #st{} = St]) ->
+ [{data, [{"State", reduce_record(St)}]}].
+
+
+reduce_record(#st{} = St) ->
+ reduce_record(St, undefined).
+
+reduce_record(#st{} = St, _) ->
+ #st{
+ header = Header,
+ id_tree = IdTree,
+ seq_tree = SeqTree,
+ local_tree = LocalTree,
+ purge_tree = PurgeTree,
+ purge_seq_tree = PurgeSeqTree
+ } = St,
+ St#st{
+ header = reduce_record(Header, undefined),
+ id_tree = reduce_record(IdTree, undefined),
+ seq_tree = reduce_record(SeqTree, undefined),
+ local_tree = reduce_record(LocalTree, undefined),
+ purge_tree = reduce_record(PurgeTree, undefined),
+ purge_seq_tree = reduce_record(PurgeSeqTree, undefined)
+ };
+
+reduce_record(Header, _) when element(1, Header) =:= db_header ->
Review comment:
hm, true.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794238836
In fact my first take on this problem was a parse transform. The problem with this approach is use of `rebar`. Rebar has bugs the parse transform configured in main application is not passed to included application for umbrella apps. We would have to update `rebar.config` for each app we include.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
rnewson commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794236398
could this be done with a parse transform? When we used lager it added a parse transform (https://github.com/apache/couchdb-lager/blob/master/src/lager_transform.erl) which captures module name among other things.
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-794327252
> Did you show the parse_transform version? I've lost track.
I didn't
----------------------------------------------------------------
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
[GitHub] [couchdb] iilyak merged pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
iilyak merged pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380
--
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
[GitHub] [couchdb] bessbd commented on a change in pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#discussion_r581203669
##########
File path: src/chttpd/src/chttpd_node.erl
##########
@@ -71,7 +71,9 @@ handle_node_req(#httpd{method='PUT', path_parts=[_, Node, <<"_config">>, Section
Value = couch_util:trim(chttpd:json_body(Req)),
Persist = chttpd:header_value(Req, "X-Couch-Persist") /= "false",
OldValue = call_node(Node, config, get, [Section, Key, ""]),
- case call_node(Node, config, set, [Section, Key, ?b2l(Value), Persist]) of
+ IsSensitive = Section == <<"admins">>,
+ Opts = #{persisit => Persist, sensitive => IsSensitive},
Review comment:
Nit
```suggestion
Opts = #{persist => Persist, sensitive => IsSensitive},
```
----------------------------------------------------------------
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
[GitHub] [couchdb] janl commented on pull request #3380: Clean up logs 3.x.v2
Posted by GitBox <gi...@apache.org>.
janl commented on pull request #3380:
URL: https://github.com/apache/couchdb/pull/3380#issuecomment-795453294
Good cleanup @iilyak! This is a lot easer to comprehend.
The one place where this could do with a comment about what happens is https://github.com/apache/couchdb/pull/3380/files#diff-33000329e1e5eb2554d72f4675fb32c2bf3e0599c981f044683467cb25efc02aR378 which seems very meta and it is not clear to me what is happening there, or if we still need it, now that ddocs are out of scope for redacting.
I agree with @rnewson on the duplication and agree with methods to mitigate. A separate repo sounds good. I would go with option 1. until we have more transforms, at which point we can move things around.
----------------------------------------------------------------
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