Blocklist to disable specific metric tags or metric names

[sungwy]

closes: #29663

This PR adds two additional configuration parameters:

statsd_disabled_tags:
      description: |
        If you want to avoid sending all the available metrics tags to StatsD,
        you can configure a blocklist of prefixes (comma separated) to filter out metric tags 
        that start with the elements of the list (e.g: "job_id,run_id")
      version_added: 2.6.0
      type: string
      example: job_id,run_id,dag_id,task_id
      default: job_id,run_id

statsd_block_list:
      description: |
        If you want to avoid sending all the available metrics to StatsD,
        you can configure a block list of prefixes (comma separated) to filter out metrics that
        start with the elements of the list (e.g: "scheduler,executor,dagrun").
        If statsd_allow_list and statsd_block_list are both configured, statsd_block_list is ignored
      version_added: 2.6.0
      type: boolean
      example: ~
      default: "False"

statsd_disabled_tags allows users to define a blocklist of tag name prefixes to disable.

statsd_invert_allow_list is a boolean parameter that converts the functionality of statsd_invert_allow_list from an allowlist to a blocklist.

The reason we need the metric name blocklist on top of a blocklist just for metric tags is because there are legacy metric names that have high cardinality under the definition explored in #29663 like: local_task_job.task_exit.<job_id>.<dag_id>.<task_id>.<return_code> that users might want to disable to reduce their metric storage costs.

[sungwy]

@hussein-awala @potiuk may I ask for your review on this PR? Thank you both very much for your input!

[potiuk]

I think it is very confusing as defined now. I think we should change "allow_list_validator" field into "list_validator" and have two implementations:

• AllowListValidator
• BlockListValidator

And let the user choose which validator to use (by name for example). The way where you invert meaning of a list is super confusing. IMHO

[sungwy]

I think it is very confusing as defined now. I think we should change "allow_list_validator" field into "list_validator" and have two implementations:

• AllowListValidator
• BlockListValidator

And let the user choose which validator to use (by name for example). The way where you invert meaning of a list is super confusing. IMHO

I see what you are saying @potiuk . I've made your suggested changes in stats.py. I've also introduced a separate statsd_block_list config parameter that expects a comma delimited string instead of statsd_invert_allow_list. Made a note on the config comment to note that if both statsd_allow_list and statsd_block_list are set, then statsd_block_list will be ignored in favor of statsd_allow_list.

[potiuk]

Much more straightforward now :). I'd love another committer to review it too.

[potiuk]

One small NIT. Probably we should have a warning in the logs if both are configured.

[potiuk]

Static checks need fixing.

[hussein-awala]

Why do we need to check if the tag is filtered here?

[sungwy]

@hussein-awala

Aren't we only concatenating metric tags that are allowed? (i.e. not disabled / not blocked)

If it's test(k) returns True, we will want to publish that metric tag

[hussein-awala]

My bad, I thought that this filter:

            tags_list = [
                f"{key}:{value}" for key, value in tags.items() if self.metric_tags_validator.test(key)
            ]

is used for the two clients datadog and influxdb, but just found that is just used for datadog, and if self.metric_tags_validator.test(k): is used for influxdb

[sungwy]

Yes - influxdb integration just adds the tag key:value pairs to the metric name with a comma-delimited standard, instead of having a separate Statsd client implementation that takes tags as a separate parameter.

[sungwy]

Static checks need fixing.

Fixed the checks and added the log based on your feedback! @potiuk