-
Couldn't load subscription status.
- Fork 4.3k
feat(glue-alpha): add optional metrics control for cost optimization #35154
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
Conversation
Add enableMetrics and enableObservabilityMetrics properties to SparkJobProps and RayJobProps interfaces, allowing users to disable CloudWatch metrics collection for cost control while maintaining backward compatibility. - Add conditional logic to exclude metrics arguments when disabled - Maintain defaults = true for backward compatibility - Apply same pattern to all 7 job types (6 Spark + 1 Ray) - Add comprehensive test coverage (8 new test cases) - Update README with cost optimization examples
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
There is insufficient integration test coverage.
| * | ||
| * When enabled, adds '--enable-metrics' to job arguments. | ||
| * | ||
| * @default true - metrics are enabled by default for backward compatibility |
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.
| * @default true - metrics are enabled by default for backward compatibility | |
| * @default true |
| role, | ||
| script, | ||
| enableMetrics: false, | ||
| enableObservabilityMetrics: true, |
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.
Should probably add a comment here indicating that this is optional, or remove this line from the example, since we explain that enableObservabilityMetrics is true by default.
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.
This test produces an app but doesn't validate whether metrics are emitted or not.
- Fix JSDoc @default comments to be drop-in values without explanatory text - Improve README example by removing redundant enableObservabilityMetrics line - Enhance integration test with AwsSdkCall assertions to validate actual job configurations - Add comprehensive API-level validation that metrics arguments are correctly included/excluded Addresses review feedback from @iankhou on PR aws#35154: - JSDoc @default values now follow CDK conventions - README example is cleaner and more accurate - Integration test now validates real AWS API responses instead of just deployment - Added assertions to verify --enable-metrics and --enable-observability-metrics arguments are properly handled in job DefaultArguments The enhanced integration test uses awsApiCall('Glue', 'getJob') to validate: - Jobs with disabled metrics don't have metrics arguments - Jobs with selective control have correct argument combinations - Default behavior maintains backward compatibility
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Comments on closed issues and PRs are hard for our team to see. |
Add enableMetrics and enableObservabilityMetrics properties to SparkJobProps and RayJobProps interfaces, allowing users to disable CloudWatch metrics collection for cost control while maintaining backward compatibility.
Issue # (if applicable)
Closes #35149.
Reason for this change
AWS Glue Alpha Spark and Ray jobs currently hardcode CloudWatch metrics enablement (
--enable-metricsand--enable-observability-metrics), preventing users from disabling these metrics to reduce CloudWatch costs. This is particularly important for cost-conscious environments where detailed metrics monitoring is not required, such as:Users have requested the ability to selectively disable these metrics while maintaining the current best-practice defaults for backward compatibility.
Description of changes
Core Implementation:
Extended SparkJobProps Interface:
Conditional Logic in SparkJob:
Parallel Implementation for RayJob:
RayJobPropsinterfaceDesign Decisions:
??): Used to provide safe defaults while allowing explicitfalsevaluesenableMetricsandenableObservabilityMetricsallow granular controlAlternatives Considered and Rejected:
enableAllMetricsproperty: Rejected for lack of granular controlFiles Modified:
lib/jobs/spark-job.ts: Interface extension + conditional logiclib/jobs/ray-job.ts: Parallel implementationtest/pyspark-etl-jobs.test.ts: 5 new test casestest/ray-job.test.ts: 3 new test casestest/integ.job-metrics-disabled.ts: Integration test (NEW)README.md: Documentation section addedDescribe any new or updated permissions being added
No new IAM permissions required. This change only affects the arguments passed to existing Glue jobs. The conditional logic excludes CloudWatch metrics arguments when disabled, but doesn't introduce new AWS API calls or require additional permissions.
The existing IAM permissions for Glue job execution remain unchanged:
glue:StartJobRunglue:GetJobRunglue:GetJobRunsDescription of how you validated changes
Unit Testing:
enableMetrics: false,enableObservabilityMetrics: true)Integration Testing:
integ.job-metrics-disabled.tsintegration testManual Testing:
Quality Assurance:
Test Examples:
Checklist
Additional Quality Checks:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license