Log delivery to CloudWatch Logs in provider account #36
Conversation
|
One thing I can't tell from this PR is what happens to logs emitted by the plugin itself? We probably want those logs emitted into the handler's provisioned platform account, but won't want logs from the end-developer's plugin emitted into the platform account. We do however probably want logs from the plugin emitted to the provider's account so they can troubleshoot. |
At present the log handler attaches to the root logger, so all logs go to provider, the existing (default lambda logger) is not modified/removed, so provider logs also go to the platform. Will experiment with altering the logger namespace and detaching the default log handler from provider to drop customer logs from being sent to the platform. One exception I see here is that we're trapping errors in the plugin and logging tracebacks in the plugin. This is really useful for troubleshooting, but will result in exceptions raised by customer code delivering tracebacks into the platform cwl. |
| logging.Handler.__init__(self) | ||
| self.group = group | ||
| self.stream = stream.replace(":", "__") | ||
| self.client = boto3.client("logs", **creds) |
There was a problem hiding this comment.
dependency injection of the client would make testing easier i think
There was a problem hiding this comment.
the mock_provider_handler fixture is 14 lines long, which covers patching the boto client for all tests, unless you feel strongly on this, I'm inclined to leave well enough alone.
There was a problem hiding this comment.
i only really considered it because it would mean you wouldn't have to pass credentials around. the client could be instantiated in setup directly
|
| try: | ||
| log_creds = event_data["requestData"]["providerCredentials"] | ||
| except KeyError: | ||
| log_creds = {} | ||
| try: | ||
| log_group = event_data["requestData"]["providerLogGroupName"] | ||
| except KeyError: | ||
| log_group = "" |
There was a problem hiding this comment.
if any of these KeyErrors happen, line 54 (if log_creds and log_group:) can never be true, so could simply bail out while logging an error?
Issue #, if available: #21
Description of changes:
Enables simple log delivery to the provider account.
Log stream names are created as
f"{arn.replace(':', '__')}/{LogicalResourceId}"orf"{CallerAccountId}-{region}/{action}if arn or logical_id is not present in the request (suspect this is the case withlistcalls)Doesn't do any batching/queuing of messages to cwl which would ensure ordering and handle throttles. this can be added in a future PR.
It also doesn't redirect stdout (
print, etc) to the log handler, so only log messages created vialoggingmodule will be sent to provider.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.