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

Fix schema filtering on query relations #3449

Merged
merged 4 commits into from
Apr 22, 2019

Conversation

fischaz
Copy link
Contributor

@fischaz fischaz commented Apr 3, 2019

Fixed the loading of the configuration file (avoid KeyError and check yaml structure).
Fix filtering loop on query_scope to continue on other rows.
Simplify parameter and removed 'relations' as 'relations_config' contains it all.

What does this PR do?

Fix the filtering of the relations queries in the postgres integration. Previously, the filtering would 'return' and abort all metrics if one table was not matching the schema requirement in the configuration file. This was incorrect and the method should have skipped that row (non matching) but continued for the other row in the result to push matching metrics.

This also improved a bit the parsing of the configuration file (avoid KeyError and check for type of object and key present in dict) and removed redundant argument in methods.

Motivation

we have tables with the same name in different schemas in our database, but are only interested in monitoring one schema.table combination, not all. The current datadog would either report both table (wasting metrics and dashboard) or fail to run/report if we forced the schema.

Additional Notes

N/A - first PR :)

Review checklist (to be filled by reviewers)

  • 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
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

Fixed the loading of the configuration file (avoid KeyError and check yaml structure).
Fix filtering loop on query_scope to continue on other rows.
Simplify parameter and removed 'relations' as 'relations_config' contains it all.
@fischaz fischaz requested a review from a team as a code owner April 3, 2019 06:21
@fischaz fischaz changed the title Fixed schema filtering on postgres relations (#3440) changelog/Fixed schema filtering on postgres relations (#3440) Apr 3, 2019
@fischaz
Copy link
Contributor Author

fischaz commented Apr 3, 2019

ok, I failed at adding the label here.

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #3449 into master will decrease coverage by 1.82%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##           master    #3449      +/-   ##
==========================================
- Coverage   86.11%   84.28%   -1.83%     
==========================================
  Files         725        9     -716     
  Lines       37483      751   -36732     
  Branches     4498      109    -4389     
==========================================
- Hits        32277      633   -31644     
+ Misses       3967       84    -3883     
+ Partials     1239       34    -1205

@ofek ofek changed the title changelog/Fixed schema filtering on postgres relations (#3440) Fix schema filtering on query relations (#3440) Apr 22, 2019
@ofek ofek changed the title Fix schema filtering on query relations (#3440) Fix schema filtering on query relations Apr 22, 2019
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.

@fischaz Excellent first PR, thank you!

Also fyi, only those with write access can add labels

@ofek ofek merged commit 360eda2 into DataDog:master Apr 22, 2019
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.

2 participants