basic execution statistics for _find #768

willholley · 2017-08-22T14:48:41Z

Overview

Accept an "execution_stats" parameter to _find. If present, return a new "execution_stats" object in the response which contains information about the query executed. Currently, this is only
implemented for json/all_docs indexes and contains:

total key lookups
total document lookups (when include_docs=true used)
total quorum document lookups (when fabric doc lookups used)

Testing recommendations

Query _find with different selectors. Add "execution_stats":true to the request body and see that an "execution_stats" field is returned. Test with different index types.

This PR adds new unit tests for the execution stats and existing behaviour is already well covered by tests.

GitHub issue number

Related Pull Requests

Checklist

Code is written and works correctly;
Changes are covered by tests;
Documentation reflects the changes;

tonysun83 · 2017-08-22T21:24:24Z

src/mango/src/mango_cursor_special.erl

@@ -38,7 +38,7 @@ create(Db, Indexes, Selector, Opts) ->
    Limit = couch_util:get_value(limit, Opts, mango_opts:default_limit()),
    Skip = couch_util:get_value(skip, Opts, 0),
    Fields = couch_util:get_value(fields, Opts, all_fields),
-    Bookmark = couch_util:get_value(bookmark, Opts), 
+    Bookmark = couch_util:get_value(bookmark, Opts),


did you intentionally make a whitespace change?

tonysun83 · 2017-08-22T21:24:33Z

src/mango/src/mango_cursor.erl

@@ -69,6 +69,7 @@ explain(#cursor{}=Cursor) ->
    } = Cursor,
    Mod = mango_idx:cursor_mod(Idx),
    Opts = lists:keydelete(user_ctx, 1, Opts0),
+


tonysun83 · 2017-08-22T22:27:57Z

src/mango/src/mango_execution_stats.hrl

+% License for the specific language governing permissions and limitations under
+% the License.
+
+-record(execution_stats, {


consider changing Lookup to Fetched? It's not a huge deal, but the word lookup seems indicate key range executions search execution vs rows returned.

@tonysun83 ideally the key lookup counter would be as you describe - it's just not implemented that way yet (code assumes key lookups == doc lookups until we have covering indexes). Note that the current counters don't say anything about the number of rows returned - just the number of internal row / document fetches.

@tonysun83 I've renamed these to xxExamined

tonysun83 · 2017-08-22T22:45:56Z

src/mango/src/mango_httpd.erl

@@ -266,6 +267,11 @@ run_find(Resp, Db, Sel, Opts) ->
    mango_crud:find(Db, Sel, fun handle_doc/2, Acc0, Opts).


+handle_doc({add_key, Key, Value = #execution_stats{}}, Acc0) ->


I think this should work without the need for the extra redundant code

handle_doc({add_key, Key, Value = #execution_stats{}}, Acc0) -> JSONValue = mango_execution_stats:to_json(Value), handle_doc({add_key, Key, JSONValue}, Acc0);

It's not clear to me which code is redundant here. The intent is to perform custom JSON serialisation for the execution_stats record - without that, the response is invalid (JSON serialisation fails).

I think what Tony is saying here is you don't need to do the lists:keystore in this function. Rather convert to json and then call handle_doc() again so that the function below will do all lists:keystore work.
I also think you could consider doing the to_json and in the cursor:execute sections. Then you don't need to add a whole new function.

tonysun83 · 2017-08-22T22:56:31Z

src/mango/test/15-execution-stats-test.py

+        assert resp["execution_stats"]["total_key_lookups"] == 3
+        assert resp["execution_stats"]["total_doc_lookups"] == 0
+        assert resp["execution_stats"]["total_quorum_doc_lookups"] == 3
+


no new end of line

tonysun83 · 2017-08-22T23:00:44Z

src/mango/src/mango_cursor_view.erl

-            case mango_selector:match(Cursor#cursor.selector, Doc) of
+    ExecutionStats0 = mango_execution_stats:incr_key_lookups(Cursor#cursor.execution_stats),
+    case doc_member(Cursor#cursor.db, Props, Cursor#cursor.opts, ExecutionStats0) of
+        {ok, Doc, execution_stats, ExecutionStats1} ->


consider changing the return value here to be:

{ok, Doc, {execution_stats, ExecutionStats1}};

The function doc_member/4 will have to be modified as well

tonysun83 · 2017-08-22T23:03:54Z

overall looks pretty good with some minor nitpicks.

garrensmith

This is looking awesome. Just some small changes.

garrensmith · 2017-08-24T10:04:40Z

src/mango/src/mango_cursor_text.erl

@@ -105,7 +107,8 @@ execute(Cursor, UserFun, UserAcc) ->
        query_args = QueryArgs,
        user_fun = UserFun,
        user_acc = UserAcc,
-        fields = Cursor#cursor.fields
+        fields = Cursor#cursor.fields,
+        execution_stats = mango_execution_stats:log_execution_start(Stats)


I think a bit of redundancy here with the double execution. Maybe something like mango_execution_stats:log_start. Based on the module name we know its the execution stats.

garrensmith · 2017-08-24T10:06:31Z

src/mango/src/mango_cursor_text.erl

@@ -119,7 +122,7 @@ execute(Cursor, UserFun, UserAcc) ->
            JsonBM = dreyfus_bookmark:pack(FinalBM),
            Arg = {add_key, bookmark, JsonBM},
            {_Go, FinalUserAcc} = UserFun(Arg, LastUserAcc),
-            {ok, FinalUserAcc}
+            mango_execution_stats:maybe_add_execution_stats(FinalCAcc, FinalUserAcc)


I would just do mango_execution_stats:maybe_add_stats

garrensmith · 2017-08-24T10:10:02Z

src/mango/src/mango_cursor_view.erl

    Cursor = Cursor0#cursor{
        user_fun = UserFun,
-        user_acc = UserAcc
+        user_acc = UserAcc,
+        execution_stats = mango_execution_stats:log_execution_start(Stats)


This is minor. But I wonder if this shouldn't be done in the create function rather than execute.

garrensmith · 2017-08-24T10:19:20Z

src/mango/src/mango_httpd.erl

@@ -266,6 +267,11 @@ run_find(Resp, Db, Sel, Opts) ->
    mango_crud:find(Db, Sel, fun handle_doc/2, Acc0, Opts).


+handle_doc({add_key, Key, Value = #execution_stats{}}, Acc0) ->


I think what Tony is saying here is you don't need to do the lists:keystore in this function. Rather convert to json and then call handle_doc() again so that the function below will do all lists:keystore work.
I also think you could consider doing the to_json and in the cursor:execute sections. Then you don't need to add a whole new function.

garrensmith · 2017-08-24T10:19:38Z

src/mango/test/15-execution-stats-test.py

@@ -0,0 +1,59 @@
+# -*- coding: latin-1 -*-


I don't think this is needed.

Accept an "execution_stats" parameter to _find. If present, return a new "execution_stats" object in the response which contains information about the query executed. Currently, this is only implemented for json/all_docs indexes and contains: - total keys examined (currently always 0 for json indexes) - total documents examined (when include_docs=true used) - total quorum documents examined (when fabric doc lookups used)

Include execution time in mango execution stats. Only implemented for views.

Accept an "execution_stats" parameter to _find. If present, return a new "execution_stats" object in the response which contains information about the query executed. Currently, this is only implemented for json/all_docs indexes and contains: - total keys examined (currently always 0 for json indexes) - total documents examined (when include_docs=true used) - total quorum documents examined (when fabric doc lookups used)

willholley force-pushed the mango_execution_stats branch from 2667910 to b9df55b Compare August 22, 2017 14:52

tonysun83 reviewed Aug 22, 2017

View reviewed changes

willholley force-pushed the mango_execution_stats branch 3 times, most recently from 5d77c9f to 2843c72 Compare August 23, 2017 14:42

garrensmith requested changes Aug 24, 2017

View reviewed changes

willholley force-pushed the mango_execution_stats branch from 2843c72 to 2b0cbbe Compare August 24, 2017 14:09

willholley added 2 commits August 24, 2017 16:28

Add execution time to mango execution stats

9dccc3b

Include execution time in mango execution stats. Only implemented for views.

willholley force-pushed the mango_execution_stats branch 2 times, most recently from f9d8fcf to e6ac226 Compare August 24, 2017 16:41

Add execution stats for text indexes

95d5a61

willholley force-pushed the mango_execution_stats branch from e6ac226 to 95d5a61 Compare August 25, 2017 12:29

migrate tests to use unittest assertions

71faeab

garrensmith approved these changes Aug 30, 2017

View reviewed changes

garrensmith merged commit af839e1 into apache:master Aug 30, 2017

willholley mentioned this pull request Aug 30, 2017

API documentation for mango execution stats apache/couchdb-documentation#160

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic execution statistics for _find #768

basic execution statistics for _find #768

willholley commented Aug 22, 2017

tonysun83 Aug 22, 2017

tonysun83 Aug 22, 2017

tonysun83 Aug 22, 2017 •

edited

Loading

willholley Aug 23, 2017

willholley Aug 23, 2017

tonysun83 Aug 22, 2017

willholley Aug 23, 2017

garrensmith Aug 24, 2017

tonysun83 Aug 22, 2017

tonysun83 Aug 22, 2017 •

edited

Loading

tonysun83 commented Aug 22, 2017

garrensmith left a comment

garrensmith Aug 24, 2017

garrensmith Aug 24, 2017

garrensmith Aug 24, 2017

garrensmith Aug 24, 2017

garrensmith Aug 24, 2017

		@@ -266,6 +267,11 @@ run_find(Resp, Db, Sel, Opts) ->
		mango_crud:find(Db, Sel, fun handle_doc/2, Acc0, Opts).


		handle_doc({add_key, Key, Value = #execution_stats{}}, Acc0) ->

basic execution statistics for _find #768

basic execution statistics for _find #768

Conversation

willholley commented Aug 22, 2017

Overview

Testing recommendations

GitHub issue number

Related Pull Requests

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonysun83 Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonysun83 Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

tonysun83 commented Aug 22, 2017

garrensmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonysun83 Aug 22, 2017 •

edited

Loading

tonysun83 Aug 22, 2017 •

edited

Loading