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

New Integration ProxySQL #6144

Merged
merged 29 commits into from
Apr 2, 2020
Merged

New Integration ProxySQL #6144

merged 29 commits into from
Apr 2, 2020

Conversation

FlorianVeaux
Copy link
Member

@FlorianVeaux FlorianVeaux commented Mar 25, 2020

ProxySQL is a new integration, originally implemented here

This PR is based on this implementation but a few changes are to be noted.
To show the full diff, you can use this permalink

Implementation-wise

  1. Most of the query logic was removed to rely on the QueryManager util. This util is used in other database like integrations and allows us to abstract a lot of technicalities to focus only on how to connect and execute queries against the database. The rest is handled by the QueryManager.
  2. The version of ProxySQL is now collected.
  3. A new service check called proxysql.backend.connect is emitted for each mysql backend that proxysql knows about. The value of the service check is based on the availability of the given backend.
  4. The integration uses a configuration spec so that the conf.yaml.example is generated automatically.
  5. Updated docs to reflect it is a default integration now
  6. Added SSL support

Metric-wise

  1. Most of time related metrics (like proxysql.query_processor_time_ms) that means the number of time spent doing a given thing are converted into temporal_percent (i.e proxysql.query_processor_time_pct) that represent the amount of time spent in that state as a percentage of total time. This is generally easier to work with.
  2. Added proxysql.uptime
  3. Made query_cache metrics a rate
  4. Updated the metadata.csv to use gauge instead of rate everywhere. When the agent submits a rate, the backend type for that metric is a gauge
  5. Kept a single slite3_memory metric under the name proxysql.memory.sqlite3_memory_bytes

@FlorianVeaux FlorianVeaux force-pushed the florian/proxysql branch 2 times, most recently from ea0a387 to 02d1668 Compare March 26, 2020 10:17
@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #6144 into master will increase coverage by 2.33%.
The diff coverage is 90.04%.

Impacted Files Coverage Δ
proxysql/datadog_checks/proxysql/ssl_utils.py 34.78% <34.78%> (ø)
proxysql/datadog_checks/proxysql/proxysql.py 90.90% <90.90%> (ø)
proxysql/tests/test_proxysql.py 97.56% <97.56%> (ø)
proxysql/datadog_checks/proxysql/__about__.py 100.00% <100.00%> (ø)
proxysql/datadog_checks/proxysql/__init__.py 100.00% <100.00%> (ø)
proxysql/datadog_checks/proxysql/queries.py 100.00% <100.00%> (ø)
proxysql/tests/common.py 100.00% <100.00%> (ø)
proxysql/tests/conftest.py 100.00% <100.00%> (ø)
mesos_slave/datadog_checks/mesos_slave/__init__.py
...g_checks_base/datadog_checks/base/stubs/tagging.py
... and 844 more

@FlorianVeaux FlorianVeaux changed the title WIP - New Integration ProxySQL New Integration ProxySQL Mar 27, 2020
proxysql/manifest.json Outdated Show resolved Hide resolved
l0k0ms
l0k0ms previously approved these changes Mar 27, 2020
Copy link
Contributor

@l0k0ms l0k0ms left a comment

Choose a reason for hiding this comment

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

G2G doc wise after the small nits are integrated :)

Co-Authored-By: Pierre Guceski <pierre.guceski@datadoghq.com>
FlorianVeaux and others added 7 commits March 27, 2020 16:45
Co-Authored-By: Pierre Guceski <pierre.guceski@datadoghq.com>
Co-Authored-By: Pierre Guceski <pierre.guceski@datadoghq.com>
Co-Authored-By: Pierre Guceski <pierre.guceski@datadoghq.com>
Co-Authored-By: Pierre Guceski <pierre.guceski@datadoghq.com>
Copy link
Contributor

@therve therve left a comment

Choose a reason for hiding this comment

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

Looks great! Some minor comments, mostly a doc discrepancy.

proxysql/datadog_checks/proxysql/proxysql.py Show resolved Hide resolved
proxysql/datadog_checks/proxysql/proxysql.py Outdated Show resolved Hide resolved
proxysql/datadog_checks/proxysql/proxysql.py Outdated Show resolved Hide resolved
proxysql/datadog_checks/proxysql/proxysql.py Outdated Show resolved Hide resolved
proxysql/README.md Outdated Show resolved Hide resolved
therve
therve previously approved these changes Mar 31, 2020
Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Awesome!!!

proxysql/datadog_checks/proxysql/proxysql.py Outdated Show resolved Hide resolved
proxysql/datadog_checks/proxysql/proxysql.py Outdated Show resolved Hide resolved
proxysql/datadog_checks/proxysql/ssl_utils.py Show resolved Hide resolved
proxysql/tests/conftest.py Show resolved Hide resolved
proxysql/tests/conftest.py Outdated Show resolved Hide resolved
)


def get_check(instance):
Copy link
Contributor

@ofek ofek Apr 1, 2020

Choose a reason for hiding this comment

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

For when we have multiple test files, may want to do this now #6117 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

But as a fixture you need to set an extra param in each test function.
In that case there is a single test file, but even with multiple, we could import the helper method from somewhere and I think it's cleaner to have a single import than updating every test signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

by that logic, what's the point of fixtures for config?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a fixture is useful if you execute code in it. You read a file maybe, or you do a deepcopy of an integration config. By that mean, it abstracts a lot of complexity in your test, you just use the result of that fixture as a parameter of your tests.

If the result of your fixture is a static function that you could define what's the point? Seems more complex (the test runs the code of the fixture, gets the result, put in in an argument of the test, then you call that argument (because the result of that fixture was a function itself)....) than simply defining the function statically.

Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>
FlorianVeaux and others added 3 commits April 1, 2020 10:42
Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>
Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>
Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Excellent!

@FlorianVeaux FlorianVeaux merged commit 06e96d7 into master Apr 2, 2020
@FlorianVeaux FlorianVeaux deleted the florian/proxysql branch April 2, 2020 08:43
@pkatiushyn
Copy link

Hello, this does not seem to work with ProxySQL user specified in admin-stats_credentials.

2020-04-30 12:16:00 UTC | CORE | ERROR | (pkg/collector/python/datadog_agent.go:116 in LogMessage) | proxysql:5f743655467150c0 | (core.py:68) | Error querying stats_mysql_global: (1045, u'ProxySQL Admin Error: no such table: stats.stats_mysql_global')
2020-04-30 12:16:00 UTC | CORE | WARN | (pkg/collector/python/datadog_agent.go:118 in LogMessage) | proxysql:5f743655467150c0 | (proxysql.py:83) | Failed to fetch records from query: `SELECT variable_value FROM main.global_variables WHERE variable_name='admin-version'`.

@tabacco
Copy link
Contributor

tabacco commented Jun 3, 2020

For users in admin-stats_credentials, the 'stats' database is named 'main'. Also, there's no way for stats users to get the proxysql version.

@pkatiushyn
Copy link

It's just my suggestion. I think it's not secure to use admin user for monitoring and I think it is possible to use user specified in admin-stats_credentials.
ProxySQL version probably can be obtained from shell proxysql -v.

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.

7 participants