-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[MySQL] Adds another option for innodb aio reads and writes #660
[MySQL] Adds another option for innodb aio reads and writes #660
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.
Looks good, would just like to see a comment at the top of the if-block to now what we may expect of the newfound format.
mysql/check.py
Outdated
@@ -1023,6 +1023,9 @@ def _get_stats_from_innodb_status(self, db): | |||
elif len(row) == 18: | |||
results['Innodb_pending_normal_aio_reads'] = long(row[4]) | |||
results['Innodb_pending_normal_aio_writes'] = long(row[12]) | |||
elif len(row) == 22: |
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.
could you please add the format in the commented section above.... Man there are way too many formats 😅
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.
Done 🙂
normal aio reads"
mysql/CHANGELOG.md
Outdated
@@ -45,3 +52,45 @@ | |||
[#291]: https://github.com/DataDog/integrations-core/issues/291 | |||
[#329]: https://github.com/DataDog/integrations-core/issues/329 | |||
[#394]: https://github.com/DataDog/integrations-core/issues/394 | |||
[#660]: https://github.com/DataDog/integrations-core/pull/660 |
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.
Small bug in edition here: duplicate file after footer.
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.
Sorry about that one, copy/paste failed me. Will be more careful on that. Should be good now
mysql/check.py
Outdated
@@ -891,6 +891,12 @@ def _get_slave_status(self, db, above_560, nonblocking): | |||
self.warning("Privileges error accessing the process tables (must grant PROCESS): %s" % str(e)) | |||
return {} | |||
|
|||
def _are_values_numeric(self, array): | |||
for v in array: |
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.
the all
func does that:
return all([v.isdigit() for v in array])
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.
I think we can make this a little cleaner just by harnessing the ValueError
exceptions that could arise. Let me know how you feel about it @nmuesch
mysql/check.py
Outdated
results['Innodb_pending_normal_aio_writes'] = long(row[10]) | ||
if len(row) == 8: | ||
# (len(row) == 8) Pending normal aio reads: 0, aio writes: 0, | ||
if row[4].isdigit() and row[7].isdigit(): |
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's nice that you did this, but I don't think you really needed to ;)
The actual long(row[n])
will raise a ValueError if we can't parse to a long. So wrapping this entire Pending normal aio reads in a try...except ValueError
would've probably sufficed. 😅
This would probably apply to all the blocks below.
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.
The only issue of doing the entire surrounding try/except is that we are coming to the point where multiple len==16 return different output. So we would have to check if some values were a digit anyway. (Also this code came from @derekwbrown after our discussion, can't take the credit :) )
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.
@nmuesch Originally did this as you suggest; but on only one if
. I suggested this as a way to catch new formats going forward. Could also wrap the entire section in try...except
i suppose.
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.
Ok, discussed further. Will do a combination of the two approaches to be cleaner/less lines :)
mysql/check.py
Outdated
long(row[13]) + long(row[14])) | ||
# (len(row) == 16) Pending normal aio reads: [0, 0, 0, 0] , aio writes: [0, 0, 0, 0] , | ||
if self._are_values_numeric(row[4:8]) and self._are_values_numeric(row[11:15]): | ||
results['Innodb_pending_normal_aio_reads'] = (long(row[4]) + long(row[5]) + |
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.
Wouldn't this fail for row[4]: long('[0')
for example? I think we have to sanitize that:
eg. long(row[4].strip('['))
Similarly, for row[7]: long(row[7])
---> long(row[7].strip(']'))
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.
I think it's "cleaned" here https://github.com/DataDog/integrations-core/pull/660/files#diff-8ec4e9a82d9ad3186148923dd699e609R932
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.
Looks good! Thanks @nmuesch
What does this PR do?
Adds another format option to the mysql innodb aio reads and writes output.
Motivation
Customer reached out using MySQL 5.5.38, with the output of SHOW INNODB STATUS of:
Also takes from @derekwbrown's update to ensure we don't crash in the future when new formats arise.
Testing Guidelines
An overview on testing
is available in our contribution guidelines.
Versioning
manifest.json
CHANGELOG.md
Additional Notes
Anything else we should know when reviewing?