Skip to content
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

Provide a reason for not having an execution plan (MySQL) #9569

Merged
merged 11 commits into from
Jun 29, 2021

Conversation

alexbarksdale
Copy link
Member

What does this PR do?

Provides a reason for not having a MySQL execution plan.

Motivation

Currently in Database Monitoring, the UX is lacking when it comes to execution plans. There are cases where it's impossible to collect or something failed, so we should indicate the reason it could not be collected and actions to fix it, if any.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@alexbarksdale alexbarksdale requested review from a team and removed request for a team June 21, 2021 16:55
@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #9569 (dbab18b) into master (991c3d0) will increase coverage by 0.00%.
The diff coverage is 90.32%.

Flag Coverage Δ
mysql 85.70% <90.32%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mysql/datadog_checks/mysql/statement_samples.py 80.50% <88.88%> (+0.33%) ⬆️
mysql/tests/test_mysql.py 96.46% <100.00%> (+0.19%) ⬆️
mysql/tests/conftest.py 94.32% <0.00%> (+0.04%) ⬆️
mysql/datadog_checks/mysql/mysql.py 79.87% <0.00%> (+0.20%) ⬆️
mysql/datadog_checks/mysql/innodb_metrics.py 66.09% <0.00%> (+0.42%) ⬆️

@alexbarksdale alexbarksdale marked this pull request as ready for review June 22, 2021 18:30
@alexbarksdale alexbarksdale requested review from a team as code owners June 22, 2021 18:30
mysql/datadog_checks/mysql/statement_samples.py Outdated Show resolved Hide resolved
mysql/datadog_checks/mysql/statement_samples.py Outdated Show resolved Hide resolved
mysql/datadog_checks/mysql/statement_samples.py Outdated Show resolved Hide resolved
mysql/datadog_checks/mysql/statement_samples.py Outdated Show resolved Hide resolved
except pymysql.err.DatabaseError as e:
if len(e.args) != 2:
raise
# we don't cache failed plan collection failures for specific queries because some queries in a schema
# can fail while others succeed. The failed collection will be cached for the specific query
# so we won't try to explain it again for the cache duration there.
db_explain_error, err = DBExplainError.database_error, e
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should collect all of the errors, by strategy? The problem with this approach is that we'll only get the error for the last strategy, potentially hiding other setup issues.

Perhaps for mysql instead of just one (code, reason) we have a map of strategy -> (code, reason)? Maybe under a different key: collection_error_by_strategy

@justiniso what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline, but for context, there are cases where we wont have a strategy to make a mapping of strategy -> (code, msg) (i.e _can_explain()) so we were thinking, what if we simplify it to where we structure it as[{strategy, code, message}, ...] under collection_errors to be more universal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to implement this approach, we can revert if necessary.

@alexbarksdale alexbarksdale requested a review from djova June 23, 2021 14:38
djova
djova previously approved these changes Jun 23, 2021
Copy link
Contributor

@djova djova left a comment

Choose a reason for hiding this comment

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

👍 looks good. Left a couple more minor comments

@alexbarksdale alexbarksdale merged commit 45fefb2 into master Jun 29, 2021
@alexbarksdale alexbarksdale deleted the alex.barksdale/no-exec-plan-reason-mysql branch June 29, 2021 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants