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/04/23 20:23:52 UTC
[GitHub] [couchdb] kocolosk opened a new pull request #3526: Adopt structured logging via new Erlang 21 logger
kocolosk opened a new pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526
## Overview
This PR adds log statement as structured reports (i.e. Erlang maps) using the macros defined by Erlang itself as part of the new approach to logging implemented in Erlang 21.
## Testing recommendations
Use `dev/run` and inspect the new log files in `dev/logs`
## Related Issues or Pull Requests
Initial discussion about structured logging dates to at least #1373
## Checklist
- [x] Code is written and works correctly
- [ ] 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 a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623026862
##########
File path: src/chttpd/src/chttpd_handlers.erl
##########
@@ -44,7 +45,12 @@ handler_info(HttpReq) ->
Default = {'unknown.unknown', #{}},
try
select(collect(handler_info, [Method, PathParts, HttpReq]), Default)
- catch ?STACKTRACE(Type, Reason, Stack)
+ catch Type:Reason:Stack ->
+ ?LOG_ERROR(#{
Review comment:
`what => handler_info_failure,`?
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623117158
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -270,24 +282,31 @@ find_cluster_file([{Type, Location} | Rest]) ->
),
{ok, Location};
{ok, _} ->
+ ?LOG_ERROR(Msg#{
+ status => permissions_error,
+ details => "CouchDB needs read/write access to FDB cluster file"
+ }),
couch_log:error(
"CouchDB needs read/write access to FDB cluster file: ~s",
[Location]
),
{error, cluster_file_permissions};
{error, Reason} when Type =:= custom ->
+ ?LOG_ERROR(Msg#{status => Reason}),
couch_log:error(
"Encountered ~p error looking for FDB cluster file: ~s",
[Reason, Location]
),
{error, Reason};
{error, enoent} when Type =:= default ->
+ ?LOG_INFO(Msg#{status => enoent}),
Review comment:
Or call file:format_error(Reason)?
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623074340
##########
File path: src/couch_replicator/src/couch_replicator_worker.erl
##########
@@ -265,6 +271,14 @@ fetch_doc(Source, {Id, Revs, PAs}, DocHandler, Acc) ->
[Id, couch_doc:revs_to_strs(Revs)]),
WaitMSec = config:get_integer("replicator", "missing_doc_retry_msec",
?MISSING_DOC_RETRY_MSEC),
+ ?LOG_ERROR(#{
+ what => missing_document,
+ in => replicator,
+ source => couch_replicator_api_wrap:db_uri(Source),
+ docid => Id,
+ revisions => couch_doc:revs_to_strs(Revs),
+ retry_delay => WaitMSec / 1000
Review comment:
Needs unit suffix.
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r621207699
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -268,6 +275,11 @@ handle_request_int(MochiReq) ->
span_ok(HttpResp),
{ok, Resp};
#httpd_resp{status = aborted, reason = Reason} ->
+ ?LOG_ERROR(#{
+ what => abnormal_response_termation,
+ id => get(nonce),
Review comment:
Addressed in 5a139b249
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r620718356
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -268,6 +275,11 @@ handle_request_int(MochiReq) ->
span_ok(HttpResp),
{ok, Resp};
#httpd_resp{status = aborted, reason = Reason} ->
+ ?LOG_ERROR(#{
+ what => abnormal_response_termation,
+ id => get(nonce),
Review comment:
Excellent idea on `set_process_metadata`, I imagine there are a few locations where that could come in handy over time.
I'm OK changing `id` as well.
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623109396
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -254,14 +255,25 @@ find_cluster_file([{custom, undefined} | Rest]) ->
find_cluster_file(Rest);
find_cluster_file([{Type, Location} | Rest]) ->
+ Msg = #{
+ in => fdb_connection_setup,
Review comment:
No, good catch. Wondering if that `in` ought to be a `what`; there aren't any other messages with the same value of `in`
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623062290
##########
File path: src/couch/src/couch_proc_manager.erl
##########
@@ -321,7 +322,7 @@ find_proc(#client{lang = Lang, ddoc = DDoc, ddoc_key = DDocKey} = Client) ->
find_proc(Lang, Fun) ->
try iter_procs(Lang, Fun)
- catch ?STACKTRACE(error, Reason, StackTrace)
+ catch error:Reason:StackTrace ->
Review comment:
I removed all the ?STACKTRACE invocations because I was removing the macro, but I didn't add logging to these modules because as far as I can tell they are leftover cruft and should be removed entirely in favor of the ones in `couch_js`
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619508970
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -325,6 +349,12 @@ apply_tx_option(Db, Option, Val, binary) ->
true ->
set_option(Db, Option, BinVal);
false ->
+ ?LOG_ERROR(#{
+ what => invalid_transaction_option_value,
+ option => Option,
+ value => Val,
+ details => "string transaction option must be less than 16 bytes"
Review comment:
Would it be `less than` or `less than or equal`?
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623042001
##########
File path: src/couch/src/couch_proc_manager.erl
##########
@@ -321,7 +322,7 @@ find_proc(#client{lang = Lang, ddoc = DDoc, ddoc_key = DDocKey} = Client) ->
find_proc(Lang, Fun) ->
try iter_procs(Lang, Fun)
- catch ?STACKTRACE(error, Reason, StackTrace)
+ catch error:Reason:StackTrace ->
Review comment:
Is it intentional omission of?
```
catch error:Reason:StackTrace ->
?LOG_ERROR(#{
what => process_lookup_failure,
result => error,
details => Reason,
stack => StackTrace
}),
```
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623106343
##########
File path: src/couch_replicator/src/couch_replicator_job.erl
##########
@@ -299,6 +348,12 @@ handle_info({'EXIT', Pid, normal}, #rep_state{workers = Workers} = State) ->
Workers ->
%% Processes might be linked by replicator's auth plugins so
%% we tolerate them exiting `normal` here and don't crash
+ ?LOG_WARNING(#{
+ what => unknown_process_exit,
Review comment:
Changed to `linked_process_exit`
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623075718
##########
File path: src/couch_replicator/src/couch_replicator_worker.erl
##########
@@ -354,6 +383,11 @@ maybe_flush_docs(#httpdb{} = Target, Batch, Doc) ->
JsonDoc = ?JSON_ENCODE(couch_doc:to_json_obj(Doc, [revs, attachments])),
case SizeAcc + iolist_size(JsonDoc) of
SizeAcc2 when SizeAcc2 > ?DOC_BUFFER_BYTE_SIZE ->
+ ?LOG_DEBUG(#{
+ what => flush_doc_batch,
+ in => replicator,
+ batch_size => SizeAcc2
Review comment:
Needs unit suffix` _bytes`.
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619543209
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -56,6 +57,12 @@ ddoc_to_mrst(DbName, #doc{id=Id, body={Fields}}) ->
DictBySrcAcc
end;
({Name, Else}, DictBySrcAcc) ->
+ ?LOG_ERROR(#{
+ what => invalid_view_definition,
+ db => DbName,
+ ddoc => Id,
+ view => Name
Review comment:
Yes, this was an intentional omission on my part. I didn't make many changes like this but I did try to avoid logging user data except for DB names and document IDs.
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623107692
##########
File path: src/chttpd/src/chttpd_handlers.erl
##########
@@ -44,7 +45,12 @@ handler_info(HttpReq) ->
Default = {'unknown.unknown', #{}},
try
select(collect(handler_info, [Method, PathParts, HttpReq]), Default)
- catch ?STACKTRACE(Type, Reason, Stack)
+ catch Type:Reason:Stack ->
+ ?LOG_ERROR(#{
Review comment:
👍
--
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] kocolosk merged pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk merged pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623042831
##########
File path: src/couch/src/couch_sup.erl
##########
@@ -118,21 +129,37 @@ maybe_launch_admin_annoyance_reporter() ->
notify_starting() ->
+ ?LOG_INFO(#{
+ what => starting_couchdb,
+ version => couch_server:get_version()
+ }),
couch_log:info("Apache CouchDB ~s is starting.~n", [
couch_server:get_version()
]).
notify_started() ->
+ ?LOG_INFO(#{
+ what => starting_couchdb_complete,
+ time_to_relax => true
Review comment:
:-)
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623121972
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -254,14 +255,25 @@ find_cluster_file([{custom, undefined} | Rest]) ->
find_cluster_file(Rest);
find_cluster_file([{Type, Location} | Rest]) ->
+ Msg = #{
+ in => fdb_connection_setup,
Review comment:
I think the `status` could become `what` as well. Except the `status => ok` case.
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619542832
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -325,6 +349,12 @@ apply_tx_option(Db, Option, Val, binary) ->
true ->
set_option(Db, Option, BinVal);
false ->
+ ?LOG_ERROR(#{
+ what => invalid_transaction_option_value,
+ option => Option,
+ value => Val,
+ details => "string transaction option must be less than 16 bytes"
Review comment:
The `case` statement checks for `< 16` so that's what I put into the log message. I know the previous log message said differently. I didn't check if a 16 byte was actually allowed by `erlfdb` or not; if it is, we should update the test in the `case` statement as well as the log message.
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619874015
##########
File path: src/couch/src/couch_flags_config.erl
##########
@@ -132,10 +137,7 @@ parse_flags_term(FlagsBin) ->
[] ->
lists:usort(Flags);
_ ->
- {error, {
- "Cannot parse list of tags: ~n~p",
- Errors
- }}
+ {error, Errors}
Review comment:
I ran the unit tests and noticed this change would need a corresponding change in the tests suite
```
in call from eunit_proc:run_group/2 (eunit_proc.erl, line 561)
**error:{assertEqual,[{module,couch_flags_config_tests},
{line,115},
{expression,"couch_flags_config : parse_flags_term ( LongBinary )"},
{expected,{error,{"Cannot parse list of tags: ~n~p",
[{too_long,<<"aaaaaaaaaaaaaaaa"...>>}]}}},
{value,{error,[{too_long,<<"aaaaaaaaaaaaaaaaaaaa"...>>}]}}]}
output:<<"">>
```
##########
File path: src/couch/src/couch_flags_config.erl
##########
@@ -132,10 +137,7 @@ parse_flags_term(FlagsBin) ->
[] ->
lists:usort(Flags);
_ ->
- {error, {
- "Cannot parse list of tags: ~n~p",
- Errors
- }}
+ {error, Errors}
Review comment:
This should fix it:
```diff
diff --git a/src/couch/test/eunit/couch_flags_config_tests.erl b/src/couch/test/eunit/couch_flags_config_tests.erl
index ed7df1123..63fabfdde 100644
--- a/src/couch/test/eunit/couch_flags_config_tests.erl
+++ b/src/couch/test/eunit/couch_flags_config_tests.erl
@@ -98,10 +98,8 @@ test_config() ->
parse_flags_term_test_() ->
LongBinary = binary:copy(<<"a">>, ?MAX_FLAG_NAME_LENGTH + 1),
- ExpectedError = {error, {"Cannot parse list of tags: ~n~p",
- [{too_long, LongBinary}]}},
- ExpectedUnknownError = {error,{"Cannot parse list of tags: ~n~p",
- [{invalid_flag,<<"dddddddd">>}]}},
+ ExpectedError = {error, [{too_long, LongBinary}]},
+ ExpectedUnknownError = {error, [{invalid_flag,<<"dddddddd">>}]},
[
{"empty binary", ?_assertEqual(
[], couch_flags_config:parse_flags_term(<<>>))},
```
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623054971
##########
File path: src/couch_js/src/couch_js_os_process.erl
##########
@@ -115,12 +134,24 @@ readjson(OsProc) when is_record(OsProc, os_proc) ->
case ?JSON_DECODE(Line) of
[<<"log">>, Msg] when is_binary(Msg) ->
% we got a message to log. Log it and continue
+ ?LOG_INFO(#{
+ what => user_defined_log,
+ in => os_process,
+ port => OsProc#os_proc.port,
+ msg => Msg
+ }),
couch_log:info("OS Process ~p Log :: ~s",
[OsProc#os_proc.port, Msg]),
readjson(OsProc);
[<<"error">>, Id, Reason] ->
throw({error, {couch_util:to_existing_atom(Id),Reason}});
[<<"fatal">>, Id, Reason] ->
+ ?LOG_INFO(#{
Review comment:
Do we want to include `Id`?
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623118671
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -270,24 +282,31 @@ find_cluster_file([{Type, Location} | Rest]) ->
),
{ok, Location};
{ok, _} ->
+ ?LOG_ERROR(Msg#{
+ status => permissions_error,
+ details => "CouchDB needs read/write access to FDB cluster file"
+ }),
couch_log:error(
"CouchDB needs read/write access to FDB cluster file: ~s",
[Location]
),
{error, cluster_file_permissions};
{error, Reason} when Type =:= custom ->
+ ?LOG_ERROR(Msg#{status => Reason}),
couch_log:error(
"Encountered ~p error looking for FDB cluster file: ~s",
[Reason, Location]
),
{error, Reason};
{error, enoent} when Type =:= default ->
+ ?LOG_INFO(Msg#{status => enoent}),
couch_log:info(
"No FDB cluster file found at ~s",
[Location]
),
find_cluster_file(Rest);
{error, Reason} when Type =:= default ->
+ ?LOG_WARNING(Msg#{status => Reason}),
Review comment:
We potentially can call `file:format_error(Reason)` here. It depends on the type of a status field. Would you prefer it to stay atom?
--
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] kocolosk commented on pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#issuecomment-828737689
All set from my perspective now. This is only adding the logging invocations; all the work of defining and configuring the handlers, formatters, and filters will come in a separate 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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619543752
##########
File path: src/couch_replicator/src/couch_replicator_job.erl
##########
@@ -865,6 +948,11 @@ state_strip_creds(#rep_state{source = Source, target = Target} = State) ->
adjust_maxconn(Src = #{<<"http_connections">> := 1}, RepId) ->
+ ?LOG_NOTICE(#{
Review comment:
Good catch, fixed.
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619511969
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -16,6 +16,7 @@
-include_lib("couch/include/couch_db.hrl").
-include_lib("chttpd/include/chttpd.hrl").
+-include_lib("kernel/include/logger.hrl").
Review comment:
Since we include `logger.hrl` in `chttpd.hrl`, should also include it here?
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623060997
##########
File path: src/chttpd/src/chttpd_sup.erl
##########
@@ -99,6 +102,13 @@ append_if_set({Key, Value}, Opts) when Value > 0 ->
append_if_set({_Key, 0}, Opts) ->
Opts;
append_if_set({Key, Value}, Opts) ->
+ ?LOG_ERROR(#{
+ what => invalid_config_setting,
+ section => chttpd_auth_cache,
+ key => Key,
+ value => Value,
+ details => "value must be a positive integer"
Review comment:
Oh you know what, I think I just overlooked the second function clause there. "Positive integer" means 1.., this should read "value must be a non-negative integer"
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623077047
##########
File path: src/couch_replicator/src/couch_replicator_worker.erl
##########
@@ -371,12 +405,27 @@ flush_docs(Target, DocList) ->
handle_flush_docs_result(FlushResult, Target, DocList).
-handle_flush_docs_result({error, request_body_too_large}, _Target, [Doc]) ->
+handle_flush_docs_result({error, request_body_too_large}, Target, [Doc]) ->
+ ?LOG_ERROR(#{
+ what => doc_write_failure,
+ in => replicator,
+ target => couch_replicator_api_wrap:db_uri(Target),
+ reason => request_body_too_large,
+ docid => extract_value(<<"_id">>, Doc)
+ }),
couch_log:error("Replicator: failed to write doc ~p. Too large", [Doc]),
couch_replicator_stats:new([{doc_write_failures, 1}]);
handle_flush_docs_result({error, request_body_too_large}, Target, DocList) ->
Len = length(DocList),
{DocList1, DocList2} = lists:split(Len div 2, DocList),
+ ?LOG_NOTICE(#{
+ what => split_large_batch,
+ in => replicator,
+ target => couch_replicator_api_wrap:db_uri(Target),
+ reason => request_body_too_large,
+ original_batch_size => Len,
Review comment:
`original_batch_size_bytes`
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623160113
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -270,24 +282,31 @@ find_cluster_file([{Type, Location} | Rest]) ->
),
{ok, Location};
{ok, _} ->
+ ?LOG_ERROR(Msg#{
+ status => permissions_error,
+ details => "CouchDB needs read/write access to FDB cluster file"
+ }),
couch_log:error(
"CouchDB needs read/write access to FDB cluster file: ~s",
[Location]
),
{error, cluster_file_permissions};
{error, Reason} when Type =:= custom ->
+ ?LOG_ERROR(Msg#{status => Reason}),
couch_log:error(
"Encountered ~p error looking for FDB cluster file: ~s",
[Reason, Location]
),
{error, Reason};
{error, enoent} when Type =:= default ->
+ ?LOG_INFO(Msg#{status => enoent}),
couch_log:info(
"No FDB cluster file found at ~s",
[Location]
),
find_cluster_file(Rest);
{error, Reason} when Type =:= default ->
+ ?LOG_WARNING(Msg#{status => Reason}),
Review comment:
I like including the atoms for easier reporting but would be fine to include the `format_error` output in a `details` field.
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619507841
##########
File path: src/couch_views/src/couch_views_util.erl
##########
@@ -56,6 +57,12 @@ ddoc_to_mrst(DbName, #doc{id=Id, body={Fields}}) ->
DictBySrcAcc
end;
({Name, Else}, DictBySrcAcc) ->
+ ?LOG_ERROR(#{
+ what => invalid_view_definition,
+ db => DbName,
+ ddoc => Id,
+ view => Name
Review comment:
Noticed this doesn't include the `Else` bit. Probably for the better to avoid leaking internal design doc bits into the logs (even when they are invalid).
--
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] wohali commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
wohali commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619480709
##########
File path: rebar.config.script
##########
@@ -191,7 +191,7 @@ ErlOpts = case os:getenv("ERL_OPTS") of
end.
AddConfig = [
- {require_otp_vsn, "20|21|22|23"},
+ {require_otp_vsn, "21|22|23"},
Review comment:
can't review the rest but wanted to explicitly say "+1 for dropping 20.x on main, -1 for dropping 20.x on 3.x."
That means this wouldn't easily be backported to 3.x, until such time as we can confirm >20.x on 3.x code is stable at scale.
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623074553
##########
File path: src/couch_replicator/src/couch_replicator_worker.erl
##########
@@ -273,6 +287,14 @@ fetch_doc(Source, {Id, Revs, PAs}, DocHandler, Acc) ->
[Id, couch_doc:revs_to_strs(Revs)]),
WaitMSec = config:get_integer("replicator", "missing_doc_retry_msec",
?MISSING_DOC_RETRY_MSEC),
+ ?LOG_ERROR(#{
+ what => missing_attachment_stub,
+ in => replicator,
+ source => couch_replicator_api_wrap:db_uri(Source),
+ docid => Id,
+ revisions => couch_doc:revs_to_strs(Revs),
+ retry_delay => WaitMSec / 1000
Review comment:
Needs unit suffix
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619955790
##########
File path: src/couch/src/couch_flags_config.erl
##########
@@ -132,10 +137,7 @@ parse_flags_term(FlagsBin) ->
[] ->
lists:usort(Flags);
_ ->
- {error, {
- "Cannot parse list of tags: ~n~p",
- Errors
- }}
+ {error, Errors}
Review comment:
This should fix it:
```diff
diff --git a/src/couch/test/eunit/couch_flags_config_tests.erl b/src/couch/test/eunit/couch_flags_config_tests.erl
index ed7df1123..63fabfdde 100644
--- a/src/couch/test/eunit/couch_flags_config_tests.erl
+++ b/src/couch/test/eunit/couch_flags_config_tests.erl
@@ -98,10 +98,8 @@ test_config() ->
parse_flags_term_test_() ->
LongBinary = binary:copy(<<"a">>, ?MAX_FLAG_NAME_LENGTH + 1),
- ExpectedError = {error, {"Cannot parse list of tags: ~n~p",
- [{too_long, LongBinary}]}},
- ExpectedUnknownError = {error,{"Cannot parse list of tags: ~n~p",
- [{invalid_flag,<<"dddddddd">>}]}},
+ ExpectedError = {error, [{too_long, LongBinary}]},
+ ExpectedUnknownError = {error, [{invalid_flag,<<"dddddddd">>}]},
[
{"empty binary", ?_assertEqual(
[], couch_flags_config:parse_flags_term(<<>>))},
```
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623107088
##########
File path: src/couch_js/src/couch_js_os_process.erl
##########
@@ -115,12 +134,24 @@ readjson(OsProc) when is_record(OsProc, os_proc) ->
case ?JSON_DECODE(Line) of
[<<"log">>, Msg] when is_binary(Msg) ->
% we got a message to log. Log it and continue
+ ?LOG_INFO(#{
+ what => user_defined_log,
+ in => os_process,
+ port => OsProc#os_proc.port,
+ msg => Msg
+ }),
couch_log:info("OS Process ~p Log :: ~s",
[OsProc#os_proc.port, Msg]),
readjson(OsProc);
[<<"error">>, Id, Reason] ->
throw({error, {couch_util:to_existing_atom(Id),Reason}});
[<<"fatal">>, Id, Reason] ->
+ ?LOG_INFO(#{
Review comment:
included as `tag => Id` which I think is consistent with other non-JS error report structures.
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623051919
##########
File path: src/couch_jobs/src/couch_jobs_server.erl
##########
@@ -95,13 +96,22 @@ handle_info(check_types, St) ->
{noreply, St};
handle_info({'DOWN', _Ref, process, Pid, Reason}, St) ->
+ ?LOG_ERROR(#{
+ what => unknown_process_crash,
Review comment:
The `unknown_process_crash` is ambiguous. It could be understood as:
- the process we don't know about crashed
- the process crashed due to unknown reason
In this case it is either `notifier` or `monitor` process. Maybe we should say something like:
```
what => monitored_process_crash,
```
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r621210575
##########
File path: src/couch/src/couch_flags_config.erl
##########
@@ -132,10 +137,7 @@ parse_flags_term(FlagsBin) ->
[] ->
lists:usort(Flags);
_ ->
- {error, {
- "Cannot parse list of tags: ~n~p",
- Errors
- }}
+ {error, Errors}
Review comment:
Applied in 4ee64e5de
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623063517
##########
File path: src/couch_replicator/src/couch_replicator_changes_reader.erl
##########
@@ -60,14 +61,23 @@ read_changes(Parent, StartSeq, Db, ChangesQueue, Options) ->
N when N > 0 ->
put(retries_left, N - 1),
LastSeq = get(last_seq),
+ LogMsg = #{
+ what => retry_changes_feed,
+ in => replicator,
+ source => couch_replicator_api_wrap:db_uri(Db),
+ sequence => LastSeq,
+ retries_remaining => N
+ },
Db2 = case LastSeq of
StartSeq ->
+ ?LOG_NOTICE(LogMsg#{delay => Db#httpdb.wait / 1000}),
Review comment:
Needs unit suffix (`delay_sec`).
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623075319
##########
File path: src/couch_replicator/src/couch_replicator_worker.erl
##########
@@ -314,6 +338,11 @@ doc_handler_flush_doc(#doc{} = Doc, {Parent, Target} = Acc) ->
spawn_writer(Target, #batch{docs = DocList, size = Size}) ->
case {Target, Size > 0} of
{#httpdb{}, true} ->
+ ?LOG_DEBUG(#{
+ what => flush_doc_batch,
+ in => replicator,
+ batch_size => Size
Review comment:
Needs unit suffix `_bytes`.
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623169685
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -270,24 +282,31 @@ find_cluster_file([{Type, Location} | Rest]) ->
),
{ok, Location};
{ok, _} ->
+ ?LOG_ERROR(Msg#{
+ status => permissions_error,
+ details => "CouchDB needs read/write access to FDB cluster file"
+ }),
couch_log:error(
"CouchDB needs read/write access to FDB cluster file: ~s",
[Location]
),
{error, cluster_file_permissions};
{error, Reason} when Type =:= custom ->
+ ?LOG_ERROR(Msg#{status => Reason}),
couch_log:error(
"Encountered ~p error looking for FDB cluster file: ~s",
[Reason, Location]
),
{error, Reason};
{error, enoent} when Type =:= default ->
+ ?LOG_INFO(Msg#{status => enoent}),
Review comment:
adding `format_error` output as a `details` field.
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619501828
##########
File path: src/couch_replicator/src/couch_replicator_job.erl
##########
@@ -865,6 +948,11 @@ state_strip_creds(#rep_state{source = Source, target = Target} = State) ->
adjust_maxconn(Src = #{<<"http_connections">> := 1}, RepId) ->
+ ?LOG_NOTICE(#{
Review comment:
The original message logged RepId, should we include it in the logger report too?
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623066920
##########
File path: src/couch_replicator/src/couch_replicator_httpc.erl
##########
@@ -348,6 +355,14 @@ update_first_error_timestamp(HttpDb) ->
log_retry_error(Params, HttpDb, Wait, Error) ->
Method = string:to_upper(atom_to_list(get_value(method, Params, get))),
Url = couch_util:url_strip_password(full_url(HttpDb, Params)),
+ ?LOG_NOTICE(#{
+ what => retry_request,
+ in => replicator,
+ method => Method,
+ url => Url,
+ retry_delay => Wait / 1000,
Review comment:
Needs unit suffix
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623042001
##########
File path: src/couch/src/couch_proc_manager.erl
##########
@@ -321,7 +322,7 @@ find_proc(#client{lang = Lang, ddoc = DDoc, ddoc_key = DDocKey} = Client) ->
find_proc(Lang, Fun) ->
try iter_procs(Lang, Fun)
- catch ?STACKTRACE(error, Reason, StackTrace)
+ catch error:Reason:StackTrace ->
Review comment:
Is it intentional omission of
```
catch error:Reason:StackTrace ->
?LOG_ERROR(#{
what => process_lookup_failure,
result => error,
details => Reason,
stack => StackTrace
}),
```
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623070155
##########
File path: src/couch_replicator/src/couch_replicator_job.erl
##########
@@ -299,6 +348,12 @@ handle_info({'EXIT', Pid, normal}, #rep_state{workers = Workers} = State) ->
Workers ->
%% Processes might be linked by replicator's auth plugins so
%% we tolerate them exiting `normal` here and don't crash
+ ?LOG_WARNING(#{
+ what => unknown_process_exit,
Review comment:
Ambiguous meaning (see another similar comment earlier).
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623063517
##########
File path: src/couch_replicator/src/couch_replicator_changes_reader.erl
##########
@@ -60,14 +61,23 @@ read_changes(Parent, StartSeq, Db, ChangesQueue, Options) ->
N when N > 0 ->
put(retries_left, N - 1),
LastSeq = get(last_seq),
+ LogMsg = #{
+ what => retry_changes_feed,
+ in => replicator,
+ source => couch_replicator_api_wrap:db_uri(Db),
+ sequence => LastSeq,
+ retries_remaining => N
+ },
Db2 = case LastSeq of
StartSeq ->
+ ?LOG_NOTICE(LogMsg#{delay => Db#httpdb.wait / 1000}),
Review comment:
Needs unit suffix
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619541796
##########
File path: src/couch_replicator/src/couch_replicator_changes_reader.erl
##########
@@ -60,14 +61,23 @@ read_changes(Parent, StartSeq, Db, ChangesQueue, Options) ->
N when N > 0 ->
put(retries_left, N - 1),
LastSeq = get(last_seq),
+ LogMsg = #{
+ what => retry_changes_feed,
+ in => replicator,
+ source => couch_replicator_api_wrap:db_uri(Db),
+ sequence => LastSeq,
+ retries_remaining => N
+ },
Db2 = case LastSeq of
StartSeq ->
+ ?LOG_NOTICE(maps:merge(LogMsg, #{delay => Db#httpdb.wait / 1000})),
Review comment:
Done
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619518841
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -16,6 +16,7 @@
-include_lib("couch/include/couch_db.hrl").
-include_lib("chttpd/include/chttpd.hrl").
+-include_lib("kernel/include/logger.hrl").
Review comment:
Ah, I had gone back and forth on whether to include it in the header file(s) and just forgot to remove it. This is one topic I wanted to get feedback on. I ended up adding logger.hrl explicitly everywhere, but would it be better to embed in other headers? I don't have a strong preference.
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623062355
##########
File path: src/couch_replicator/src/couch_replicator_auth_session.erl
##########
@@ -170,11 +176,13 @@ handle_call(stop, _From, State) ->
handle_cast(Msg, State) ->
+ ?LOG_ERROR(#{what => unexpected_cast, in => replicator, msg => Msg}),
couch_log:error("~p: Received un-expected cast ~p", [?MODULE, Msg]),
{noreply, State}.
handle_info(Msg, State) ->
+ ?LOG_ERROR(#{what => unexpected_msg, in => replicator, msg => Msg}),
Review comment:
Nitpick.
It is better to be consistent the `unexpected_message` is used in `couch_js_os_process`.
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623066542
##########
File path: src/couch_replicator/src/couch_replicator_docs.erl
##########
@@ -154,9 +162,17 @@ update_rep_doc(RepDbName, RepDbUUID, RepDocId, KVs, Wait)
end
catch
throw:conflict ->
+ Delay = couch_rand:uniform(erlang:min(128, Wait)) * 100,
+ ?LOG_ERROR(#{
+ what => replication_doc_conflict,
+ in => replicator,
+ replication_db => RepDbName,
+ replication_doc => RepDocId,
+ retry_delay => Delay
Review comment:
Needs unit suffix.
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623060593
##########
File path: src/couch_replicator/src/couch_replicator_api_wrap.erl
##########
@@ -256,6 +270,13 @@ open_doc_revs(#httpdb{} = HttpDb, Id, Revs, Options, Fun, Acc) ->
),
#httpdb{retries = Retries, wait = Wait0} = HttpDb,
Wait = 2 * erlang:min(Wait0 * 2, ?MAX_WAIT),
+ ?LOG_NOTICE(#{
+ what => retry_request,
+ in => replicator,
+ url => Url,
+ delay => Wait / 1000,
Review comment:
needs unit suffix, one of:
- `_ms` for msec
- `_sec` for sec
- `_hr` for hours
- `_us` for microseconds
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623034170
##########
File path: src/chttpd/src/chttpd_sup.erl
##########
@@ -99,6 +102,13 @@ append_if_set({Key, Value}, Opts) when Value > 0 ->
append_if_set({_Key, 0}, Opts) ->
Opts;
append_if_set({Key, Value}, Opts) ->
+ ?LOG_ERROR(#{
+ what => invalid_config_setting,
+ section => chttpd_auth_cache,
+ key => Key,
+ value => Value,
+ details => "value must be a positive integer"
Review comment:
Message doesn't indicate whither 0 is allowed or not. In erlang specs there are two distinct types http://erlang.org/doc/reference_manual/typespec.html:
- non_neg_integer() - `0..`
- pos_integer() - `1..`
- neg_integer() - `..-1`
Which doesn't make things clearer IMO because the logs are read by users/operators not Erlang developers.
Maybe we should say something like
- `value must be a positive integer [0..`
- `value must be a positive integer 0..`
- `value must be a positive integer including 0`
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619500068
##########
File path: src/couch_replicator/src/couch_replicator_changes_reader.erl
##########
@@ -60,14 +61,23 @@ read_changes(Parent, StartSeq, Db, ChangesQueue, Options) ->
N when N > 0 ->
put(retries_left, N - 1),
LastSeq = get(last_seq),
+ LogMsg = #{
+ what => retry_changes_feed,
+ in => replicator,
+ source => couch_replicator_api_wrap:db_uri(Db),
+ sequence => LastSeq,
+ retries_remaining => N
+ },
Db2 = case LastSeq of
StartSeq ->
+ ?LOG_NOTICE(maps:merge(LogMsg, #{delay => Db#httpdb.wait / 1000})),
Review comment:
Could do `LogMsg#{delay => Db#httpdb.wait / 1000}` here too, it's a bit shorter. Same for other `maps:merge` calls
--
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 #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r620598792
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -268,6 +275,11 @@ handle_request_int(MochiReq) ->
span_ok(HttpResp),
{ok, Resp};
#httpd_resp{status = aborted, reason = Reason} ->
+ ?LOG_ERROR(#{
+ what => abnormal_response_termation,
+ id => get(nonce),
Review comment:
1) I think we can use `logger:set_process_metadata(#{http_request_id => Nonce})` once on the line after we calculated `Nonce` and do not pass it explicitly everywhere.
2) I think `id` name is too generic this would make correlation of log events on the external log querying system problematic.
--
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] kocolosk commented on a change in pull request #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623169398
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -254,14 +255,25 @@ find_cluster_file([{custom, undefined} | Rest]) ->
find_cluster_file(Rest);
find_cluster_file([{Type, Location} | Rest]) ->
+ Msg = #{
+ in => fdb_connection_setup,
Review comment:
I could see that too. I have a slight preference for `what => fdb_connection_setup`
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619575055
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -325,6 +349,12 @@ apply_tx_option(Db, Option, Val, binary) ->
true ->
set_option(Db, Option, BinVal);
false ->
+ ?LOG_ERROR(#{
+ what => invalid_transaction_option_value,
+ option => Option,
+ value => Val,
+ details => "string transaction option must be less than 16 bytes"
Review comment:
Good point. I think the check is wrong. I remember going by the other client's API docs (Python I think) https://apple.github.io/foundationdb/api-python.html#database-options and it said `up to 16 hexadecimal digits`. But we can fix it in a separate PR.
Out of curiosity, had actually disabled the check locally and set to a much higher value and nothing seems to have happened but it's better to just go by the documentation and stick to =< 16.
`fdbserver --help` also indicates the same:
```
/usr/sbin/fdbserver --help | grep -A2 machine_id
-i ID, --machine_id ID
Machine and zone identifier key (up to 16 hex characters).
Defaults to a random value shared by all fdbserver processes
```
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619522197
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -16,6 +16,7 @@
-include_lib("couch/include/couch_db.hrl").
-include_lib("chttpd/include/chttpd.hrl").
+-include_lib("kernel/include/logger.hrl").
Review comment:
Ah makes sense. Having it everywhere is better it seems -- it's more explicit
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r620297833
##########
File path: src/couch/src/couch_flags_config.erl
##########
@@ -132,10 +137,7 @@ parse_flags_term(FlagsBin) ->
[] ->
lists:usort(Flags);
_ ->
- {error, {
- "Cannot parse list of tags: ~n~p",
- Errors
- }}
+ {error, Errors}
Review comment:
Ah, yes, this was one area where I ended up refactoring things a bit as I was struggling with the pattern of carrying around a log formatting message as the return value through that whole parsing suite. Thanks for the heads up.
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623115773
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -270,24 +282,31 @@ find_cluster_file([{Type, Location} | Rest]) ->
),
{ok, Location};
{ok, _} ->
+ ?LOG_ERROR(Msg#{
+ status => permissions_error,
+ details => "CouchDB needs read/write access to FDB cluster file"
+ }),
couch_log:error(
"CouchDB needs read/write access to FDB cluster file: ~s",
[Location]
),
{error, cluster_file_permissions};
{error, Reason} when Type =:= custom ->
+ ?LOG_ERROR(Msg#{status => Reason}),
couch_log:error(
"Encountered ~p error looking for FDB cluster file: ~s",
[Reason, Location]
),
{error, Reason};
{error, enoent} when Type =:= default ->
+ ?LOG_INFO(Msg#{status => enoent}),
Review comment:
should we say `status => file_not_found` to be less cryptic for windows users?
--
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] nickva commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r619874015
##########
File path: src/couch/src/couch_flags_config.erl
##########
@@ -132,10 +137,7 @@ parse_flags_term(FlagsBin) ->
[] ->
lists:usort(Flags);
_ ->
- {error, {
- "Cannot parse list of tags: ~n~p",
- Errors
- }}
+ {error, Errors}
Review comment:
I ran the unit tests and noticed this change would need a corresponding change in the tests suite
```
in call from eunit_proc:run_group/2 (eunit_proc.erl, line 561)
**error:{assertEqual,[{module,couch_flags_config_tests},
{line,115},
{expression,"couch_flags_config : parse_flags_term ( LongBinary )"},
{expected,{error,{"Cannot parse list of tags: ~n~p",
[{too_long,<<"aaaaaaaaaaaaaaaa"...>>}]}}},
{value,{error,[{too_long,<<"aaaaaaaaaaaaaaaaaaaa"...>>}]}}]}
output:<<"">>
```
--
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] kocolosk commented on a change in pull request #3526: Adopt structured logging via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r621211342
##########
File path: src/chttpd/src/chttpd.erl
##########
@@ -16,6 +16,7 @@
-include_lib("couch/include/couch_db.hrl").
-include_lib("chttpd/include/chttpd.hrl").
+-include_lib("kernel/include/logger.hrl").
Review comment:
Removed nested header inclusion in 654baa7
--
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 #3526: Add structured logging reports via new Erlang 21 logger
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3526:
URL: https://github.com/apache/couchdb/pull/3526#discussion_r623107823
##########
File path: src/fabric/src/fabric2_server.erl
##########
@@ -254,14 +255,25 @@ find_cluster_file([{custom, undefined} | Rest]) ->
find_cluster_file(Rest);
find_cluster_file([{Type, Location} | Rest]) ->
+ Msg = #{
+ in => fdb_connection_setup,
Review comment:
Is the omission of `what` key in this module intentional?
--
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