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 config spec #6908

Merged
merged 8 commits into from
Jun 24, 2020
Merged

Add config spec #6908

merged 8 commits into from
Jun 24, 2020

Conversation

florimondmanca
Copy link
Contributor

@florimondmanca florimondmanca commented Jun 16, 2020

What does this PR do?

Add config spec for MySQL

Motivation

Config specs everywhere! Also I might need to add a charset option for a support case, so having the config spec done beforehand would be nice.

Additional Notes

Not super happy with optional sections such as ssl, custom_queries or queries showing up as un-commented in the generated conf.yaml.example - but we can look at tweaking ddev later.

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

sarina-dd
sarina-dd previously approved these changes Jun 17, 2020
Copy link
Member

@AlexandreYang AlexandreYang left a comment

Choose a reason for hiding this comment

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

Nice. Not the easiest config spec to translate :)

Left few comments.

mysql/datadog_checks/mysql/data/conf.yaml.example Outdated Show resolved Hide resolved
mysql/datadog_checks/mysql/data/conf.yaml.example Outdated Show resolved Hide resolved
# extra_performance_metrics: true

## Log Section (Available for Agent >=6.0)
options:
Copy link
Member

@AlexandreYang AlexandreYang Jun 22, 2020

Choose a reason for hiding this comment

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

(nit) Shall we keep that commented by default ? I guess doesn't change much to have an empty dict either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree, but ddev doesn't seem to support it for now. I agree an empty options dict by default shouldn't be a problem since we do

options = instance.get('options', {}) or {}  # options could be None if empty in the YAML

I can look at how to add commented-out-by-default sections to ddev, but looks like it's not a blocker to get this PR merged.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, not a blocker for this PR.

mysql/datadog_checks/mysql/data/conf.yaml.example Outdated Show resolved Hide resolved
Copy link
Contributor Author

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

@hithwen @AlexandreYang Thanks, I think I addressed all your comments.

Comment on lines -107 to -108
# # Columns without a name are ignored. Add additional columns to skip:
# # - {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understood this comment: is it that eg if the query returns 3 columns, we can do - {} on the first one and define the 2 others to only submit metrics for the last 2 columns? Not sure this is correct, since if the user wants to "skip columns" then they can just omit them in the SELECT clause 🤔 So I removed it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ofek added this feature recently so he must remember what he meant

@florimondmanca
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@hithwen hithwen left a comment

Choose a reason for hiding this comment

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

Service in the logs section is not recommended anymore

mysql/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
mysql/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
mysql/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
@hithwen
Copy link
Contributor

hithwen commented Jun 23, 2020

Can you bump checks base on the setup to 11.0.0? (to include service features)

@florimondmanca florimondmanca requested a review from hithwen June 23, 2020 14:43
ofek
ofek previously requested changes Jun 23, 2020
mysql/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
@florimondmanca florimondmanca requested a review from ofek June 24, 2020 08:34
@florimondmanca florimondmanca dismissed stale reviews from ofek and hithwen June 24, 2020 11:51

Outdated

@florimondmanca florimondmanca merged commit 5ed6f1b into master Jun 24, 2020
@florimondmanca florimondmanca deleted the mysql-spec branch June 24, 2020 11:52
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.

6 participants