feat: add explain support for non-cursor commands #2599

HanaPearlman · 2020-10-27T21:42:17Z

Description

This PR implements explain functionality for the relevant non-cursor operations: update, remove, distinct, findAndModify, and mapReduce. These operations now accept an additional explain option, either a boolean or a string specifying the requested verbosity, and will be run as explain commands with the specified verbosity.

Current syntax: collection.updateOne(..., {explain: verbosity}).
Other syntax, e.g.: collection.explain(verbosity).updateOne(...) is easy to add (or switch to) if we want.

What changed?

A new Explainable aspect was added so that, in CommandOperation, operations with that aspect can use the explain property already on the class. Logic for validating the explain verbosity passed through the options, adding the explain field to the command, and canRetryWrite have been added to CommandOperation.

Not all explainable commands actually call executeCommand, including updateOne, deleteOne, etc. These are handled in the corresponding way: the specified explain verbosity is passed through the options until the command document is created, and then the explain is added to it.

Important note: adding an explain to a command alters its shape significantly. For this reason, it should happen at the end of the command building process. This has led to some weirdness where write operations that are being explained still seem like they are doing writes (still call executeWriteOperation in server.ts). I couldn't see a workaround for this that avoided repeating lots of command building logic. Open to any other suggestions!

NODE-2852

emadum

LGTM with some nits 👍

emadum · 2020-11-04T16:15:42Z

src/explain.ts

+  }
+
+  /** Checks that the server supports explain on the given command.*/
+  static explainSupportedOnCmd(server: Server, cmd: Document): boolean {


Could we overload explainSupported to handle either an op: string or a cmd: Document, instead of having two separate methods that are pretty similar in implementation?

emadum · 2020-11-04T16:17:51Z

src/operations/command.ts

+    if (cmd.explain) {
+      cmd = decorateWithExplain(cmd, cmd.explain);
+    }
+
    server.command(
      this.ns.toString(),
      cmd,


This is a stylistic nit, but I think it'd be better to do cmd.explain ? decorateWithExplain(cmd, cmd.explain) : cmd here rather than reassigning the cmd parameter.

emadum · 2020-11-04T16:31:13Z

src/utils.ts

+    delete command.explain;
+  }
+
+  command = { explain: command, verbosity: explain.verbosity };


I'd just return this object literal directly rather than reassigning command.

src/explain.ts

src/operations/explainable_command.ts

src/cursor/cursor.ts

src/explain.ts

nbbeeken

lgtm!

src/operations/explainable_command.ts

emadum

A few extra nitpicks.

emadum · 2020-11-12T13:02:53Z

src/explain.ts

+import type { Server } from './sdam/server';
+import { maxWireVersion } from './utils';
+
+export const Verbosity = {


Nit: can we expand Verbosity to ExplainVerbosity for clarity?

emadum · 2020-11-12T13:08:48Z

test/functional/explain.test.js

+    return setupDatabase(this.configuration);
+  });
+
+  it('shouldHonorBooleanExplainWithDeleteOne', {


Nit: test names for new tests should be written in normal text with spaces rather than camel case.

emadum · 2020-11-12T16:02:08Z

src/index.ts

@@ -163,6 +163,11 @@ export type {
 export type { DbPrivate, DbOptions } from './db';
 export type { AutoEncryptionOptions, AutoEncryptionLoggerLevels, AutoEncrypter } from './deps';
 export type { AnyError, ErrorDescription } from './error';
+export type {
+  ExplainOptions,
+  ExplainVerbosity as Verbosity,


Suggested change

ExplainVerbosity as Verbosity,

ExplainVerbosity,

Oops, not sure how that happened!

emadum · 2020-11-12T16:02:19Z

src/index.ts

+export type {
+  ExplainOptions,
+  ExplainVerbosity as Verbosity,
+  ExplainVerbosityLike as VerbosityLike


Suggested change

ExplainVerbosityLike as VerbosityLike

ExplainVerbosityLike

nbbeeken

lgtm

mbroadst

LGTM!

HanaPearlman changed the title ~~Node 2852/master/explain non cursor~~ feat: add explain support for non-cursor commands Oct 28, 2020

HanaPearlman requested a review from emadum October 28, 2020 19:36

HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch 5 times, most recently from 58ef4a2 to 35d1814 Compare November 2, 2020 18:59

HanaPearlman added 12 commits November 2, 2020 15:29

FindOneOperation extends CommandOperation

a8dfabe

implement explain for write commands

5bded70

implement explain for distinct command

4374b6b

implement explain for findAndModify commands

f00fbfe

implement explain for mapReduce command

1d937a8

first attempt: handle removing sessions for write operations

2a9b1be

consider explain during canRetryWrite

237a70f

small enum cleanup

c2d0a53

model explain after read concern

0126381

create ExplainableCommand class

82715bd

check explain value in explain command constructor

3bf0d78

quick cursor fix

7090c39

HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch from d4a3fb2 to 7090c39 Compare November 2, 2020 20:31

move explain cmd/options to separate file

82f0e60

HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch from 7f19744 to 82f0e60 Compare November 2, 2020 22:38

some commenting

f5e65ed

HanaPearlman marked this pull request as ready for review November 3, 2020 15:01

HanaPearlman requested review from mbroadst and nbbeeken November 3, 2020 17:15

emadum approved these changes Nov 4, 2020

View reviewed changes

nbbeeken requested changes Nov 4, 2020

View reviewed changes

src/explain.ts Outdated Show resolved Hide resolved

src/explain.ts Outdated Show resolved Hide resolved

src/operations/explainable_command.ts Outdated Show resolved Hide resolved

src/cursor/cursor.ts Outdated Show resolved Hide resolved

src/explain.ts Outdated Show resolved Hide resolved

respond to comments

4bdf503

HanaPearlman requested a review from nbbeeken November 5, 2020 18:09

HanaPearlman added 2 commits November 9, 2020 09:16

test bug fix

03333d7

Merge branch 'master' into NODE-2852/master/explain-non-cursor

20ef5da

nbbeeken approved these changes Nov 9, 2020

View reviewed changes

add explain-related exports to index

c1a4ff9

nbbeeken requested changes Nov 11, 2020

View reviewed changes

src/operations/explainable_command.ts Outdated Show resolved Hide resolved

src/operations/explainable_command.ts Outdated Show resolved Hide resolved

emadum suggested changes Nov 12, 2020

View reviewed changes

HanaPearlman added 5 commits November 12, 2020 09:18

use aspects and throw from fromOptions

2c7334b

use expanded explain types for clarity

d486214

change test names and ordering

ddc9826

clean up

6a17656

Merge branch 'master' into NODE-2852/master/explain-non-cursor

7235fb7

HanaPearlman requested review from emadum and nbbeeken November 12, 2020 15:14

emadum suggested changes Nov 12, 2020

View reviewed changes

fix explain export in index

001ec2e

HanaPearlman requested a review from emadum November 12, 2020 17:13

nbbeeken approved these changes Nov 12, 2020

View reviewed changes

HanaPearlman mentioned this pull request Nov 12, 2020

[Draft] feat: add explain support for cursor commands HanaPearlman/node-mongodb-native#1

Closed

emadum approved these changes Nov 12, 2020

View reviewed changes

check explain supported in individual op classes

0b37761

HanaPearlman force-pushed the NODE-2852/master/explain-non-cursor branch from 8c5ace7 to 0b37761 Compare November 13, 2020 21:03

Merge branch 'master' into NODE-2852/master/explain-non-cursor

31bb9eb

mbroadst approved these changes Nov 13, 2020

View reviewed changes

HanaPearlman merged commit 4472308 into mongodb:master Nov 16, 2020

HanaPearlman deleted the NODE-2852/master/explain-non-cursor branch November 16, 2020 14:02

HanaPearlman mentioned this pull request Nov 16, 2020

feat: add explain support for cursor commands #2622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add explain support for non-cursor commands #2599

feat: add explain support for non-cursor commands #2599

HanaPearlman commented Oct 27, 2020 •

edited

Loading

emadum left a comment

emadum Nov 4, 2020

emadum Nov 4, 2020

emadum Nov 4, 2020

nbbeeken left a comment

emadum left a comment

emadum Nov 12, 2020

emadum Nov 12, 2020

emadum Nov 12, 2020

HanaPearlman Nov 12, 2020

emadum Nov 12, 2020

nbbeeken left a comment

mbroadst left a comment

feat: add explain support for non-cursor commands #2599

feat: add explain support for non-cursor commands #2599

Conversation

HanaPearlman commented Oct 27, 2020 • edited Loading

Description

What changed?

emadum 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

nbbeeken left a comment

Choose a reason for hiding this comment

emadum 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

nbbeeken left a comment

Choose a reason for hiding this comment

mbroadst left a comment

Choose a reason for hiding this comment

HanaPearlman commented Oct 27, 2020 •

edited

Loading