Internally compile the include patterns in the autodiscovery feature
#14768
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
FlorentClarret
changed the title
include patterns in the discovery featureinclude patterns in the autodiscovery feature
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
jose-manuel-almaza
requested changes
Jun 15, 2023
| d = Discovery(mock_get_items, include={'a.*': None, 'b.*': None}) | ||
| assert list(d.get_items()) == [('a.*', 'a', 'a', None), ('b.*', 'b', 'b', None)] | ||
| assert mock_get_items.call_count == 1 | ||
| assert d._filter._compiled_include_patterns == {p: re.compile(p) for p in ['a.*', 'b.*']} |
There was a problem hiding this comment.
You are refactoring and not adding new functionality. In that case no new unit tests should be needed, and the proof of that is that you are checking private properties of the class that might be refactored in the future and force you to change your unit tests then, when that shouldn't be necessary.
There was a problem hiding this comment.
Sounds good to me. I think we can keep the other test even if it's not directly related to my modification, what do you think?
There was a problem hiding this comment.
It is very similar to test_include_not_empty, but yes... I think we can keep it
There was a problem hiding this comment.
so similar that we can merge them together! Done. Thanks
ofek
approved these changes
Jun 15, 2023
jose-manuel-almaza
approved these changes
Jun 15, 2023
FlorentClarret
deleted the
florentclarret/base_check/discovery_compile_include
branch
aquiladayc
added a commit
that referenced
this pull request
Jun 28, 2023
remove lines Update CHANGELOG.md Draft CHANGELOG.md Update metadata.csv Update metadata.json Update conftest.py Update CHANGELOG.md Remove change log Update ecs_fargate.py Add __init__ function Fix ecs_fargate.py and add tests Add new performance counter metrics (#14625) * Add new cluster metrics * Add host and VM metrics * Add realtime fixtures * update metadata.csv * Add throughput metric Move cancel waiting logic to test functions for DBMAsyncJob (#14773) * init commit * revert changes to sqlserver, mysql, utils tests Add validations for removed dependencies (#14556) * Map out new licenses validation * Implement validations for extra licenses * Add constants to config.toml * Implement license validation * Uncomment legacy licenses validation * Keep license command addition in same place * Small style change * Update config.toml override values * Refactor * Fix style * Apply suggestions from code review Co-authored-by: Ofek Lev <ofekmeister@gmail.com> * Update suggestions * Require CI for license validation tests and update to use empty envvars * Fix permission for file * Add windows version of setting github env vars * Fix windows file * Change to powershell script * Output GITHUB_ENV on windows CI * Convert entirely to powershell * Change back to bat file * Test DD_GITHUB_USER value * Print github user in license test * Manually set Github user and token in test * Fix config_file * Print github user * Check if tokens are the same * Remove additional space in bat script * Fix style and remove test code * Change order of scripts * Try commenting out model.github override * Revert previous commit * Change to threads instead of async * Switch out async request to requests * Clean up * Fix style --------- Co-authored-by: Ofek Lev <ofekmeister@gmail.com> DOCS-5656 gke setup links to operator/helm (#14746) dbm-oracle-dashboard (#14736) Internally compile the `include` patterns in the autodiscovery feature (#14768) * Internally compile the `include` patterns in the discovery feature * address Use Git for versioning (#14778) Upgrade Pydantic model code generator (#14779) * Upgrade Pydantic model code generator * address build standalone binaries for ddev (#14774) Remove `pyperclip` dependency and clipboard functionality (#14782) [Release] Bumped datadog_checks_dev version to 20.0.0 (#14784) * [Release] Bumped datadog_checks_dev version to 20.0.0 * [Release] Update metadata Bump the minimum version of datadog-checks-dev (#14785) update license path (#14783) Allow all projects to be collected in REST implementation (#14433) * Bug Fix Teamcity rest with all projects * . Rewrite Postgres size query and add `postgresql.relation.{tuples,pages,all_visible}` + toast_size metrics (#14500) * Bumped dependency version * Rewrite size metric query Use new query executor Split toast size from table size Add partition_of tag Add pages/tuples/allvisible metrics Optimise query to minimise stat calls * Add partitioned test tables * Update metadata with new metric * Fix version check * Fix wal_level for tests [SNMP] Add metadata for traps telemetry metrics (#14769) * Add metadata for traps telemetry metrics * Remove commas from desc 🤦 * Add units Temporarily disable py2 tests on PRs (#14793) fix(redisdb): return len of stream instead of 1 (#14722) Currently the code compute the len of the stream but report always 1 This change fixes this. Update Netflow dashboard (#14794) * update Netflow dashboard * remove datadog_demo_keep:true * avg -> sum * rename dashboard revert manifest.json (#14797) Add User Profiles support (#14752) Remove Content (#14766) Update wording and add extra install directions for ODBC (#14781) * Update wording and add extra install directions for ODBC * Update README.md Add ability to choose tag to append to VM hostname (#14657) * Add ability to choose tag to append to VM hostname * Add a test for integration tags * Sort list * Change log to debug * Fix style * Allow user to choose a datadog tag for vm hostname Fix ability to release ddev (#14790) Disable server info and version collection when collect_server_info is false (#14610) * if collect_server_info is set to false disable server info and version collection * the collect metadata in check.py has to check for the collect_server_info before attempting to collect server info, even when the base url is well formated * add testing to see if metadata is collected when collect_server_info is false * add testing to see if metadata is collected when collect_server_info is false * fix typo * fix typo * commit * commit * commit * commit typo * fix check.py [Release] Bumped datadog_checks_dev version to 20.0.1 (#14806) * [Release] Bumped datadog_checks_dev version to 20.0.1 * [Release] Update metadata [Release] Bumped ddev version to 3.0.0 (#14807) * [Release] Bumped ddev version to 3.0.0 * [Release] Update metadata fix build flake for ddev (#14808) Fix ddev platform installers and releasing (#14812) Bump postgres integration to Python 3 (#14813) update changelog generation (#14810) Update ecs_fargate/tests/fixtures/metadata_v4.json Co-authored-by: Cedric Lamoriniere <cedric.lamoriniere@datadoghq.com> Update ecs_fargate/tests/fixtures/stats_linux_v4.json Co-authored-by: Cedric Lamoriniere <cedric.lamoriniere@datadoghq.com> Update test_unit_v4.py Update license term Update test_unit_v4.py update license format Update metadata.csv Fix unit name Set the `marker` option to `not e2e` by default (#14804) Add profile for hp-ilo (#14771) * add profile for hp-ilo * add tests for new hp-ilo profile * fix linter * hp-ilo4 extends hp-ilo * delete unnecessary product_name field * lint * move hp-ilo to default-profiles Update profiles with missing devices (#14695) * update cisco-asr * update cisco-catalyst-wlc * update cisco-catalyst * update cisco-legacy-wlc * update cisco-nexus * update dell-poweredge * update juniper-ex * update juniper-mx * add cisco-isr * add models + move cisco5700WLC to cisco-catalyst-wlc * move cisco-isr to default-profiles Add profile 3com-huawei (#14694) Revert "Set the `marker` option to `not e2e` by default (#14804)" (#14815) This reverts commit 3f4c885. Sort assert_device_metadata tags (#14816) Add per vendor generic profiles (#14721) * add dell generic profile * add fortinet generic profile * add juniper generic profile * move vendor profiles to default-profiles * add test for cisco * add test for dell * add test for fortinet * add test for juniper * linter * linter Update formatting for changelogs (#14814) * Update formatting for changelogs * Update formatting for changelogs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
The autodiscovery feature added in this PR allows us to provide an
includepattern as a string or directly as a regular expression. If the pattern is a string, we do not compile it and always forward the string tore.searchwhich in turn will compile the string and use an internal cache. This PR pre-compiles the string in the discovery class to avoid multiple compilations of the same pattern.I also added a test to ensure the filter is working as expected if we directly provide a compile regular expression.
Motivation
Even if there's an internal cache, I think we should compile them on our side to be completely sure they're not going to be recompiled each time the check runs
Additional Notes
I'm going to use this feature in this PR
The autodiscovery feature is already used in the cloudera integration
This PR is a proposition, let me know what you think!
Review checklist (to be filled by reviewers)
changelog/andintegration/labels attachedqa/skip-qalabel.