-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
704 log slow transactions #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minimal change from my end.
# calculate statistics for this group | ||
datetime, txnNumber, autocommit, readConcern, timeActiveMicros, timeInactiveMicros, duration = g | ||
stats = OrderedDict() | ||
#stats['lsid'] = lsid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid committing commented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
mtools/util/logevent.py
Outdated
|
||
if not self._counters_calculated: | ||
self._counters_calculated = True | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments
try: | ||
import numpy as np | ||
except ImportError: | ||
np = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems numpy is not used, we might remove the import
self.mloginfo.argparser_sectiongroup.add_argument('--tsort', | ||
action='store', | ||
|
||
choices=['duration' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird ident, keep the [] in the same line
|
||
if len(self.mloginfo.args['logfile']) > 1: | ||
# merge log files by time | ||
for logevent in self._merge_logfiles(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is _merge_logfiles defined? I couldn't find
@@ -901,6 +1041,14 @@ def _parse_document(self): | |||
self._ninserted = doc[u'ninserted'] if 'ninserted' in doc else None | |||
self._ndeleted = doc[u'ndeleted'] if 'ndeleted' in doc else None | |||
self._numYields = doc[u'numYield'] if 'numYield' in doc else None | |||
self._txnNumber = doc[u'txnNumber'] if 'txnNumber' in doc else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole snippet can probably be optimized in a better way:
all the lines can use doc.get('x')
instead of doc['x'] if 'x' in doc else None
then, setting the attribute can be done with self.__setattr__
and all the attributes can be in a list and we can loop through them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments (also see other reviews :) )
|
||
|
||
|
||
LogTuple = namedtuple('LogTuple', ['datetime', 'txnNumber', 'autocommit', 'readConcern', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
if self.mloginfo.progress_bar_enabled and (i % 1000 == 0): | ||
if le.datetime: | ||
progress_curr = self.mloginfo._datetime_to_epoch(le |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid breaking long lines like this, le.datetime can be in the same line
self.mloginfo.update_progress(1.0) | ||
|
||
# no queries in the log file | ||
if len(grouping) < 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be replaced by
if not len(grouping):
as per other reviews
if (counter == 'level' and token.startswith('level')): | ||
try: | ||
self._readConcern = ( | ||
split_tokens[t + 1 + self.datetime_nextpos + 2].replace(',', '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to split like this (unless it should be a tuple, but it doesn't look like it)
regardless, split_token should be one level down at least (if breaking the line like this)
|
||
except ValueError: | ||
pass | ||
elif (counter == 'readTimestamp' and token.startswith('readTimestamp')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these blocks (this and the one below) are badly indented, please review
… blocks, indent corrections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description of changes
mloginfo has a —transactions flag. This flag has been used to instruct the parser to look for the lines containing the “transaction” label (for reference, see SERVER-36461). Once the line is found, the parameters can be parsed similar to queries. The issue of legacy tokens as well as pre 2.5.2 tokens (tokens with spaces in between such as numYields: 2) must be dealt separately with exceptions or if-else conditional loops.
The level of readConcern can also be parsed as part of the process above (for reference, see SERVER-36414)
The information displayed in mloginfo —transactions is fairly basic and only for “at a glance”. For a more detailed account, mlogfilter —transactions flag can be added to parse the entire line. The parser will always look for “transaction” keyword and it can decide whether to accept or reject the log line.
Testing
The following testing tools have been used to test the newly added code:
The test result of a code coverage can be seen in table below:
Name Stmts Miss Cover
---------------------------------------------------------------------
mtools/mlogfilter/filters/logline_filter.py 62 1 98%
mtools/mlogfilter/filters/slow_filter.py 14 0 100%
mtools/mlogfilter/filters/transaction_filter.py 14 5 64%
mtools/mloginfo/sections/transaction_section.py 68 2 97%
Version Information
| macOS | 10.14.5 |
| mongod | v4.0.10 |
| MongoDB shell | v4.0.10 |
| Python | 2.7.10 |