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

Disable generic tags #10027

Merged
merged 22 commits into from
Sep 9, 2021
Merged

Disable generic tags #10027

merged 22 commits into from
Sep 9, 2021

Conversation

hithwen
Copy link
Contributor

@hithwen hithwen commented Sep 2, 2021

Alternate to #9853.
Adds a hidden feature flag to replace generic tags like "cluster" with non generic tags like "snowflake_cluster". This tag will be shown (by specs override) on affected integrations only.

In this model we only add one flag for disabling generic tags and the functionality of check_generic_tags would be added only to the affected integrations.

When reviewing focus on:

  • datadog_checks_base/datadog_checks/base/checks/base.py
  • datadog_checks_dev/datadog_checks/dev/tooling/templates/configuration/instances/default.yaml
  • datadog_checks_base/tests/base/checks/test_agent_check.py

The rest are updated config models

Notes:

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #10027 (54fd380) into master (fce3de3) will increase coverage by 0.00%.
The diff coverage is 94.44%.

Flag Coverage Δ
btrfs 82.91% <ø> (ø)
cloud_foundry_api 95.98% <ø> (+0.12%) ⬆️
crio 100.00% <ø> (ø)
external_dns 100.00% <ø> (ø)

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

Copy link
Contributor

@coignetp coignetp left a comment

Choose a reason for hiding this comment

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

I prefer this option, because it needs only one config flag, and new customers get rid of generic tags faster.

However, if I understand correctly, there is one use case we won't cover anymore ; we won't be able to consider more tags as generic without affecting existing users. If 2 months after merging this, we want to consider url tag as generic, they won't have any option to have <integration_name>_cluster tag and url tag at the same time.
I don't think it's such a big issue, since we probably won't update the generic tag list, but it's something we should be aware of.

@hithwen
Copy link
Contributor Author

hithwen commented Sep 2, 2021

we won't be able to consider more tags as generic without affecting existing users.

With the previous implementation there were these cases:

  • check & disable : only new tag would be emitted
  • !check & disable: only new tag would be emitted
  • ! check & !disable: only old tag would be emitted
  • check & !disable: both tags would be emitted

With the current implementation:

  • !disable: old tag would be emitted
  • disable: new tag would be emitted

But then I'll edit affected integrations to always emit new tag so effectively check would be enabled for them.

In both implementations the only way to add more tags is to edit the list

@hithwen hithwen merged commit 6a7abf9 into master Sep 9, 2021
@hithwen hithwen deleted the js/generic-tags-feature-2 branch September 9, 2021 13:00
github-actions bot pushed a commit that referenced this pull request Sep 9, 2021
* Transform generic tags
* Add config option
* Sync models

Co-authored-by: Paul <[email protected]> 6a7abf9
@ofek ofek changed the title Disable generic tags - option 2 Disable generic tags Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/tooling integration/amazon_msk integration/apache integration/avi_vantage integration/cassandra_nodetool integration/cassandra integration/consul integration/datadog_checks_base integration/datadog_checks_dev integration/datadog_cluster_agent integration/directory integration/elastic integration/envoy integration/gitlab_runner integration/glusterfs integration/go_expvar integration/gunicorn integration/haproxy integration/harbor integration/http_check integration/ibm_mq integration/istio integration/kafka_consumer integration/kafka integration/mapr integration/mcache integration/mesos_master integration/mesos_slave integration/mysql integration/nagios integration/network integration/nfsstat integration/nginx_ingress_controller integration/nginx integration/openldap integration/oracle integration/postfix integration/postgres integration/powerdns_recursor integration/process integration/proxysql integration/rabbitmq integration/redisdb integration/riak integration/riakcs integration/sap_hana integration/sonarqube integration/spark integration/sqlserver integration/squid integration/ssh_check integration/statsd integration/supervisord integration/system_core integration/system_swap integration/tcp_check integration/teamcity integration/tls integration/tomcat integration/twemproxy integration/twistlock integration/varnish integration/vault integration/vertica integration/voltdb integration/vsphere integration/win32_event_log integration/windows_service integration/wmi_check integration/yarn integration/zk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants