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

azure - metric filter - support for child resources #4743

Merged
merged 14 commits into from Sep 12, 2019

Conversation

@jomalsan
Copy link
Contributor

commented Sep 6, 2019

The following is a proposal for how we can extend the current MetricFilter to make it easy to support child resources. It also includes an example of a metric filter for cosmosdb-databases. This can be tested with the following yaml policy:

policies:
  - name: cosmosdb-database-metric
    resource: azure.cosmosdb-database
    filters:
      - type: metric
        metric: TotalRequestUnits
        op: le
        aggregation: total
        threshold: 1000
        timeframe: 24

The core of the proposed change is refactoring the portion of MetricFilter.get_metric_data that changes between child and parent resources into a function. This way child resources can implement their own metric filter by inheriting from MetricFilter and overriding the method (get_metric_data) needed to implement their custom logic. An example of this is included for CosmosDb.Database

Please comment and ask further questions. I have raw notes going over the different solutions to this problem that I am happy to share, but the proposed solution here seems to be the best. Once everyone has agreed on an implementation I will extend the methodology for other child resources, include tests and new example documentation.

@stefangordon
Copy link
Collaborator

left a comment

Yeah this will be a big improvement. Thanks for writing it up!

"""CosmosDB Database Metric Filter
"""
def __init__(self, data, manager=None):
super().__init__(data, manager)

This comment has been minimized.

Copy link
@stefangordon

stefangordon Sep 6, 2019

Collaborator

Looks like we can drop __init__ entirely?

This comment has been minimized.

Copy link
@jomalsan

jomalsan Sep 7, 2019

Author Contributor

Removed

@@ -212,6 +205,16 @@ def get_metric_data(self, resource):
self._write_metric_to_resource(resource, metrics_data, m)

return m

def get_metric_data(self, resource):

This comment has been minimized.

Copy link
@stefangordon

stefangordon Sep 6, 2019

Collaborator

Did you consider breaking out a get_filter()? Seems like in most cases the override could be a single line that just sets that, especially if we handle adding the and automatically ('and'.join((self.get_filter(), self.filter)))

This comment has been minimized.

Copy link
@logachev

logachev Sep 6, 2019

Collaborator

a little confusing what is the difference between self.filter and self.get_filter()..
Maybe create a subclass for all Child resource metrics and use self.parent_filter instead of `self.get_filter() ?

This comment has been minimized.

Copy link
@jomalsan

jomalsan Sep 6, 2019

Author Contributor

I thought about breaking out get_filter() but the override in the child class needs to modify the resource id field that is getting passed in and the filter field. This way we maintain one function that needs to be overridden instead of two.

@logachev I think the confusion might be that in this context self.filter is an Azure SDK concept and has nothing to do with the c7n concept of filter. An example for Cosmos DB databases would be self.filter = "DatabaseName eq 'testdatabase'" which only returns metrics for the given database

This comment has been minimized.

Copy link
@stefangordon

stefangordon Sep 10, 2019

Collaborator

Still feels weird to me to do this thing on each child as we'll have lots of places to update it if the API or our functionality changes:

class CosmosDBCollectionMetricFilter(MetricFilter):
    def get_metric_data(self, resource):
        if self.filter is None:
            parent_filter = "CollectionName eq '%s'" % resource['id']
        else:
            parent_filter = "%s and CollectionName eq '%s'" % (self.filter, resource['id'])
        return self.client.metrics.list(
            resource['c7n:parent-id'],
            timespan=self.timespan,
            interval=self.interval,
            metricnames=self.metric,
            aggregation=self.aggregation,
            filter=parent_filter
        )

Seems like worst case you could do this instead on each resource with less changes to the base and subclass

class CosmosDBCollectionMetricFilter(MetricFilter):
    def __init__(self, data, manager=None):
        super(CosmosDBCollectionMetricFilter, self).__init__(data, manager)
        self.filter = ' and '.join(("CollectionName eq '%s'" % resource['id'], self.filter))
        self.id_override =  resource['c7n:parent-id']

Then in the base you only have to change resource['id'] to self.id_override or resource['id']

Or you could put the string modification in the base and just set the one field (all children have the resource['c7n:parent-id']) so you can build the two fields you need if you set this:

class CosmosDBCollectionMetricFilter(MetricFilter):
    def __init__(self, data, manager=None):
        super(CosmosDBCollectionMetricFilter, self).__init__(data, manager)
        self.parent_filter_dimension = 'CollectionName'

This comment has been minimized.

Copy link
@stefangordon

stefangordon Sep 10, 2019

Collaborator

Another option could be just having the base check is_instance(self.manager.resource_type, ChildResourceManager) and then not subclassing at all, instead adding a metrics_dimension field to the resource type for each child so you can use self.manager.resource_type.metrics_dimension and self.manager.resource_type.parent_key to get what you need automatically.

@stefangordon stefangordon removed this from In progress in azure Sep 6, 2019

@@ -212,6 +205,16 @@ def get_metric_data(self, resource):
self._write_metric_to_resource(resource, metrics_data, m)

return m

def get_metric_data(self, resource):

This comment has been minimized.

Copy link
@logachev

logachev Sep 6, 2019

Collaborator

a little confusing what is the difference between self.filter and self.get_filter()..
Maybe create a subclass for all Child resource metrics and use self.parent_filter instead of `self.get_filter() ?

if self.filter is None:
parent_filter = "DatabaseName eq '%s'" % resource['id']
else:
parent_filter = "%s and DatabaseName eq '%s'" % (self.filter, resource['id'])

This comment has been minimized.

Copy link
@logachev

logachev Sep 6, 2019

Collaborator

I think we need add parentheses: (%s) and (DatabaseName eq '%s')

This comment has been minimized.

Copy link
@jomalsan

jomalsan Sep 7, 2019

Author Contributor

Parentheses are invalid in the filter syntax: https://docs.microsoft.com/en-us/rest/api/monitor/filter-syntax
It looks like the only "complex" logic that is allowed is the "and" operator (not even "or" works), so they just assume that it is assertion1 and assertion2 and ...

jomalsan added 2 commits Sep 7, 2019
@stefangordon

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

I think just add a little test to get coverage for that new subclass and mark it as ready for review. Either a unit test on that one method with a mock on the list call, or just a new policy/cassette on this resource type.

@jomalsan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

I think just add a little test to get coverage for that new subclass and mark it as ready for review. Either a unit test on that one method with a mock on the list call, or just a new policy/cassette on this resource type.

I plan on adding the extended subclass to support cosmos db containers as well. Then tests for both and add the examples for both of them

@jomalsan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Added child metric for cosmosdb-collections and added tests for both of the new filters.

The cassettes will have to be re-recorded after PR #4748 is merged

jomalsan added 5 commits Sep 9, 2019

@jomalsan jomalsan marked this pull request as ready for review Sep 10, 2019

jomalsan added 2 commits Sep 11, 2019
jomalsan and others added 2 commits Sep 12, 2019

@stefangordon stefangordon merged commit e7197f1 into cloud-custodian:master Sep 12, 2019

15 checks passed

Custodian - CI Build #20190912.7 succeeded
Details
Custodian - CI (Container Cask) Container Cask succeeded
Details
Custodian - CI (Docs) Docs succeeded
Details
Custodian - CI (Lint) Lint succeeded
Details
Custodian - CI (Test Python27) Test Python27 succeeded
Details
Custodian - CI (Test Python36) Test Python36 succeeded
Details
Custodian - CI (Test Python37) Test Python37 succeeded
Details
Custodian - CI (Test Win2019 Python36) Test Win2019 Python36 succeeded
Details
Custodian - CI (Test Win2019 Python37) Test Win2019 Python37 succeeded
Details
codecov/patch 91.67% of diff hit (target 89.47%)
Details
codecov/project/azure 91.87% (-<.01%) compared to d7afef7
Details
codecov/project/custodian 90.53% (+0.01%) compared to d7afef7
Details
codecov/project/gcp 91.25% remains the same compared to d7afef7
Details
codecov/project/k8s 93.72% remains the same compared to d7afef7
Details
codecov/project/mailer 62.13% remains the same compared to d7afef7
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.