cached cloudwatch log group tags #540
Conversation
| DD_S3_BUCKET_NAME, self.CACHE_LOCK_FILENAME | ||
| ) | ||
| try: | ||
| file_content = cache_lock_object.get() |
There was a problem hiding this comment.
I'm not sure I fully understand how this s3 file is being used as a lock. My thought would be that there would be something to block on acquiring the lock, but I'm not seeing that
There was a problem hiding this comment.
To acquire the lock we create a lock s3 file, and then to remove the lock we delete it. If the file exists we know that the lock is active and there is a lambda running that's in the process of updating the tags. If the lock is active and we didn't grab it, then we don't need to update the tags in the current running lambda process as they'll be updated in the process that grabbed the lock.
aws/logs_monitoring/cache.py
Outdated
| """ | ||
| new_tags = {} | ||
| for log_group in self.tags_by_id.keys(): | ||
| new_tags[log_group] = get_log_group_tags(log_group) |
There was a problem hiding this comment.
Shouldn't we return False (first var) if there was an exception here similar to in LambdaCustomTagsCache?
I think if there was an exception from get_log_group_tags, we'd overwrite as empty tags which we don't want to do
There was a problem hiding this comment.
Here we're making multiple api calls to refill the cache, so I don't want to return false and not write to the cache if one of those api calls fails. I suppose if all the api calls fail we could return False here and skip writing to the s3 cache.
If the api call fails we can take the value we have stored from the local cache to avoid overwriting on a failed api call.
| log_group_tags = self.tags_by_id.get(log_group, None) | ||
| if log_group_tags is None: | ||
| log_group_tags = get_log_group_tags(log_group) | ||
| self.tags_by_id[log_group] = log_group_tags |
There was a problem hiding this comment.
Is this needed? I think we're updating self.tags_by_id in the _refresh function
There was a problem hiding this comment.
Right, we'll get here if we're encountering a log group that isn't in self.tags_by_id, so it'll be a log group we haven't gotten logs from before. In this case we grab tags for it and add it temporarily to self.tags_by_id. It'll get added to the s3 cache when we refresh. This lets us get tags for it right away rather than waiting for a refresh or refreshing all tags whenever we get 1 new log group.
|
Just to echo my comments in #531 (comment) - it seems that the existing |
|
Yep, all tag collection will be controlled by that argument. |
Wonderful - thanks @jvanbrie - reviewed that commit, makes this whole PR opt-in 👍 |
…ataDog/datadog-serverless-functions into jon.vanbriesen/cached_log_tags
|
@jvanbrie did you cut the release right? I can't see any of your change between the latest release and the past? aws-dd-forwarder-3.42.0...aws-dd-forwarder-3.43.0 Maybe I'm just having a slow day here? 😄. |
|
Something is definitely fishy here. Looks like the changes didn't make it into that release. I'll cut a new release tomorrow and see if that has the changes. |
|
Thanks for bringing this up, just released a new version and it grabbed the changes this time. aws-dd-forwarder-3.43.0...aws-dd-forwarder-3.44.0 |
No dramas @jvanbrie - agreed - can deff see the changes in there now! |
What does this PR do?
This pr separates out our caching logic and creates a new cache specifically for cloudwatch log tags.
Motivation
The caching logic was built around the lambda custom tags use case, and that isn't great for future caching use cases. In addition we've gotten request for support for cloudwatch log group tags.
Testing Guidelines
Lambdas are running this new code just fine. We can see the s3 caches updating as well.
Additional Notes
Types of changes
Check all that apply