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.

Sign up for GitHub

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

Jump to bottom

Add DBM SQL obfuscator options #9639

Merged
merged 13 commits into from
Jul 9, 2021
Merged

Add DBM SQL obfuscator options #9639

merged 13 commits into from
Jul 9, 2021

Conversation

alexbarksdale
Copy link
Member

What does this PR do?

The obfuscation library within the datadog-agent is being updated to support options to configure how the obfuscator behaves. This PR adds an obfuscator option (quantize_sql_tables) to the MySQL instance config and implements the necessary changes to use options on the integration side. For more context, please refer to this PR that is responsible for implementing the obfuscator options on the agent.

Motivation

This integration implements the obfuscator library located in the datadog-agent and it needs to be updated to support the new options functionality. For a full explanation how this integration implements the library, please refer to this PR.

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 as code owners July 6, 2021 20:42
@alexbarksdale alexbarksdale mentioned this pull request Jul 6, 2021
Merged
1 task
cswatt
cswatt previously approved these changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@cswatt cswatt left a comment

Choose a reason for hiding this comment

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

approved for docs!

@codecov
Copy link

codecov bot commented Jul 7, 2021 •
edited
Loading

Codecov Report

Merging #9639 (0bb1945) into master (5584fc3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Flag Coverage Δ
mysql 84.90% <100.00%> (+0.11%) ⬆️

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

@ghost ghost added the documentation label Jul 7, 2021
remeh
remeh reviewed Jul 8, 2021
View reviewed changes
mysql/tests/test_mysql.py
@@ -377,7 +377,7 @@ def test_statement_metrics_with_duplicates(aggregator, dbm_instance, datadog_age

mysql_check = MySql(common.CHECK_NAME, {}, instances=[dbm_instance])

def obfuscate_sql(query):
def obfuscate_sql(query, options=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth having a test with the option set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally, however, in order to create a proper test for methods that interact with C bindings that communicate with the agent directly would require a hard dependency (rtloader) which would have some overhead to do because it would need to touch the build system in some way. Methods like this (agent stub methods) are difficult to test because of the disconnect, but I agree that it would be nice, it's just currently not worth it as we'd like to get this in before the 7.30 freeze.

djova
djova requested changes Jul 8, 2021
View reviewed changes
mysql/datadog_checks/mysql/statements.py Outdated Show resolved Hide resolved
@alexbarksdale alexbarksdale requested a review from djova July 8, 2021 19:57
remeh
remeh previously approved these changes Jul 8, 2021
View reviewed changes
Copy link
Contributor

@remeh remeh left a comment

Choose a reason for hiding this comment

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

LGTM from agent-core perspective!

djova
djova previously approved these changes Jul 8, 2021
View reviewed changes
ofek
ofek requested changes Jul 8, 2021
View reviewed changes
datadog_checks_base/datadog_checks/base/stubs/datadog_agent.py Outdated
@@ -65,7 +65,7 @@ def write_persistent_cache(self, key, value):
def read_persistent_cache(self, key):
return self._cache.get(key, '')

def obfuscate_sql(self, query):
def obfuscate_sql(self, query, options=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do this in a separate PR https://datadoghq.dev/integrations-core/guidelines/pr/#separation-of-concerns

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: #9662

@alexbarksdale alexbarksdale dismissed stale reviews from djova and remeh via 23bc050 July 8, 2021 20:41
@alexbarksdale alexbarksdale mentioned this pull request Jul 8, 2021
Merged
4 tasks
@alexbarksdale alexbarksdale requested a review from ofek July 8, 2021 20:54
@ofek ofek changed the title [MySQL][database-monitoring] Add and implement obfuscator options Add DBM SQL obfuscator options Jul 8, 2021
@alexbarksdale alexbarksdale merged commit 0d1d290 into master Jul 9, 2021
@alexbarksdale alexbarksdale deleted the alex.barksdale/quantize-config-option-mysql branch July 9, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofek ofek ofek approved these changes

@cswatt cswatt cswatt left review comments

@remeh remeh remeh left review comments

@djova djova djova left review comments

Assignees
No one assigned
Labels
documentation integration/datadog_checks_base integration/mysql
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alexbarksdale @cswatt @remeh @djova @ofek