-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[SDBM-2104] Updates Postgres config handling around dbname when autodiscovery used #21884
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if re.search(exclude_pattern, config.dbname, re.IGNORECASE): | ||
| # Check if user explicitly set dbname | ||
| if 'dbname' in instance: | ||
| validation_result.add_error( | ||
| f'The configured dbname "{config.dbname}" matches the autodiscovery ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not reject configs that intentionally exclude the global view DB
The new validation treats any autodiscovery configuration where dbname matches an exclude pattern as an error. However the exclude list is only used to filter which databases are collected from autodiscovery; it does not dictate which database the agent connects to for the global view. Many existing setups connect to postgres for discovery but exclude it so no metrics are gathered from that database. With this change, those previously valid configurations now fail validation even though connection succeeds and metrics were intentionally suppressed. Consider limiting this error to the case where dbname was auto‑defaulted or when the database truly cannot be connected to, rather than rejecting explicit dbname values that happen to be excluded.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if config.database_autodiscovery.enabled and config.dbname and 'dbname' not in instance: | ||
| import re | ||
|
|
||
| for exclude_pattern in config.database_autodiscovery.exclude: | ||
| try: | ||
| if re.search(exclude_pattern, config.dbname, re.IGNORECASE): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle null autodiscovery exclude list before iterating
The new validation assumes config.database_autodiscovery.exclude is iterable, but the model defines this field as optional. If a user explicitly sets database_autodiscovery.exclude: null to remove default exclusions, the merge logic leaves exclude as None and this loop will raise TypeError: 'NoneType' object is not iterable, preventing the check from starting. Guard against None (e.g., iterate over config.database_autodiscovery.exclude or ()) before scanning for regex matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Validate dbname is not excluded when using autodiscovery | ||
| # Only validate when dbname was NOT explicitly set by the user (auto-defaulted) | ||
| # If user explicitly set dbname, they may intentionally want to connect to an excluded database | ||
| # for global operations while excluding it from per-database metric collection | ||
| if config.database_autodiscovery.enabled and config.dbname and 'dbname' not in instance: | ||
| import re | ||
|
|
||
| for exclude_pattern in config.database_autodiscovery.exclude: | ||
| try: | ||
| if re.search(exclude_pattern, config.dbname, re.IGNORECASE): | ||
| # Auto-defaulted dbname conflicts - suggest setting global_view_db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align autodiscovery exclude validation with runtime matching
The new validation rejects configurations when the auto‑defaulted dbname case‑insensitively matches an exclude pattern (re.search(..., re.IGNORECASE)). However the autodiscovery code that actually filters databases (Filter in datadog_checks_base/utils/discovery/filter.py) compiles exclude patterns without any flags, so matching is case‑sensitive. This discrepancy means a config such as global_view_db: "POSTGRES" with exclude: ["postgres"] will now fail validation even though the runtime logic would treat those values as different databases. Either remove the IGNORECASE flag here or make the runtime filter case‑insensitive so both paths behave consistently.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…iscovery used (DataDog#21884) * Updates config handling around dbname when autodiscovery used * Add changelog * Update logic to properly handle expected cases * Fix regex and additional cases b9e94e3
…iscovery used (#21884) * Updates config handling around dbname when autodiscovery used * Add changelog * Update logic to properly handle expected cases * Fix regex and additional cases
* [MOPU-112] Remove "silenced" options with values from monitor definitions. * Fix Python upgrade tests to be independent of repository Python version (#21888) * Microsoft Copilot: Introduction of assets for first version of integration (reports only) (#21436) * [DBM][Postgres] Debug log if extension is not in loader map (#21891) * Added pg_stat_statement to PG_EXTENSION_LOADER_QUERY * Updated query * Updated code based on discussion with Engineering * [DOCS-12665] Fix typo in metric name (#21890) * [SDBM-2104] Updates Postgres config handling around dbname when autodiscovery used (#21884) * Updates config handling around dbname when autodiscovery used * Add changelog * Update logic to properly handle expected cases * Fix regex and additional cases * Create shared schemas collector for DBM integrations (#21720) * Create shared schemas collector for DBM integrations * WIP * WIP * Changelog * Warning * Remove unused * Lint * AI Fixes * Fix * Feedback * Feedback * Lint * Refactor health * Fix * WIP * Revert "WIP" This reverts commit aa5b86b. * Comment * [DBMON-5907] Update dbms process signatures (#21892) * Update process signatures * Revert accidental single quote addition * Release `datadog_checks_base` (#21903) * [Release] Bumped datadog_checks_base version to 37.24.0 * [Release] Update metadata * Fix Unicode handling in YAML config parsing Update the pull request reference for a fix in YAML configuration parsing. * box ga (#21907) * Bump base version for Postgres, MySQL, SQLServer (#21906) * Bump base version for Postgres, MySQL, SQLServer * Changelog * Lint * [Lustre] Fix device discovery for older versions of Lustre (<2.15.5) (#21901) * Add test for non-yaml output of lctl dl * Implement fallback to non-yaml version of lctl dl * Add changelog * Format * Assert for exact match of devices in test * Unify device list fixtures for readability * Improve code based on review * Remove redundant devices definition * Format * Minor syntax improvement * Add Process Signatures to Windows Integrations (#21909) * adding process signatures * adjusting signatures * adding another signature * adjusting signatures * Add owner field to agent-integrations manifest files (#21912) Adds 'owner': 'agent-integrations' field to 3 integration manifest.json files: - barracuda_secure_edge - cloudgen_firewall - zscaler_private_access This provides clear ownership tracking for these security and networking integrations as part of the initiative to add owner fields to all integration manifest.json files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <noreply@anthropic.com> * IDP-800: Add owner field to saas-integrations manifest files (#21913) * Add owner field to saas-integrations manifest files Adds 'owner': 'saas-integrations' field to 7 integration manifest.json files: - cofense_triage - crowdstrike_fdr - have_i_been_pwned - kandji - obsidian_security - push_security - zerofox_cloud_platform This provides clear ownership tracking for these SaaS security integrations as part of the initiative to add owner fields to all integration manifest.json files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Add owner field to obsidian_security and zerofox_cloud_platform These two integrations were missed in the previous commit due to different indentation formatting in their manifest.json files. --------- Co-authored-by: Claude <noreply@anthropic.com> * [AI-6169] DDS: Forescout Integration v1.0.0 (#21617) * Add forescout integration * Update test samples * Update changelog file * Update pipeline and samples * Add test sample results * Add images and manifest file * Logo update in dashboards * Address review comments * update dashboard panels name * Address review comments * Update logo images in dashboards * Resolve validate-dashboard CI check --------- Co-authored-by: akaila-crest <abhi.kaila@crestdata.ai> Co-authored-by: Thibault Krebs <150821137+thibaultkrebs@users.noreply.github.com> --------- Co-authored-by: Kyle Neale <kyle.neale@datadoghq.com> Co-authored-by: Brian Williams <bskinny@users.noreply.github.com> Co-authored-by: Aldrick Castro <aldrick.castro@datadoghq.com> Co-authored-by: Eva Parish <eva.parish@datadoghq.com> Co-authored-by: Eric Weaver <eweaver755@gmail.com> Co-authored-by: Seth Samuel <seth.samuel@datadoghq.com> Co-authored-by: apiazza-dd <120594144+apiazza-dd@users.noreply.github.com> Co-authored-by: dkirov-dd <166512750+dkirov-dd@users.noreply.github.com> Co-authored-by: Mohammad Rafi <36865458+mrafi97@users.noreply.github.com> Co-authored-by: davidfeng-datadog <david.feng@datadoghq.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Tirthraj Chaudhari <105789910+tirthrajchaudhari-crest@users.noreply.github.com> Co-authored-by: akaila-crest <abhi.kaila@crestdata.ai> Co-authored-by: Thibault Krebs <150821137+thibaultkrebs@users.noreply.github.com>
What does this PR do?
Currently
dbnameis not required, but will default to "postgres" if not provided in the config. The goal of this PR is to fix a config bug where we'll still try to connect to the "postgres" db if usingdatabase_autodiscoverywith "postgres" in the exclusion list.Currently the config below will result in still attempting to connect and query from the "postgres" database which might not exist or be in use on some providers
After the change this config will now result in a config validation error telling the user to either set
dbnameordatabase_autodiscovery.global_view_dbCurrently the config below will result in still attempting to connect and query from the "postgres" database which might not exist or be in use on some providers
After the change
dbnamewill becomemydbinstead of "postgresMotivation
This was identified as a bug via a customer support ticket
Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged