You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "pgj (via GitHub)" <gi...@apache.org> on 2023/05/03 23:07:36 UTC
[GitHub] [couchdb] pgj opened a new pull request, #4569: feat(`mango`): add `keys_examined` for `execution_stats`
pgj opened a new pull request, #4569:
URL: https://github.com/apache/couchdb/pull/4569
Add another field to the shard-level Mango execution statistics to keep track of the count of keys that were examined for the query. Note that this requires to change the way how stats are stored -- an approach similar to that of the view callback arguments was chosen, which features a map (#4394).
The change in the format also implies a potential incompatibility between different version of nodes, which may happen for mixed-version clusters. Introduce a new configuration option so that users could control the common known version of the message format in use between the nodes to avoid problems on upgrades.
This is related to the covering indexes work and takes inspiration from earlier works of @mikerhodes , see #4410 and #4413 for the details.
Note that slight increase in unit test coverage for `mango_execution_stats` due to the implicit testing via `mango_cursor_view`. The only missing part is `incr_docs_examined/1` but that is not used from there, only from `mango_cursor_text` and `mango_cursor_nouveau` .
```console
$ make eunit apps=mango
```
before:
```shell
[..]
Code Coverage:
[..]
mango_cursor_view : 100%
[..]
mango_execution_stats : 63%
[..]
Total : 46%
```
after:
```shell
[..]
Code Coverage:
[..]
mango_cursor_view : 100%
[..]
mango_execution_stats : 94%
[..]
Total : 47%
```
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] Documentation changes were made in the `src/docs` folder
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1190229695
##########
src/mango/src/mango_execution_stats.erl:
##########
@@ -106,3 +118,34 @@ log_stats(Stats) ->
Nonce = list_to_binary(couch_log_util:get_msg_id()),
MStats1 = MStats0#{nonce => Nonce},
couch_log:report("mango-stats", MStats1).
+
+-spec shard_init() -> any().
+shard_init() ->
+ put(?SHARD_STATS_KEY, #shard_stats{}).
+
+-spec shard_incr_keys_examined() -> any().
+shard_incr_keys_examined() ->
+ incr(#shard_stats.keys_examined).
+
+-spec shard_incr_docs_examined() -> any().
+shard_incr_docs_examined() ->
+ incr(#shard_stats.docs_examined).
+
+-spec incr(atom()) -> any().
+incr(Key) ->
+ case get(?SHARD_STATS_KEY) of
+ #shard_stats{} = Stats0 ->
Review Comment:
Consider using a map here as well otherwise we're switching from a map to record and have to do the awkward setelement
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] chewbranca commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
chewbranca commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1190202442
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -379,14 +395,22 @@ view_cb({row, Row}, #mrargs{extra = Options} = Acc) ->
ok = rexi:stream2(ViewRow),
set_mango_msg_timestamp();
{Doc, _} ->
- put(mango_docs_examined, get(mango_docs_examined) + 1),
+ mango_execution_stats:shard_incr_docs_examined(),
couch_stats:increment_counter([mango, docs_examined]),
Process(Doc)
end,
{ok, Acc};
-view_cb(complete, Acc) ->
+view_cb(complete, #mrargs{extra = Options} = Acc) ->
+ ShardStats = mango_execution_stats:shard_get_stats(),
+ case couch_util:get_value(execution_stats_map, Options, false) of
+ true ->
+ Stats = ShardStats;
+ false ->
+ DocsExamined = maps:get(docs_examined, ShardStats),
+ Stats = {docs_examined, DocsExamined}
+ end,
Review Comment:
We should do the assignment to the stats variable outside of the case statement, I believe the compiler generates an "assignment from nested block" warning or some such here as well.
```erlang
Stats = case couch_util:get_value(execution_stats_map, Options, false) of
true ->
ShardStats;
false ->
DocsExamined = maps:get(docs_examined, ShardStats),
{docs_examined, DocsExamined}
end,
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1186234859
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -384,9 +409,15 @@ view_cb({row, Row}, #mrargs{extra = Options} = Acc) ->
Process(Doc)
end,
{ok, Acc};
-view_cb(complete, Acc) ->
+view_cb(complete, #mrargs{extra = Options} = Acc) ->
+ case couch_util:get_value(execution_stats_map, Options) of
Review Comment:
It look a bit cleaner to have a default `false` value and then explicitly match on `true` or `false`. What do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] pgj commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1190214351
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -379,14 +395,22 @@ view_cb({row, Row}, #mrargs{extra = Options} = Acc) ->
ok = rexi:stream2(ViewRow),
set_mango_msg_timestamp();
{Doc, _} ->
- put(mango_docs_examined, get(mango_docs_examined) + 1),
+ mango_execution_stats:shard_incr_docs_examined(),
couch_stats:increment_counter([mango, docs_examined]),
Process(Doc)
end,
{ok, Acc};
-view_cb(complete, Acc) ->
+view_cb(complete, #mrargs{extra = Options} = Acc) ->
+ ShardStats = mango_execution_stats:shard_get_stats(),
+ case couch_util:get_value(execution_stats_map, Options, false) of
+ true ->
+ Stats = ShardStats;
+ false ->
+ DocsExamined = maps:get(docs_examined, ShardStats),
+ Stats = {docs_examined, DocsExamined}
+ end,
Review Comment:
You are right, and it is completely logical. I did not see any warnings from the compiler related to that, 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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] pgj commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1190284962
##########
src/mango/src/mango_execution_stats.erl:
##########
@@ -106,3 +118,34 @@ log_stats(Stats) ->
Nonce = list_to_binary(couch_log_util:get_msg_id()),
MStats1 = MStats0#{nonce => Nonce},
couch_log:report("mango-stats", MStats1).
+
+-spec shard_init() -> any().
+shard_init() ->
+ put(?SHARD_STATS_KEY, #shard_stats{}).
+
+-spec shard_incr_keys_examined() -> any().
+shard_incr_keys_examined() ->
+ incr(#shard_stats.keys_examined).
+
+-spec shard_incr_docs_examined() -> any().
+shard_incr_docs_examined() ->
+ incr(#shard_stats.docs_examined).
+
+-spec incr(atom()) -> any().
+incr(Key) ->
+ case get(?SHARD_STATS_KEY) of
+ #shard_stats{} = Stats0 ->
Review Comment:
I was also thinking about using maps but I eventually decided not to because their advantages are not fully exploited in this situation. But I agree with the awkwardness. And if it does not have a bad impact on performance or performance is not a concern here, I can change this.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] pgj commented on pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#issuecomment-1540577732
Please do not merge this now, I would like to do some refactoring (move the process dictionary function under `mango_execution_stats`) per @chewbranca's recommendations.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] chewbranca merged pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "chewbranca (via GitHub)" <gi...@apache.org>.
chewbranca merged PR #4569:
URL: https://github.com/apache/couchdb/pull/4569
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1186238890
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -187,7 +206,10 @@ base_args(#cursor{index = Idx, selector = Selector, fields = Fields} = Cursor) -
{selector, Selector},
{callback_args, viewcbargs_new(Selector, Fields, undefined)},
- {ignore_partition_query_limit, true}
+ {ignore_partition_query_limit, true},
+
+ % Request execution statistics in a map.
Review Comment:
At some point we could presumably remove this. Should we add a TODO or some kind of version marker to help make it easier to clean it up. https://github.com/apache/couchdb/issues/4396
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] pgj commented on pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#issuecomment-1542386990
OK -- the planned changes are 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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a diff in pull request #4569: feat(`mango`): add `keys_examined` for `execution_stats`
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #4569:
URL: https://github.com/apache/couchdb/pull/4569#discussion_r1186259726
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -384,9 +409,15 @@ view_cb({row, Row}, #mrargs{extra = Options} = Acc) ->
Process(Doc)
end,
{ok, Acc};
-view_cb(complete, Acc) ->
+view_cb(complete, #mrargs{extra = Options} = Acc) ->
+ case couch_util:get_value(execution_stats_map, Options) of
Review Comment:
we should be 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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org