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

Update default configuration to collect postgres database by default #12999

Conversation

alexandre-normand
Copy link
Contributor

@alexandre-normand alexandre-normand commented Sep 21, 2022

What does this PR do?

This PR aims to address confusion that comes up related to missing data due to the default postgres database being ignored. This updates the default configuration to enable data collection on the default database. We had talked about removing postgres from the default ignored_databases but since collect_default_database also directly relates to it, I think it makes sense to update both configuration defaults. For reference, here's where the collect_default_database gets used and how it relates to ignored_databases: https://github.com/DataDog/integrations-core/blob/master/postgres/datadog_checks/postgres/config.py#L73-L74.

Note that this is technically a breaking change because the collection behavior is going to change. We're going ahead with this change because it's going to be a better overall experience moving forward.

Motivation

Additional Notes

I validated the performance and impact of this on our integration environment. Note that I tested this change along with #12998 because they are both related to the same goal of reducing confusion regarding our data collection, going out at the same time. The material impact is that prior to both changes, query samples/activity was not collected on the default postgres database. With the two changes, they now are. And because query metric collection didn't honor the ignore_databases configuration before, query metrics on the default postgres database remains unchanged with the new default configuration.

Screen Shot 2022-09-23 at 9 24 24 AM

Screen Shot 2022-09-23 at 9 24 32 AM

Screen Shot 2022-09-23 at 9 24 39 AM

Here's a clearer way to see the impact. This shows the different behavior of 7.39.0 vs the version with the changes with one deployment using the default config and another one setting collect_default_database to false.
Screen Shot 2022-09-23 at 9 30 19 AM

Screen Shot 2022-09-23 at 9 25 56 AM

Screen Shot 2022-09-23 at 9 26 04 AM

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

@github-actions
Copy link

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

2 similar comments
@github-actions
Copy link

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

@github-actions
Copy link

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@alexandre-normand alexandre-normand force-pushed the alex.normand/update-default-configuration-to-collect-default-postgres-database-by-default branch from f89599e to c55707e Compare September 21, 2022 23:48
@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Merging #12999 (8ba5d45) into master (ed547c1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Flag Coverage Δ
postgres 91.87% <100.00%> (+0.19%) ⬆️

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

@alexandre-normand alexandre-normand changed the title Update default configuration for collect_default_database to true Update default configuration to collect postgres database by default Sep 23, 2022
Copy link
Contributor

@jmeunier28 jmeunier28 left a comment

Choose a reason for hiding this comment

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

lgtm

@alexandre-normand alexandre-normand merged commit 68b2ae6 into master Sep 26, 2022
@alexandre-normand alexandre-normand deleted the alex.normand/update-default-configuration-to-collect-default-postgres-database-by-default branch September 26, 2022 16:03
steveny91 pushed a commit that referenced this pull request Oct 27, 2022
…12999)

* Update default configuration for collect_default_database to true

* Fix unit test to account for the default db being included

* Also update ignore_databases to reduce confusion
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