feat(logs): configure custom subscription filter name by tam0ri · Pull Request #26498 · aws/aws-cdk

tam0ri · 2023-07-25T16:29:29Z

Currently, we can't set the subscription filter name as a prop for L2 SubscriptionFilter construct. This PR introduces the new prop filterName. This will let us set a specific name without requiring escape hatches.

Closes #26485

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

kaizencc

Couple comments @tam0ri. Thanks for taking this on!

kaizencc · 2023-07-25T19:56:41Z

packages/aws-cdk-lib/aws-logs/lib/log-group.ts

+  /**
+   * The name of the subscription filter.
+   */
+  readonly filterName?: string;


need a default here

Thank you for your comment! Sorry, I don't understand your intention. What do you mean need a default? As I described in the below conversation, I think it's better that filterName is optional because this can still let CDK/CFn generate physical id (filter name) automatically when this prop isn't specified.

For optional properties, we supply a default value in the documentation so our users know what it means when they don't supply anything:

/** * The name of the subscription filter. * * @default - a generated name is created for you */ readonly filterName?: string;

Makes sense! I aligned with logGroupName and logStreamName.

aws-cdk/packages/aws-cdk-lib/aws-logs/lib/log-group.ts

Lines 381 to 386 in e53e8dc

/**

* Name of the log group.

*

* @default Automatically generated

*/

readonly logGroupName?: string;

aws-cdk/packages/aws-cdk-lib/aws-logs/lib/log-group.ts

Lines 502 to 509 in e53e8dc

/**

* The name of the log stream to create.

*

* The name must be unique within the log group.

*

* @default Automatically generated

*/

readonly logStreamName?: string;

kaizencc · 2023-07-25T19:57:34Z

packages/aws-cdk-lib/aws-logs/lib/subscription-filter.ts

      destinationArn: destProps.arn,
      roleArn: destProps.role && destProps.role.roleArn,
      filterPattern: props.filterPattern.logPatternString,
+      filterName: this.physicalName,


wait, won't this change all subscription filters even if they don't supply props.filterName? Why is there a need to propagate the filterName to physicalName this way

Thank you for your comment!

won't this change all subscription filters even if they don't supply props.filterName?

In my understanding, if we don't specify filterName prop introduced by this PR, this.physicalName will be undefined and then filterName for L1 CfnSubscriptionFilter construct will be also undefined. As a result, FilterName porperty for AWS::Logs::SubscriptionFilter resource in generted CFn template will be omitted. I already tested with the sample app in my env.

Why is there a need to propagate the filterName to physicalName this way

I align this with the similar constructs such as LogGroup, LogStream or MetricFilter as below.

aws-cdk/packages/aws-cdk-lib/aws-logs/lib/log-group.ts

Lines 466 to 469 in 51b20f9

constructor(scope: Construct, id: string, props: LogGroupProps = {}) {

super(scope, id, {

physicalName: props.logGroupName,

});

aws-cdk/packages/aws-cdk-lib/aws-logs/lib/log-stream.ts

Lines 67 to 70 in 9a3399f

constructor(scope: Construct, id: string, props: LogStreamProps) {

super(scope, id, {

physicalName: props.logStreamName,

});

aws-cdk/packages/aws-cdk-lib/aws-logs/lib/metric-filter.ts

Lines 25 to 28 in 9a3399f

constructor(scope: Construct, id: string, props: MetricFilterProps) {

super(scope, id, {

physicalName: props.filterName,

});

From CFn perspective, if we specify FilterName property, this is used as physical id. Shouldn't we propagate the filterName to physicalName?

We can't, because that would be a breaking change for users who don't specify filterName. The behavior that existed before needs to be maintained, unfortunately.

In practice, if we really want to change the behavior we hide it behind a feature flag. But this ins't important enough to go down that route, imo.

Oh wait you're saying this.physicalName will be undefined if no filterName is given... hmm maybe thats the case but I would be surprised.

Nevermind, you are right :)

Pull request has been modified.

aws-cdk-automation · 2023-07-27T00:36:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6e876f1
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-07-27T17:55:36Z

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).

…7621) In #26498, I injected specific account id and region in integration test snapshot by mistake. This PR replaces them with variables by taking snapshot correctly. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(logs): configure custom subscription filter name

c278fec

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Jul 25, 2023

aws-cdk-automation requested a review from a team July 25, 2023 16:29

github-actions bot added p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Jul 25, 2023

aws-cdk-automation previously requested changes Jul 25, 2023

View reviewed changes

modiify README

aec8177

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 25, 2023

kaizencc previously requested changes Jul 25, 2023

View reviewed changes

Add fdefault valule of filterName for document.

0f6f5ec

tam0ri had a problem deploying to test-pipeline July 26, 2023 14:21 — with GitHub Actions Failure

Merge branch 'main' into feat/custom-subscription-filter-name

6e876f1

tam0ri requested a review from kaizencc July 27, 2023 08:37

kaizencc approved these changes Jul 27, 2023

View reviewed changes

mergify bot merged commit 7ddb305 into aws:main Jul 27, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

tam0ri mentioned this pull request Oct 20, 2023

chore(logs): remove hardcoded account id and region from snapshot #27621

Merged

	/**
	* Name of the log group.
	*
	* @default Automatically generated
	*/
	readonly logGroupName?: string;

	/**
	* The name of the log stream to create.
	*
	* The name must be unique within the log group.
	*
	* @default Automatically generated
	*/
	readonly logStreamName?: string;

	constructor(scope: Construct, id: string, props: LogGroupProps = {}) {
	super(scope, id, {
	physicalName: props.logGroupName,
	});

	constructor(scope: Construct, id: string, props: LogStreamProps) {
	super(scope, id, {
	physicalName: props.logStreamName,
	});

	constructor(scope: Construct, id: string, props: MetricFilterProps) {
	super(scope, id, {
	physicalName: props.filterName,
	});

Conversation

tam0ri commented Jul 25, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Jul 27, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants