feat/aws/inactive cloudwatch log group#19
feat/aws/inactive cloudwatch log group#19gabrielecalafange wants to merge 23 commits intointegrationfrom
Conversation
Pull Request DescriptionHello team, I hope this message finds you well. I’m a Computer Science student at the Federal University of Campina Grande (Brazil), and part of a research team working on FinOps initiatives. At our Distributed Systems Laboratory (LSD), we’ve been contributing to the evolution of OptScale. This pull request introduces a new AWS cost-optimization recommendation for identifying Inactive CloudWatch Log Groups. CloudWatch Logs can silently accumulate storage costs over time. When log groups have no retention policy configured and exhibit no recent activity, they effectively become unused storage. This recommendation helps maintain observability hygiene and reduce unnecessary CloudWatch expenses by detecting log groups that are safe cleanup candidates. Recommendation BehaviorA CloudWatch Log Group is considered inactive when all of the following conditions are met:
Implementation details
UI UpdateAdjusted cost-savings formatting: values below USD $1 are now displayed as "<$1" instead of $0, to avoid misleading perception of zero impact and improve clarity in cost-impact reporting. Known LimitationsPricing values are hard-coded and limited to one AWS region Next Steps
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for CloudWatch Log Groups as a new discoverable resource type in AWS, including metrics collection, cost estimation, and recommendations for identifying inactive log groups.
- Added LogGroupResource model with CloudWatch metrics support (ingestion, storage, query)
- Implemented AWS discovery methods for log groups with parallel metrics fetching
- Created recommendation module to identify inactive log groups and estimate potential savings
- Added UI components and translations for the new recommendation type
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/cloud_adapter/model.py | Added LogGroupResource class and log_group enum value; changed string quotes to double quotes |
| tools/cloud_adapter/clouds/aws.py | Implemented log group discovery, metrics collection, and helper methods for AWS CloudWatch |
| rest_api/rest_api_server/tests/unittests/test_discovery_infos_api.py | Updated test to expect 7 resource types instead of 6 |
| rest_api/rest_api_server/controllers/base.py | Added logic to populate resource name from meta.name if missing |
| rest_api/rest_api_server/alembic/versions/*.py | Added log_group to database enum types for discovery_info table |
| ngui/ui/src/translations/en-US/app.json | Added translation keys for inactive CloudWatch log groups UI |
| ngui/ui/src/hooks/useOptscaleRecommendations.ts | Registered new recommendation component |
| ngui/ui/src/containers/RecommendationsOverviewContainer/recommendations/*.tsx | Added InactiveCloudWatchLogGroup recommendation component |
| ngui/ui/src/components/FormattedMoney/useMoneyFormatter.ts | Refactored money formatter with TypeScript types; changed < $0.01 to < $1 threshold |
| docker_images/resource_discovery/worker.py | Enhanced error handling for discovery calls with exception logging |
| diworker/diworker/migrations/20250417123000_cloudwatch_metrics.py | Created ClickHouse table for CloudWatch metrics storage |
| diworker/diworker/importers/base.py | Added methods to save CloudWatch metrics to ClickHouse |
| diworker/diworker/importers/aws.py | Implemented log group metrics processing and persistence |
| bumiworker/bumiworker/modules/recommendations/inactive_cloud_watch_log_group.py | Core recommendation logic for detecting inactive log groups and calculating savings |
| bumiworker/bumiworker/modules/pricing/aws_cloudwatch.py | AWS CloudWatch Logs pricing constants |
| bumiworker/bumiworker/modules/module.py | Minor string formatting updates |
| bumiworker/bumiworker/modules/base.py | Renamed time_measure to TimeMeasure (PEP8 compliance) |
| bumiworker/bumiworker/modules/archive/inactive_cloud_watch_log_group.py | Archive logic for inactive log group recommendations |
Comments suppressed due to low confidence (2)
bumiworker/bumiworker/modules/base.py:109
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedImages._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method S3AbandonedBucketsArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedInstances._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedKinesisStreams._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedLBs._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InactiveCloudWatchLogGroup._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ArchiveInactiveUsersBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InsecureSecurityGroups._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstanceGenerationUpgrade._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstanceMigration._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstanceSubscription._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstancesForShutdown._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method NebiusMigration._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ObsoleteImages._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ObsoleteIps._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ObsoleteSnapshotsArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ReservedInstancesArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method RightsizingArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method S3PublicBuckets._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ShortLivingInstances._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method StuckInStateForALongTimeBase._get matches the call.
def _get(self):
bumiworker/bumiworker/modules/base.py:149
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedImages._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method S3AbandonedBucketsArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedInstances._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedKinesisStreams._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method AbandonedLBs._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InactiveCloudWatchLogGroup._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ArchiveInactiveUsersBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InsecureSecurityGroups._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstanceGenerationUpgrade._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstanceMigration._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstanceSubscription._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InstancesForShutdown._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method NebiusMigration._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ObsoleteImages._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ObsoleteIps._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ObsoleteSnapshotsArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ReservedInstancesArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method RightsizingArchiveBase._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method S3PublicBuckets._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method ShortLivingInstances._get matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method StuckInStateForALongTimeBase._get matches the call.
def _get(self):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rest_api/rest_api_server/alembic/versions/add_log_group_to_discovery_info.py
Outdated
Show resolved
Hide resolved
| return ca.get("id") or ca.get("_id") | ||
| return "" | ||
|
|
||
| def _get(self): |
There was a problem hiding this comment.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method InactiveCloudWatchLogGroup._get matches the call.
| def _get(self): | |
| def _get(self, *args, **kwargs): |
| return pool_id | ||
| return optimization.get("pool_id") | ||
|
|
||
| def _get(self, previous_options, optimizations, cloud_accounts_map, **kwargs): |
There was a problem hiding this comment.
This method requires 4 positional arguments, whereas overridden ServiceBase._get requires 1. This call correctly calls the base method, but does not match the signature of the overriding method.
This method requires 4 positional arguments, whereas overridden ModuleBase._get requires 1. This call correctly calls the base method, but does not match the signature of the overriding method.
This method requires 4 positional arguments, whereas overridden InactiveCloudWatchLogGroup._get requires 1. This call correctly calls the base method, but does not match the signature of the overriding method.
This method requires 4 positional arguments, whereas overridden ArchiveBase._get may be called with 1. This call correctly calls the base method, but does not match the signature of the overriding method.
This method requires 4 positional arguments, whereas overridden ArchiveBase._get may be called with arbitrarily many. This call correctly calls the base method, but does not match the signature of the overriding method.
| def _get(self, previous_options, optimizations, cloud_accounts_map, **kwargs): | |
| def _get(self, previous_options, **kwargs): | |
| optimizations = kwargs.get("optimizations") | |
| cloud_accounts_map = kwargs.get("cloud_accounts_map") | |
| if optimizations is None or cloud_accounts_map is None: | |
| raise ValueError("Missing required keyword arguments: 'optimizations' and/or 'cloud_accounts_map'") |
diworker/diworker/migrations/20250417123000_cloudwatch_metrics.py
Outdated
Show resolved
Hide resolved
bumiworker/bumiworker/modules/recommendations/inactive_cloud_watch_log_group.py
Outdated
Show resolved
Hide resolved
1ca70dc to
cadde8d
Compare
## Description Change available filters API behavior ## Related issue number OSN-1135 ## Special notes <!-- Please provide additional information if required. --> ## Checklist * [ ] The pull request title is a good summary of the changes * [ ] Unit tests for the changes exist * [ ] New and existing unit tests pass locally
- Fix incorrect redirect filters in table data - Now we get available meta keys based on applied filters using the available_filters api - Added toggle to apply selected categorization as an additional meta filter - Show null meta as is, without converting to "(not set)" label
- removed multiplying on node cpu and ram gb numbers (as hourly_cost is a cost for ALL cpu and ram) - added currency support - changed default cost_model costs
fix: linter errors fix: linter errors fix: linter errors fix: linter errors
fix: bumiworker tests fix: bumiworker tests ngui lint fix: imports lint fix: lint errors bumiworker and ngui fix: ordering app.json revert file revert files before lint
19c1d72 to
13cf5e2
Compare
| """ | ||
| cutoff_recent_window = self._utc_now() - timedelta(days=days_threshold) | ||
| total = 0 | ||
| for m in series or []: |
There was a problem hiding this comment.
Change these letter variables for complete and descriptive words
| """ | ||
| cutoff_recent_window = self._utc_now() - timedelta(days=RECENT_WINDOW_DAYS) | ||
| total = 0 | ||
| for m in series or []: |
There was a problem hiding this comment.
Change these letter variables for complete and descriptive words
marianezei
left a comment
There was a problem hiding this comment.
I would carefully review Copilot's recommendations. Perhaps it would be good to accept them.
nice work everyone!!
| item = { | ||
| 'cloud_resource_id': r.get('resource_id'), | ||
| 'resource_id': r.get('resource_id'), | ||
| 'resource_name': r.get('name'), | ||
| 'log_group_name': r.get('log_group_name'), | ||
| 'cloud_account_id': r.get('cloud_account_id'), | ||
| 'cloud_type': ca_info.get('type'), | ||
| 'cloud_account_name': ca_info.get('name'), | ||
| 'region': r.get('region'), | ||
| 'owner': {"id": None, "name": None}, | ||
| 'pool': {"id": r.get("pool_id"), "name": None, "purpose": None}, | ||
| 'is_excluded': is_excluded, | ||
| 'retention_in_days': r.get('retention_in_days'), | ||
| 'stored_bytes': int(r.get('stored_bytes', 0) or 0), | ||
| 'detected_at': self.created_at, | ||
| 'storage': int(r.get('stored_bytes', 0) or 0), | ||
| 'ingestion': int(ingestion_occurrences), | ||
| 'query': int(query_occurrences), | ||
| 'saving': saving, | ||
| } |
There was a problem hiding this comment.
maybe we can import this from a file where contains these kind of info. in fact...other recommendations may/might need this.
what do you think?
| filter_with_tags = response.get('filter_values', {}).get('tag') | ||
| filter_without_tags = response.get('filter_values', {}).get('without_tag') | ||
| self.assertListEqual(filter_with_tags, []) | ||
| self.assertListEqual(filter_with_tags, ['hello']) |
| cloud_accounts_map) | ||
| self._archive_data(res, previous['created_at']) | ||
| except Exception as ex: | ||
| except Exception as ex: # pylint: disable=broad-exception-caught |
There was a problem hiding this comment.
this comment needs to stay in the file?
| continue | ||
|
|
||
| response = self._aggregate_resources(ca_id) | ||
| for r in response: |
There was a problem hiding this comment.
one letter doesn't explain well what is this variable, I think we can change to something better, right?
There was a problem hiding this comment.
One case that one letter can be understandable is in a list comprehension
| x.get('cloud_account_id') is not None and | ||
| # RIFee is created once a month and is updated every day | ||
| # RIFee is created once a month and is updated every | ||
| # day |
There was a problem hiding this comment.
Why another line to write one single word? lint problems?
No description provided.