-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 - log and metric tweaks #4865
azure - log and metric tweaks #4865
Conversation
c855895
to
8fd2ebe
Compare
extra=self._get_action_log_metadata(resource)) | ||
|
||
def _get_action_log_metadata(self, resource): | ||
action = self.__class__.__name__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to use c7n registered action name rather than Python class name
policy.ctx.metrics.put_metric( | ||
'ResourceCount', len(resources), 'Count', Scope="Policy", | ||
buffer=False) | ||
|
||
policy.ctx.metrics.put_metric( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should upload metrics even if no resources were found (like 272 has early exit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah pull mode does this, it breaks from lambda implementation further I think but that is fine. Fixing.
@staticmethod | ||
def _get_event_action(): | ||
action = TestEventAction() | ||
action.client = MagicMock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: feels like it can be done in __init__()
for test classes
Some logging and metrics improvements.
ResourceTime
andActionTime
metrics to event based policy executionlog.error
calls which should belog.exception
calls (lots more to do here)I added a set of tests for the base action to cover logging and the new message result behavior, but this is still likely to be a patch coverage exception due to the number of exception handlers I updated which may not be necessary to test individually.