Skip to content

Conversation

apodelko
Copy link
Contributor

This is a fix for "InvalidDocument: key u'$_internalJsEmit' must not start with '$'" (BF-21450). It handles new metrics introduced in SERVER-56422 (and others coming in SERVER-56602) the same way as other $-prefixed field were handled in get_server_status.

Here is the patch

Copy link

@alyacb alyacb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you needed a review from me, but this LGTM.

Comment on lines +1143 to +1146
if "metrics" in ss and "operatorCounters" in ss["metrics"] and "expressions" in ss["metrics"]["operatorCounters"]:
del ss["metrics"]["operatorCounters"]["expressions"]
if "metrics" in ss and "operatorCounters" in ss["metrics"] and "match" in ss["metrics"]["operatorCounters"]:
del ss["metrics"]["operatorCounters"]["match"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, but why not delete operatorCounters the way aggStageCounters is deleted above?

Suggested change
if "metrics" in ss and "operatorCounters" in ss["metrics"] and "expressions" in ss["metrics"]["operatorCounters"]:
del ss["metrics"]["operatorCounters"]["expressions"]
if "metrics" in ss and "operatorCounters" in ss["metrics"] and "match" in ss["metrics"]["operatorCounters"]:
del ss["metrics"]["operatorCounters"]["match"]
if "metrics" in ss and "operatorCounters" in ss["metrics"]:
del ss["metrics"]["operatorCounters"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for reviewing! My thought here was to delete the minimal amount of info to fix this issue for both SERVER-56422 and SERVER-56602 (assuming we have other operationCounters). Either way it looks like a temp solution as other $-prefixed metrics may be added in the future, so we probably should look into other ways to handle it (I did created HELP-25443 to get better understanding where we are with support of $-prefixed metrics in pymango).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for explaining!

@asya999 asya999 merged commit 46f8f42 into mongodb-labs:master Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants