Skip to content

Commit

Permalink
fix(explain-plan): migrate to TS/away from Ampersand, fix sharding wa…
Browse files Browse the repository at this point in the history
…rning bug (#2494)

* chore(explain-plan): drop serverVersion from state

This does not have any actual impact on the UI.

* feat(explain-plan): replace explain-plan ampersand model

Replace the explain-plan-model Ampersand model and replace
it with a typescript helper package.

This new package matches the model broadly, except in a
few aspects:

- It automatically includes the SBE compatibiltiy helper,
  removing the need to apply it explicitly in compass-explain-plan.
- The “raw” original data property is exposed directly, not
  attached via monkey-patching in compass-explain-plan.
- `model.serialize()` is gone, the compass-explain-plan
  package now manually unpacks the properties it uses.
  This may not look very pretty, but at least it doesn’t
  hide the fact that we are using the object as an untyped
  bag of properties anymore.
- The child stage iterator helper is exposed, allowing some
  code redundancy between the packages to be removed.
- Support for pre-3.0 server explain plans is dropped.

* chore(explain-plan): replace usedIndex field to make it more meaningful

Replace the `usedIndex` field, which had an odd typing to begin
with, with a `usedIndexes` field that reflects all indices used
and what shards they were used on in a consistent way that matches
its actual usage.

* fix(explain-plan): do not display shard warning when inappropriate COMPASS-4770

This warning is supposed to tell people about situations in which
different shards used difference indices in their explain plan,
which is fine to warn about.

However, the check was fairly naïve and only warned whether multiple
indices were used at all. That can lead to false positives, since
using multiple indices by itself is not problematic.

Address this by grouping the used indices by the shards they were
run on, and seeing if these structures actually differ between
the individual shards.
  • Loading branch information
addaleax authored Oct 4, 2021
1 parent debc3b1 commit 5f78690
Show file tree
Hide file tree
Showing 57 changed files with 3,839 additions and 5,945 deletions.
2,434 changes: 793 additions & 1,641 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions packages/compass-explain-plan/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"@mongodb-js/compass-query-bar": "^7.13.0",
"@mongodb-js/compass-query-history": "^8.14.0",
"@mongodb-js/compass-status": "^5.14.0",
"@mongodb-js/explain-plan-helper": "^0.1.0",
"autoprefixer": "^9.7.0",
"babel-loader": "^8.2.2",
"chai": "^4.2.0",
Expand Down Expand Up @@ -119,8 +120,6 @@
"mongodb-client-encryption": "^1.2.3",
"mongodb-connection-model": "^21.8.0",
"mongodb-data-service": "^21.12.0",
"mongodb-explain-compat": "^2.4.0",
"mongodb-explain-plan-model": "^1.3.0",
"mongodb-query-parser": "^2.4.3",
"mongodb-reflux-store": "^0.0.1",
"node-loader": "^0.6.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ class ExplainBody extends Component {
indexType: PropTypes.oneOf(INDEX_TYPES).isRequired,
index: PropTypes.object,
viewType: PropTypes.string.isRequired,
rawExplainObject: PropTypes.object.isRequired
rawExplainObject: PropTypes.object.isRequired,
originalExplainData: PropTypes.object.isRequired
}),
openLink: PropTypes.func.isRequired,
treeStages: PropTypes.object.isRequired
Expand Down Expand Up @@ -60,7 +61,7 @@ class ExplainBody extends Component {
renderDetailsView() {
if (this.props.explain.viewType === EXPLAIN_VIEWS.json) {
return (
<ExplainJSON rawExplainObject={this.props.explain.rawExplainObject} />
<ExplainJSON originalExplainData={this.props.explain.originalExplainData} />
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ class ExplainJSON extends Component {
static displayName = 'ExplainJSONComponent';

static propTypes = {
rawExplainObject: PropTypes.object.isRequired
originalExplainData: PropTypes.object.isRequired
}

copyToClipboard = () => {
clipboard.writeText(JSON.stringify(this.props.rawExplainObject.originalData));
clipboard.writeText(JSON.stringify(this.props.originalExplainData));
}

/**
Expand All @@ -26,7 +26,7 @@ class ExplainJSON extends Component {
* @returns {React.Component} The rendered component.
*/
render() {
const doc = new HadronDocument(this.props.rawExplainObject.originalData);
const doc = new HadronDocument(this.props.originalExplainData);

return (
<div className={styles['explain-json']}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import styles from './explain-json.module.less';

describe('ExplainJSON [Component]', () => {
let component;
const rawExplainObject = { originalData: {} };
const originalExplainData = {};
const appRegistry = new AppRegistry();

beforeEach(() => {
component = mount(
<ExplainJSON rawExplainObject={rawExplainObject} />
<ExplainJSON rawExplainObject={originalExplainData} />
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class ExplainPlan extends Component {
const mapStateToProps = (state) => pick(
state,
[
'serverVersion',
'namespace',
'isEditable',
'explain',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class ExplainStates extends Component {
index: PropTypes.object,
viewType: PropTypes.string.isRequired,
rawExplainObject: PropTypes.object.isRequired,
originalExplainData: PropTypes.object.isRequired,
explainState: PropTypes.string.isRequired,
error: PropTypes.object,
resultId: PropTypes.number.isRequired
Expand Down
56 changes: 39 additions & 17 deletions packages/compass-explain-plan/src/modules/explain.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import ExplainPlanModel from 'mongodb-explain-plan-model';
import { defaults, isString, find } from 'lodash';
import { ExplainPlan } from '@mongodb-js/explain-plan-helper';
import { isString, find, groupBy, isEqual } from 'lodash';
import { treeStagesChanged } from './tree-stages';
import { globalAppRegistryEmit } from '@mongodb-js/mongodb-redux-common/app-registry';
import convertExplainCompat from 'mongodb-explain-compat';

import EXPLAIN_STATES from '../constants/explain-states';
import EXPLAIN_VIEWS from '../constants/explain-views';
Expand Down Expand Up @@ -54,9 +53,10 @@ export const INITIAL_STATE = {
numShards: 0,
parsedQuery: {},
rawExplainObject: {},
originalExplainData: {},
totalDocsExamined: 0,
totalKeysExamined: 0,
usedIndex: null,
usedIndexes: [],
resultId: resultId()
};

Expand Down Expand Up @@ -182,9 +182,19 @@ const getIndexType = (explainPlan) => {
if (!explainPlan) {
return 'UNAVAILABLE';
}
if (Array.isArray(explainPlan.usedIndex)) {
return 'MULTIPLE';

const indexInfoByShard =
groupBy(explainPlan.usedIndexes, 'shard');
const indexNamesForAllShards =
Object.values(indexInfoByShard).map(entries => entries.map(({ index }) => index));
for (let i = 0; i < indexNamesForAllShards.length; i++) {
for (let j = i + 1; j < indexNamesForAllShards.length; j++) {
if (!isEqual(indexNamesForAllShards[i], indexNamesForAllShards[j])) {
return 'MULTIPLE'; // As in, multiple index setups that differ between shards
}
}
}

if (explainPlan.isCollectionScan) {
return 'COLLSCAN';
}
Expand All @@ -200,14 +210,27 @@ const getIndexType = (explainPlan) => {
* Parses the explain plan.
*
* @param {Object} explain - The current explain plan state.
* @param {Object} data - The new explain plan received from dataService.
* @param {ExplainPlan} data - The new explain plan received from dataService.
*
* @returns {Object} The parsed explain plan.
*/
const parseExplainPlan = (explain, data) => {
const explainPlanModel = new ExplainPlanModel(data);

return defaults(explainPlanModel.serialize(), explain);
const explainPlanModel = new ExplainPlan(data);

const {
namespace, parsedQuery, executionSuccess, nReturned, executionTimeMillis,
totalKeysExamined, totalDocsExamined, rawExplainObject, originalExplainData,
usedIndexes, isCovered, isMultiKey, inMemorySort, isCollectionScan,
isSharded, numShards
} = explainPlanModel;

return {
...explain,
namespace, parsedQuery, executionSuccess, nReturned, executionTimeMillis,
totalKeysExamined, totalDocsExamined, rawExplainObject, originalExplainData,
usedIndexes, isCovered, isMultiKey, inMemorySort, isCollectionScan,
isSharded, numShards
};
};

/**
Expand All @@ -222,8 +245,8 @@ const parseExplainPlan = (explain, data) => {
const updateWithIndexesInfo = (explain, indexes) => ({
...explain,
indexType: getIndexType(explain),
index: isString(explain.usedIndex)
? find(indexes, (idx) => (idx.name === explain.usedIndex))
index: explain.usedIndexes.length > 0 && typeof explain.usedIndexes[0].index === 'string'
? find(indexes, (idx) => (idx.name === explain.usedIndexes[0].index))
: null
});

Expand Down Expand Up @@ -281,23 +304,22 @@ export const fetchExplainPlan = (query) => {
// so we return here before parsing more, and ensure we can show
// the json view of the explain plan.
explain.errorParsing = true;
explain.rawExplainObject = { originalData: data };
explain.originalExplainData = data;
explain.resultId = resultId();

return dispatch(explainPlanFetched(explain));
}

try {
explain = parseExplainPlan(explain, convertExplainCompat(data));
explain = parseExplainPlan(explain, data);
} catch (e) {
explain.errorParsing = true;
explain.rawExplainObject = { originalData: data };
explain.originalExplainData = data;
explain.resultId = resultId();

return dispatch(explainPlanFetched(explain));
}
explain = updateWithIndexesInfo(explain, indexes);
explain.rawExplainObject.originalData = data;
explain.resultId = resultId();

dispatch(explainPlanFetched(explain));
Expand All @@ -320,7 +342,7 @@ export const fetchExplainPlan = (query) => {
numberOfShards: explain.numShards,
totalDocsExamined: explain.totalDocsExamined,
totalKeysExamined: explain.totalKeysExamined,
indexUsed: explain.usedIndex
usedIndexes: explain.usedIndexes
}
));

Expand Down
6 changes: 4 additions & 2 deletions packages/compass-explain-plan/src/modules/explain.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ const explainExample = {
numShards: 0,
parsedQuery: {},
rawExplainObject: {},
originalExplainData: {},
totalDocsExamined: 18801,
totalKeysExamined: 0,
usedIndex: null,
usedIndexes: [],
viewType: 'tree'
};

Expand Down Expand Up @@ -141,9 +142,10 @@ describe('explain module', () => {
numShards: 0,
parsedQuery: {},
rawExplainObject: {},
originalExplainData: {},
totalDocsExamined: 0,
totalKeysExamined: 0,
usedIndex: null
usedIndexes: []
});
});
});
Expand Down
3 changes: 0 additions & 3 deletions packages/compass-explain-plan/src/modules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import appRegistry, {
} from '@mongodb-js/mongodb-redux-common/app-registry';
import dataService, { INITIAL_STATE as DS_STATE } from './data-service';
import namespace, { INITIAL_STATE as NAMESPACE_STATE, NAMESPACE_CHANGED } from './namespace';
import serverVersion, { INITIAL_STATE as SV_STATE } from './server-version';
import isEditable, { INITIAL_STATE as IS_EDITABLE_STATE } from './edit-mode';
import explain, { INITIAL_STATE as EXPLAIN_STATE } from './explain';
import indexes, { INITIAL_STATE as INDEXES_STATE } from './indexes';
Expand All @@ -24,7 +23,6 @@ export const INITIAL_STATE = {
appRegistry: APP_REGISTRY_STATE,
dataService: DS_STATE,
namespace: NAMESPACE_STATE,
serverVersion: SV_STATE,
isEditable: IS_EDITABLE_STATE,
explain: EXPLAIN_STATE,
indexes: INDEXES_STATE,
Expand All @@ -39,7 +37,6 @@ const appReducer = combineReducers({
appRegistry,
dataService,
namespace,
serverVersion,
isEditable,
explain,
indexes,
Expand Down
42 changes: 0 additions & 42 deletions packages/compass-explain-plan/src/modules/server-version.js

This file was deleted.

29 changes: 0 additions & 29 deletions packages/compass-explain-plan/src/modules/server-version.spec.js

This file was deleted.

22 changes: 4 additions & 18 deletions packages/compass-explain-plan/src/modules/tree-stages.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { has, omit, map, isPlainObject, keys, max, maxBy, minBy, forEach } from 'lodash';
import { ExplainPlan } from '@mongodb-js/explain-plan-helper';
import { has, omit, map, keys, max, maxBy, minBy, forEach } from 'lodash';
import d3 from 'd3';
import STAGE_CARD_PROPERTIES from '../constants/stage-card-properties';
import { format } from 'util';
Expand Down Expand Up @@ -82,28 +83,13 @@ const parseExplain = (parent, obj) => {
y: obj.y,
depth: obj.depth,
isShard: !!obj.shardName,
parent
parent,
children: [...ExplainPlan.getChildStages(obj)].map(child => parseExplain(parent, child))
};

// Extract highlights relevant for the current stage from details
stage.highlights = extractHighlights(stage);

// Recursively parse child or children of this stage
const children = (
obj.inputStage ||
obj.inputStages ||
obj.shards ||
obj.executionStages
);

if (Array.isArray(children)) {
stage.children = map(children, parseExplain.bind(this, stage));
} else if (isPlainObject(children)) {
stage.children = [parseExplain(stage, children)];
} else {
stage.children = [];
}

return stage;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const explainExample = {
rawExplainObject: {},
totalDocsExamined: 18801,
totalKeysExamined: 0,
usedIndex: null,
usedIndexes: [],
viewType: 'tree'
};

Expand Down
Loading

0 comments on commit 5f78690

Please sign in to comment.