Skip to content

Add Datadog-ProxySQL integration#531

Closed
reyiyo wants to merge 12 commits intoDataDog:masterfrom
mercadolibre:proxysql
Closed

Add Datadog-ProxySQL integration#531
reyiyo wants to merge 12 commits intoDataDog:masterfrom
mercadolibre:proxysql

Conversation

@reyiyo
Copy link
Copy Markdown

@reyiyo reyiyo commented Feb 5, 2020

What does this PR do?

This PR introduces a new agent check for ProxySQL which is a high performance MySQL proxy. ProxySQL is officially supported by Percona and is the recommended load balancer for Percona XtraDB Cluster.

Originally written by @ovaistariq (DataDog/dd-agent#3107) and opened as #39 and @bangpound (#97)

Motivation

ProxySQL is a commonly used database proxy in MySQL and Percona XtraDB Cluster deployments, and Datadog is a popular monitoring tool. Datadog currently doesn't have any agent check for ProxySQL, hence adding this agent check to Datadog will benefit other users in the community.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached

  • Feature or bugfix has tests

  • Git history is clean

  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

Not applicable.

@reyiyo reyiyo requested review from a team as code owners February 5, 2020 00:39
@reyiyo reyiyo changed the title Proxysql Add Datadog-ProxySQL integration Feb 5, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 5, 2020

Copy link
Copy Markdown
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

Hi @reyiyo, thanks for your contribution! I've made a few comments/mostly nits. Looks great so far!

@reyiyo reyiyo requested review from ChristineTChen and removed request for a team February 5, 2020 23:06
@reyiyo
Copy link
Copy Markdown
Author

reyiyo commented Feb 5, 2020

Thank you very much @ChristineTChen! The requested changes are ready

@reyiyo
Copy link
Copy Markdown
Author

reyiyo commented Feb 18, 2020

Hi @ChristineTChen! I was wondering if you've had a chance to look at this PR

Copy link
Copy Markdown
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

Hi! I took another look and added a few more comments. Overall looks good :)

@reyiyo
Copy link
Copy Markdown
Author

reyiyo commented Feb 19, 2020

Thank you @ChristineTChen! The requested changes are ready for review

Copy link
Copy Markdown
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

Docs review

@reyiyo
Copy link
Copy Markdown
Author

reyiyo commented Feb 24, 2020

Thank you @ruthnaebeck! The requested changes are ready for review.

@reyiyo reyiyo requested a review from ruthnaebeck February 24, 2020 02:27
Copy link
Copy Markdown
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

Copy link
Copy Markdown
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

Hi @reyiyo! I reviewed the metrics that the ProxySQL integration is submitting. I've left some comments on metric types.

From the metadata descriptions, it's not super clear to me why some of the rate metrics should be submitted as a rate. If the metrics should be time-normalized as rate types, then could we include that in the description or remapped name?

@reyiyo reyiyo requested a review from ChristineTChen March 10, 2020 09:22
Copy link
Copy Markdown
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

Thanks @reyiyo for taking the time to address everything! Looks good to me :)

@dsahni
Copy link
Copy Markdown
Contributor

dsahni commented Mar 11, 2020

Hi @reyiyo !
After discussing internally, we think this integration would actually make a good candidate for the integrations-core repo, where it will be included with the Datadog agent installer by default. This would make it easier for you to install the integration as well, removing the step of having to build a Python wheel yourself.

We'd also like to add a default dashboard for it and look at any other enhancements/improvements that would benefit ProxySQL/Datadog users. Please let us know what you think!

@reyiyo
Copy link
Copy Markdown
Author

reyiyo commented Mar 11, 2020

Hi @dsahni !

That'd be awesome! Please let me know how do we proceed.

@hithwen
Copy link
Copy Markdown
Contributor

hithwen commented Mar 13, 2020

Hi @reyiyo, we'll backport this PR to integrations-core (and then close the issue) and do the remaining work.

@FlorianVeaux
Copy link
Copy Markdown
Member

Hello @reyiyo,
as said we've backported your integration in the integrations-core repository with this PR. I've included some additional changes that are all described in the PR description but they are mostly implementation details and don't change metrics or service_checks for the most part.

Feel free to have a look and comment if you think of anything. Any input will be appreciated!

@reyiyo
Copy link
Copy Markdown
Author

reyiyo commented Apr 1, 2020

Thank you very much @FlorianVeaux. The PR looks great. Looking forward to see it merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants