feat(tag-filtering): enable tag filtering metrics #21
Conversation
|
Hello |
|
Hi @pavelsidla, thank you for this nice PR! I think it is indeed helpful. Maybe later we can make it more generic and work for other kinds of filters (e.g., My key suggestion is to make the configuration like the following, what do you think? metrics:
tag_filters:
- tag_key: 'my_org:team' # because it is possible to have such tags in AWS, we may need this config format to support them
tag_values:
- dev-team-1
- dev-team-2
- tag_key: EnvironmentName
tag_values:
- dev |
|
Hello i made change in code to allign it with your requirements. Please check if its ok :) |
There was a problem hiding this comment.
Hi @pavelsidla, thanks for the updates! There are some minor issues regarding the validation and a for loop. Please check my comments. I think the PR is good to be merged after fixing them :)
| # Include tag filters if provided | ||
| if tag_filters: | ||
| tag_filter_list = [] | ||
| for tag_filter in tag_filters: |
There was a problem hiding this comment.
I believe this for loop should be put under the if block, right?
| @@ -26,6 +26,11 @@ metrics: | |||
| tag_value: other | |||
| # Allowed values for metric type are AmortizedCost, BlendedCost, NetAmortizedCost, NetUnblendedCost, NormalizedUsageAmount, UnblendedCost, and UsageQuantity | |||
| metric_type: AmortizedCost | |||
| tag_filters: | |||
There was a problem hiding this comment.
Could you also update this config example? Thanks :)
| @@ -46,6 +51,11 @@ metrics: | |||
| tag_value: other | |||
| # Allowed values for metric type are AmortizedCost, BlendedCost, NetAmortizedCost, NetUnblendedCost, NormalizedUsageAmount, UnblendedCost, and UsageQuantity | |||
| metric_type: AmortizedCost | |||
| tag_filters: | |||
There was a problem hiding this comment.
Same here, needs to be updated.
| # Validate tag_filters if present | ||
| if "tag_filters" in config_metric: | ||
| tag_filters = config_metric["tag_filters"] | ||
| if not isinstance(tag_filters, dict): |
There was a problem hiding this comment.
This check should be if not isinstance(tag_filters, list) now.
| for group in config_metric["group_by"]["groups"]: | ||
| if group["label_name"] in group_label_names: | ||
| logging.error("Group label names should be unique!") | ||
| for tag_key, tag_values in tag_filters.items(): |
There was a problem hiding this comment.
Please update this validation as well.
|
Hello i am sorry for mistakes ill fix them tomorrow, and thank you for make that suggestions! |
Enabling tag based filtering metrics