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

Add MySQL Active Sessions #11709

Merged
merged 22 commits into from
Apr 4, 2022
Merged

Conversation

alexbarksdale
Copy link
Member

@alexbarksdale alexbarksdale commented Mar 21, 2022

What does this PR do?

Adds a new feature which collects query activity and includes information like wait events, when the event started, who/what/where was the query executed, etc.

Query author: @kyle-hailey

Motivation

Additional Notes

Agent health

Screen Shot 2022-03-29 at 3 47 33 PM
agent memory usage, per dbms version

  • A slight increase in mem and cpu usage is expected due to the new job

Core metrics

Screen Shot 2022-03-29 at 2 09 55 PM

  • Similarly to the agent health, there's a slight increase in latency across the jobs

Database health

Screen Shot 2022-03-30 at 9 55 46 AM

  • This view is MySQL v8.0

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

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

There's currently a weird flaky issue that causes test_minimal_config to break if the activity tests run first. Need to investigate later.
@alexbarksdale alexbarksdale marked this pull request as ready for review March 29, 2022 18:20
@alexbarksdale alexbarksdale requested review from a team as code owners March 29, 2022 18:20
Copy link
Contributor

@justiniso justiniso left a comment

Choose a reason for hiding this comment

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

First review, more inbound!

Comment on lines +50 to +52
COALESCE(
IF(thread_a.processlist_state = 'User sleep', 'User sleep',
IF(waits_a.event_id = waits_a.end_event_id, 'CPU', waits_a.event_name)), 'CPU') AS wait_event,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we do this on the backend?

Choose a reason for hiding this comment

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

Think it makes sense to do it in the SQL so what we are doing is more exposed as opposed to being more hidden in the code. We can do both. We can do the coalesce in the SQL and redo in the code if we want to control it there.

LEFT JOIN performance_schema.events_waits_current AS waits_a ON waits_a.thread_id = thread_a.thread_id AND
waits_a.event_id IN(
SELECT
MAX(waits_b.EVENT_ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing exactly?

Choose a reason for hiding this comment

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

the view events_waits_current can have multiple rows per thread (despite the documentation that states otherwise), thus we use EVENT_ID to identify the row we want to use. We want the row with the highest EVENT_ID value as the most recent and reflects the current wait

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting! That might be worth documenting for future generations of maintainers...

WHERE
thread_a.processlist_state IS NOT NULL AND
thread_a.processlist_command != 'Sleep' AND
thread_a.processlist_command != 'Daemon' AND
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this excluding?

Choose a reason for hiding this comment

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

filtering out idle threads. If the thread is sleeping or state is NULL then it is idle.
The filter on Daemon is questionable. We want to show background, i.e. non-user / non-application waits and activity, but in my testing Daemon entries looked idle. Would be worth investigating more.

Choose a reason for hiding this comment

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

here is output with "daemon"

+-----------+--------+------------------------+----------+
| thread_id | cmd    | PROCESSLIST_STATE      | EVENT_ID | 
+-----------+--------+------------------------+----------+
|        42 | Daemon | Waiting on empty queue |     NULL |
|        46 | Daemon | Suspending             |     NULL |

Guess we could try filtering on COMMMAND and PROCESSLIST_STATE but I don't have a list of PROCESSLIST_STATE values go filter. We could collect them and fitler on the backend.

Copy link
Member Author

@alexbarksdale alexbarksdale Mar 29, 2022

Choose a reason for hiding this comment

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

We could collect them and fitler on the backend.

+1, we should submit everything we think we might need because we can always adjust things later in our back-end.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would bias towards collecting fewer rows until we need them. But in this case it doesn't seem that we're confident daemon threads are truly idle.

Daemon

This thread is internal to the server, not a thread that services a client connection.

https://dev.mysql.com/doc/refman/8.0/en/thread-commands.html

COMMAND

The type of command the thread is executing on behalf of the client, or Sleep if the session is idle.

https://dev.mysql.com/doc/refman/8.0/en/performance-schema-processlist-table.html

It's hard to say but the language seems to suggest Daemon is non-idle.

mysql/datadog_checks/mysql/activity.py Outdated Show resolved Hide resolved
mysql/datadog_checks/mysql/activity.py Outdated Show resolved Hide resolved
mysql/datadog_checks/mysql/activity.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #11709 (51e8e0b) into master (fd7d999) will increase coverage by 0.02%.
The diff coverage is 93.68%.

Flag Coverage Δ
mysql 88.53% <93.68%> (+1.02%) ⬆️

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

@alexbarksdale alexbarksdale merged commit 70701cd into master Apr 4, 2022
@alexbarksdale alexbarksdale deleted the alex.barksdale/mysql-active-sessions branch April 4, 2022 22:53
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