-
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
DBMON-2051: Adding a deadlock metric to MySQL databases #16904
Conversation
The |
The |
7 similar comments
The |
The |
The |
The |
The |
The |
The |
8849ee1
to
14776c4
Compare
The |
8 similar comments
The |
The |
The |
The |
The |
The |
The |
The |
f4785d4
to
8e89bf4
Compare
mysql/datadog_checks/mysql/mysql.py
Outdated
with self._connect() as db: | ||
self._is_innodb_engine_enabled = self._is_innodb_engine_enabled(db) |
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 suggest to put this under def check
after db connection is established
with self._connect() as db: |
We can also rename the function
_is_innodb_engine_enabled
to something like _check_innodb_engine_enabled
.In the
__init__
constructor, we can just initialize a variable self.innodb_engine_enabled = None
as None to indicate the check has not been done.
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.
thx, for renaming will do.
as for moving it to check - that was my initial idea too. But I didn't like that I had to add db as an argument to runtime_queries. Although, now, I greped the code and no one is calling this from outside of the class. I thought if it's without _ then it's public and I cant change the signature.
The other option is to modify _check_innodb_engine_enabled
, I could've check there if self.conn is None or not, and if not open a connection there. Pls let me know which one is better or something else.
What I don't like in my solution and kind of in existing one is the absence of clarity - how one can know that _check_innodb_engine_enabled
has to be called and not _is_innodb_engine_enabled. Same for the runtime_queries property vs _runtime_queries. May be we call _runtime_queries -> _runtime_queries_cached and same for _is_innodb_engine_enabled -> _is_innodb_engine_enabled_cached. Like this it would be clear that this field might be None.
mysql/changelog.d/16904.added
Outdated
@@ -0,0 +1 @@ | |||
DBMON-2051: Adding a deadlock metric to Maria DB |
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 would not include the ticket in the changelog since this is mostly for customers and they can't have access to our Jira board
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.
True, now, I remembered in my previous company we had two separate entries one for customers and one internal with the ticket. Ok, let me remove it, thx.
e513c14
* DBMON-2051: Adding a deadlock metric to Maria DB * removing print outs * removed prints * trying to create a proper deadlock * Fix thread pool limit * Add a check * Add wait to let update db threads to finish * Add select query * fix query * revert changes to conftest * fix test * Add events * remove obsolete diff * cleaned up pr * moved tests to the end * Put back empty lines * put back empty line * added a changelog * changed pr number in changelog * removed patched config * reverting back conftest * reverted common py * applied linter * Added the correct changelog * add a check for innnodb * removed superfluous method * removed whitespace * changed to monotonic_count * removed check from constructor * modified variable names * fixed changelog and test error * improved the changelog --------- Co-authored-by: root <[email protected]>
* DBMON-2051: Adding a deadlock metric to Maria DB * removing print outs * removed prints * trying to create a proper deadlock * Fix thread pool limit * Add a check * Add wait to let update db threads to finish * Add select query * fix query * revert changes to conftest * fix test * Add events * remove obsolete diff * cleaned up pr * moved tests to the end * Put back empty lines * put back empty line * added a changelog * changed pr number in changelog * removed patched config * reverting back conftest * reverted common py * applied linter * Added the correct changelog * add a check for innnodb * removed superfluous method * removed whitespace * changed to monotonic_count * removed check from constructor * modified variable names * fixed changelog and test error * improved the changelog --------- Co-authored-by: root <[email protected]>
What does this PR do?
This PR adds a new deadlock count metric to the MySQL integrations.
This metrics tracks a number of deadlocks that have been detected in the DB.
Motivation
The deadlock metric is required by customers.
Review checklist (to be filled by reviewers)
qa/skip-qa
label if the PR doesn't need to be tested during QA.